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

Michael Niedermayer michaelni at gmx.at
Thu Nov 22 05:37:07 CET 2012


On Tue, Nov 06, 2012 at 06:13:03PM +0000, Tim Nicholson wrote:
> 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.

patch applied for now, until a better solution is implemented

thanks

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121122/04757825/attachment.asc>


More information about the ffmpeg-devel mailing list