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

[Aaron Levinson]

On 5/5/2017 2:26 AM, wm4 wrote:
> 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.)

That's not how ffmpeg.c behaves today.  It writes the file headers 
before it has any encoded packets for the _last_ output stream that is 
initialized.  With my change, I make sure that the file headers aren't 
written until after it has had an opportunity to generate encoded 
packets for at least one input frame for all output streams, although 
there are some FATE test cases in which input frames aren't ready yet by 
this point, which explains why the existence of the fallback in my patch 
is necessary.

It doesn't call do_video_out() any more times with my patch than prior 
to my patch.  It just changes the order of when check_init_output_file() 
is called with respect to do_video_out() (and do_audio_out()).

> 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).

The way the code is currently structured, do_video_out() is called by 
reap_filters() after it calls init_output_stream(), and it is 
init_output_stream() that makes the call to check_init_output_file(), 
which is where the call to avformat_write_header() is done. 
reap_filters() is the only function that calls do_video_out(), and I 
think it is pretty clear that this will be done after it calls 
init_output_stream() from looking at the source code.

My TODO has to do with setting up the field order based on header 
information, rather than waiting till it gets the first input frame. 
That's the theory at least, but I don't currently know how to do that in 
practice within ffmpeg.c.

Aaron Levinson