[FFmpeg-devel] [PATCH] Fixed bug encountered when decoding interlaced video

[wm4]

On Fri, 5 May 2017 01:11:17 -0700
Aaron Levinson <alevinsn at aracnet.com> wrote:

> > As I said on IRC, I'm skeptic against this, but I'm also not sure
> > whether I understand the situation.
> >
> > First, your change seems a bit fragile, and almost relies on
> > coincidences. This could easily break in the future. (And if we add a
> > FATE test, it would probably hit random people trying to change
> > ffmpeg.c, and would cause them more work.)  
> 
> I guess I don't understand why you think this change is fragile, relies 
> on coincidences, and could easily break in the future.

Well, the implemented and working logic is that ffmpeg.c waits until it
has encoded packets for every stream, and then writes the file headers.
That should work pretty well to let metadata naturally "flow" through
decoder, filters, and encoder. But your case doesn't fit right into it,
so you apparently have to make it somehow call do_video_out() more.
(Also a thing I don't understand - do_video_out() should have been
called at least once to encode a video packet.)

And looking at how field_order is set, the existing code doesn't make
sense in the first place.

So maybe it would be better to fix the actual problem, instead of
adding another workaround.

At least that's my thinking.

>  The existing 
> code is already fragile.

Indeed. That (and me being sleep deprived) probably contributes to that
I just don't understand your fix.

Also feel free to ignore me - I'm neither maintainer of ffmpeg.c nor an
authority on it.

>  As I indicated in previous e-mails on this 
> topic, the behavior of the current code can change depending on timing 
> when there is more than one stream.  This patch should make things more 
> deterministic.  And, I do think it is appropriate to add an interlaced 
> video decoding FATE test, although there is no point to doing so until 
> this bug is fixed, as the current behavior is broken.
> 
> > Looking at the current do_video_out() function (which btw. is way too
> > long and also has broken indentation), I see the following line:
> >
> >   mux_par->field_order = AV_FIELD_PROGRESSIVE;
> >
> > (and similar assignments to field_order to set interlaced modes)
> >
> > This is apparently run on every frame. This looks pretty wrong to me.
> > It should _never_ change the codecpar after headers were written. This
> > is simply invalid API use.
> >
> > If this is really a parameter that can change per frame, and that the
> > _muxer_ needs this information (per packet I suppose), a side data
> > signaling the field order should be added. mpegtsenc.c doesn't seem to
> > read field_order at all, though.
> >
> > So I remain confused.  
> 
> Yes, I agree that the approach of changing field_order for every frame 
> is not the right way to go, but this code was already existing.  A 
> future patch could undertake the task of handling interlaced video in a 
> more proper way, and I wrote a TODO comment to that point.

I don't understand that comment either. As I already said,
do_video_out() has to be _necessarily_ called before the muxer can be
initialized (aka writing headers).