[FFmpeg-devel] [PATCH 1 of 1] movenc: enable writing of interlace information back to the 'fiel' atom. (3rd Version)

Tim Nicholson nichot20 at yahoo.com
Mon Nov 5 18:59:47 CET 2012


On 05/11/12 15:05, Michael Niedermayer wrote:
> On Fri, Nov 02, 2012 at 01:15:11PM +0000, Tim Nicholson wrote:
>> On 01/11/12 22:01, Michael Niedermayer wrote:
>>> On Thu, Nov 01, 2012 at 03:31:42PM +0000, Tim Nicholson wrote:
>>>> On 01/11/12 14:53, Michael Niedermayer wrote:
>>>>> [..]
>>>>>
>>>>> This does not look safe.
>>>>> the encoder (that can run in a seperate thread) can free or change the
>>>>> coded_frame. Even if it zeros the pointer before freeing above is
>>>>> not atomic, the pointer is checked to be not NULL then loaded into
>>>>> a register and then top_field_first read based on this pointer.
>>>>> if the data is freed between these it can crash or produce undefined
>>>>> behavior.
>>>>>
>>>>
>>>> From what I could see the data is only freed within the *close function
>>>> of the encoder, but not during the *encode2 function. As the close
>>>> function(s) are called after the the output file(s) are flushed and
>>>> closed in the main ffmpeg transcode() function, I thought this would be
>>>> safe.
>>>
>>> does it work with applications other than ffmpeg itself ?
>>> also consider some application might use libavformat without a
>>> libavcodec based encoder. And muxers should have this information
>>> at their disposal if they need it before the first frame is submited
>>> to the encoder so it can be put in a header that is sent as early as
>>> possible.
>>>
>>>
>>>> If this is not the case then afaik the *only* safe thing is to set
>>>> the flag in the encoder encode function, but this will happen every
>>>> frame which feels OTT! (but would keep the code self contained) I am
>>>> happy to do it this way if it is acceptable.
>>>
>>> Would it be possible for the user application to set this information
>>> correctly before writing the file header ?
>>> That is that the muxer would then just read it out of AVCodecContext
>>> ?
>>>
>>> [...]
>>>
>>
>> Attached a new version that works  within ffmpeg when it is setting up
>> the other interlace flags the muxer then works as described above..
>>
>> I have followed the current logic that in the absence of user
>> intervention use the input settings, so that the value of
>> enc->field_order is in sync with big_picture.interlaced_frame &
>> big_picture.top_field_first. It could be argued that in the absence of
>> specific user setting of interlaced flags (ildct etc) the output should
>> be flagged as progressive, but in that case both sets of flags should be
>> forced to progressive to keep them in sync.
>>
>> As there have been reports of FCP assuming material is interlaced unless
>> the fiel atom says otherwise, I have made sure that the flag is set one
>> way or the other and not left undefined.
>>
>> Updated fate checksums included.
>>
>>
>> -- 
>> Tim
>>
>>
> 
>>  ffmpeg.c                             |   12 ++++++++++++
>>  tests/ref/fate/vsynth1-prores        |    4 ++--
>>  tests/ref/fate/vsynth1-prores_kostya |    4 ++--
>>  tests/ref/fate/vsynth1-qtrle         |    4 ++--
>>  tests/ref/fate/vsynth1-qtrlegray     |    4 ++--
>>  tests/ref/fate/vsynth1-svq1          |    4 ++--
>>  tests/ref/fate/vsynth2-prores        |    4 ++--
>>  tests/ref/fate/vsynth2-prores_kostya |    4 ++--
>>  tests/ref/fate/vsynth2-qtrle         |    4 ++--
>>  tests/ref/fate/vsynth2-qtrlegray     |    4 ++--
>>  tests/ref/fate/vsynth2-svq1          |    4 ++--
>>  11 files changed, 32 insertions(+), 20 deletions(-)
>> 98d67c2cbee74ef5df129869ebdbc3ed5cb29403  0001-ffmpeg-add-setting-of-field_order-flag.patch
>> From 57d13331b1d12055f674f13b670eab09577608c5 Mon Sep 17 00:00:00 2001
>> From: Tim Nicholson <Tim.Nicholson at bbc.co.uk>
>> Date: Fri, 2 Nov 2012 13:09:48 +0000
>> Subject: [PATCH] ffmpeg: add setting of field_order flag
>>
>> ---
>>  ffmpeg.c                             |   12 ++++++++++++
>>  tests/ref/fate/vsynth1-prores        |    4 ++--
>>  tests/ref/fate/vsynth1-prores_kostya |    4 ++--
>>  tests/ref/fate/vsynth1-qtrle         |    4 ++--
>>  tests/ref/fate/vsynth1-qtrlegray     |    4 ++--
>>  tests/ref/fate/vsynth1-svq1          |    4 ++--
>>  tests/ref/fate/vsynth2-prores        |    4 ++--
>>  tests/ref/fate/vsynth2-prores_kostya |    4 ++--
>>  tests/ref/fate/vsynth2-qtrle         |    4 ++--
>>  tests/ref/fate/vsynth2-qtrlegray     |    4 ++--
>>  tests/ref/fate/vsynth2-svq1          |    4 ++--
>>  11 files changed, 32 insertions(+), 20 deletions(-)
>>
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index 47a90da..81ee999 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -846,6 +846,10 @@ static void do_video_out(AVFormatContext *s,
>>             method. */
>>          enc->coded_frame->interlaced_frame = in_picture->interlaced_frame;
>>          enc->coded_frame->top_field_first  = in_picture->top_field_first;
>> +        if (enc->coded_frame->interlaced_frame)
>> +            enc->field_order = enc->coded_frame->top_field_first ? AV_FIELD_TB:AV_FIELD_BT;
>> +        else
>> +            enc->field_order = AV_FIELD_PROGRESSIVE;
>>          pkt.data   = (uint8_t *)in_picture;
>>          pkt.size   =  sizeof(AVPicture);
>>          pkt.pts    = av_rescale_q(in_picture->pts, enc->time_base, ost->st->time_base);
> 
> this does not look correct.
> the field_order flag should be set before writing the header, its
> just quicktime that uses it at trailer writing time
> 

And quicktime is the *only* muxer that uses it as far as my grepping
revealed. It was introduced as an element specifically to improve the
handling of the quicktime fiel atom (see 4bf3c8f2), its not currently
used to write any header information akaik.

> also we possibly need to clarify the distinction of semantic and
> syntactic interlacing, content can be interlaced or not independant of
> being coded as interlaced or not interlaced in the video bitstream.
> I am not sure mov makes this distinction but others like h.264 do.
> Its perfectly normal to have material flaged as progressive and
> coded as interlaced frames. That is for example also one could use
> ildct on progressive material and this should not turn the material
> into interlaced.
>

I agree with this distinction. The current logic means that
interlaced/progressive flagging is carried forward from the incoming
material (with tff/bff modified by any filters if required). Thus even
if ildct is requested, it doesn't change the flag from progressive to
interlaced (or the reverse if its not specified and the incoming is
interlaced).


> video codecs that do not allow these 2 to be different can make use
> of the semantic interlaceing information in their syntax. Which i
> belive applies to some codecs in mov.
> 
> One question open is what does AVCodecContext.field_order represent
> when the 2 are different. I think it should represent the semantic
> interlacing because noone really cares if a bitmap was coded interlaced
> its much more interresting if its content is interlaced in the sense
> of even/odd lines being captured at diffrent times.
> 

Which is how the patch currently behaves. Put interlaced material in.
code it with or without ildct or an interlace aware codec and the
quicktime flag will show the material as being interlaced. Put
progressive in and the reverse is true. This behaviour is modified only
by user intervention. For example using -vf setfield=prog. This I would
submit is exactly what is wanted, as it allows the user to override the
default behaviour for a specific reason (e.g the incoming is incorrectly
flagged as tff when its bff, or has been coded interlaced but has no
temporal difference between the fields)

> With this definition field_order would be similar to the aspect ratio
> and passing it can then likely be done the same way
> it would be needed to add a interlace / top_field_first to AVFilterLink
> probably and pass the flags through avfilter and update in some
> filters
> or it will be needed to inject frames throgh avfilter before the
> muxers write header is called to get this information.
> For now iam surely also happy without this and just passing directly
> when there are no filters and not at all if there are filters.
> AVFrame's interlaced_frame/top_field_first pass through the whole chain
> But maybe iam missing something and above is not the best solution?
> 

But surely AVFrame's interlaced_frame/top_field_first pass through the
whole chain already and provide that functionality, with the final
status available to set the field_order to the required value to match.

If field_order was required for header writing then maybe this extra
complication would be worthwhile, but as it is it adds extra
complication with no gain that I can see for this use case.


> 
>> @@ -868,6 +872,14 @@ static void do_video_out(AVFormatContext *s,
>>                  big_picture.top_field_first = !!ost->top_field_first;
>>          }
>>  
>> +        if (big_picture.interlaced_frame)
>> +            if (enc->codec->id == AV_CODEC_ID_MJPEG)
>> +                enc->field_order = big_picture.top_field_first ? AV_FIELD_TT:AV_FIELD_BB;
>> +            else
>> +                enc->field_order = big_picture.top_field_first ? AV_FIELD_TB:AV_FIELD_BT;
> 
> why the special case for mjpeg ?
>

Its quicktimes odd terminology of field presentation order, and packing
order blended into one flag. Originally everything I tried out of QT
Pro used the TB/BT settings, but then I cam across some MJPEG coded and
they all used the other modes, and I noticed that mjpegdec.c uses
similar flagging, so its entirely empirical.

> 
> [...]



-- 
Tim




More information about the ffmpeg-devel mailing list