[FFmpeg-devel] [PATCH] avoid crashes if input stream size changes without using -r

[Michael Niedermayer]

On Mon, Jul 06, 2009 at 01:59:15PM +0200, Reimar D?ffinger wrote:
> On Mon, Jul 06, 2009 at 01:23:15PM +0200, Michael Niedermayer wrote:
> > On Mon, Jul 06, 2009 at 10:57:26AM +0100, M?ns Rullg?rd wrote:
> > > Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
> > > 
> > > > On Mon, Jul 06, 2009 at 10:28:38AM +0100, M?ns Rullg?rd wrote:
> > > >> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
> > > >> > On Mon, Jul 06, 2009 at 05:30:56AM +0200, Michael Niedermayer wrote:
> > > >> >> On Sun, Jul 05, 2009 at 01:45:45PM +0200, Reimar D?ffinger wrote:
> > > >> >> > I don't know why, but currently FFmpeg only checks for an input stream
> > > >> >> > size change when it is using resizing.
> > > >> >> 
> > > >> >> id guess, the idea might have been to allow size changes on the encoder
> > > >> >> side too when supported (trivial for intra only codecs)
> > > >> >
> > > >> > The code for that isn't there though (rawenc with the current code could
> > > >> > already handle that fine), FFmpeg never gives the encoder the slightest
> > > >> > hint that the resolution changed (avctx width/height stay at the
> > > >> > original values)...
> > > >> 
> > > >> Then we should fix that.
> > > >
> > > > That would mean at the same time fixing all encoders to handle it or
> > > > there will still be crashes.
> > > 
> > > CODEC_CAP_*
> > 
> > yes, a CODEC_CAP_SIZE_CHANGE or some better name could be added and the
> > common code in lavc could maybe try to detect unexcpected size changes
> > and thus prevent crashes&sec holes for codecs not supporting it
> 
> I guarantee you I will not validate any encoder if it manages to support
> this correctly.

i never asked for that


> I think it is a pain and makes for brittle code when you could get
> _exactly_ the same thing (if really wanted) by just closing and reopening
> the encoder, at least for all video formats that FFmpeg currently supports.
> Note that the same issue also exists for pix_fmt changes.

> And as an example in FFmpeg, checking in common lavc code would change
> the assumption that an error during decoding should always be fatal,
> e.g. a single frame with the wrong size due to a few bits being
> corrupted is not really a reason to give up completely.

the error reporting abilities of our API are in dear need of improvment, that
said iam not sure in which case the problem you describe could happen
few demuxers could change the size at all, mov would be changing streams as
well for a size change i suspect so it would be 2 different decoders but
an application could route them to the same encoder, or it could be said
it has to if it wants to transcode in a non mov format.


> Overall it seems to me that doing the check in lavc will not save
> applications using lavc much code (if any)...

maybe, and iam not really suggestinng to solve the issue in any specific
way, ive also already said that i dont know if we should support size
changes in encoders, some like mpeg1/2 could do it though ...

also a generic (AVOption based) check could be added to lavc to check that
fields that are expected to stay constant actually are.


> 
> > also it could be used in decoders too catching changed w/h from demuxers
> 
> That seems like rather low priority to me, for any correct decoder there
> shouldn't be any crashes since get_buffer gets them a buffer of the
> right size (hopefully).

I think i rememer that one codec at least did have a problem with that,
also there are many other tables that are based on width and height like
mb types, mv vectors, ...
its very easy to forget some of these, in that sense a regression test that
tested size changes for all decoders would be great


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090706/a3a14677/attachment.pgp>