[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
Mon Nov 5 19:27:05 CET 2012


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.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- 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/20121105/056a8f3b/attachment.asc>


More information about the ffmpeg-devel mailing list