[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
Tue Nov 6 19:13:03 CET 2012
On 06/11/12 15:49, Michael Niedermayer wrote:
> On Tue, Nov 06, 2012 at 08:38:58AM +0000, Tim Nicholson wrote:
>> On 05/11/12 18:27, Michael Niedermayer wrote:
>>> On Mon, Nov 05, 2012 at 05:59:47PM +0000, Tim Nicholson wrote:
>>>> 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.
>>>
>>> odml avi spec:
>>>
>>> Video Properties Header (vprp)
>>> ...
>>> typedef struct {
>>> ...
>>> DWORD nbFieldPerFrame;
>>> ...
>>> } VideoPropHeader;
>>>
>>> our avienc.c:
>>> [...]
>>> avio_wl32(pb, stream->width );
>>> avio_wl32(pb, stream->height);
>>> avio_wl32(pb, 1); //progressive FIXME
>>>
>>> [...]
>>>
>>>>> 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.
>>>
>>> avi needs it too and it needs it at header writing time, i did not
>>> check other containers but likely mov and avi arent the only ones
>>> that can store this.
>>>
>>
>> Hang on a minute, now you are moving the goalposts. I did not deny that
>> other mixers might need interlace information to write into headers but
>> I have not been looking at their current mechanisms. I said that
>> currently only the mov muxer uses the AVCodecContext field_order
>> parameter in order to determine what needs writing, and this patch
>> specifically addresses the missing link in the chain, in that although
>> the code to write the information has been present in the muxer for some
>> time, there has been no code to actually set the value so this can be done.
>>
>> I believe this patch now addresses that issue using what you describe as
>> the "semantic" interlace condition, which I think we agree is the right
>> thing to do.
>>
>> You now seem to want to extend the scope of this patch so that all
>> muxers may (if someone bothers to write the code) also access this
>> parameter. This sounds like a sensible longer term aim, but would
>> require considerably more work to implement fully (based upon your your
>> suggestions of passing a new parameter through AVFilterLink).
>>
>> Can we not at least, for the moment, fix the problem where we can, and
>> deal with the bigger issues later?
>
> my goal was always to correctly define the API in a way that makes it
> work for all reasonable use cases and then work toward implementing
> this.
>
And I thank you for your timer and patience in this matter, as I have
groped towards a solution.
> As a maintainer of the relevant code iam happy to put my time into
> working toward this and iam happy to apply patches that move toward
> it in small steps.
>
> Is setting the AVCodecContext.field_order after writing the header
> and based on the first encoded frame usefull as part of a full
> implementation of field_order support ?
>
If you view the field_order parameter as pertaining solely to quicktime
for which it was created, then yes, until you have encoded that first
frame you don't readily have access to all the parameters you need to
determine the field_order setting and quicktime doesn't need it
immediately. If you view filed_order as part of the way to deal with
interlace flagging and tracking through the system globally then
probably not.
However I am not sure its necessarily the best approach for such a
global fix, as Tomas has already suggested AVCodecContext may not be the
best place for this flag and its current form is weighted towards
quicktimes obscure presentation/packing order with 4 possible values to
indicate interlace.
> Iam not sure it is but if its not then it will have to be eventually
> removed again. And the work around it has been wasted. I dont mind
> if you want to put your time into this hack and its maintaince and
> dont really mind applying it if you dont want to work on teh more
> complex and complete solution ?
>
Well currently the work has already been done, so not to use it at all
is a bigger waste. Perhaps for the moment I should keep it as a private
patch, although others have been interested in getting the fiel atom
implemented.
I am happy to look at the bigger issue as part of tidying up interlace
handling, but I need to understand some things a lot better first, and
need some time to focus on this. How adding/changing the parameter and
where it may be held (AVStream and extra bits in AVFilterLink) may
impact maintenance and merging from "the other branch" are all things I
need to think about. Having a single approach that works for all muxers
is definitely something I would want to aim at in the longer term though.
Thanks again for your guidance on this, and sorry it has taken up more
of your time than it should have.
> [...]
--
Tim
More information about the ffmpeg-devel
mailing list