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

[Michael Niedermayer]

On Thu, Nov 01, 2012 at 03:31:42PM +0000, Tim Nicholson wrote:
> On 01/11/12 14:53, Michael Niedermayer wrote:
> > On Thu, Nov 01, 2012 at 12:05:01PM +0000, Tim Nicholson wrote:
> >> On 26/10/12 13:52, Tim Nicholson wrote:
> >>> On 26/10/12 12:34, Tomas Härdin wrote:
> >>>>> +    /* If field_order has set by the coder to indicate interlace coding
> >>>>> +     * update value to reflect current coded top_field_first status */
> >>>>> +    if ((track->enc->coded_frame) && (track->enc->field_order > AV_FIELD_PROGRESSIVE))
> >>>>
> >>>> Useless parentheses
> >>>
> >>> Oops, forgot to clean them out when I changed the test.
> >>
> >> No other commentsa so fixed and rebased, ping.
> >>
> >>
> >> -- 
> >> Tim
> >>
> >>
> > 
> >>  movenc.c |    5 +++++
> >>  1 file changed, 5 insertions(+)
> >> 899df4bbab88b0570f6f56d31227627445467848  0001-movenc-Update-field_order-flag-to-reflect-coded-fram.patch
> >> From 3754feef44c6d5c36feb6ec4ee01bb355af98fff Mon Sep 17 00:00:00 2001
> >> From: Tim Nicholson <Tim.Nicholson at bbc.co.uk>
> >> Date: Thu, 1 Nov 2012 11:59:20 +0000
> >> Subject: [PATCH] movenc: Update field_order flag to reflect coded frame
> >>  top_field_first flag
> >>
> >> ---
> >>  libavformat/movenc.c |    5 +++++
> >>  1 files changed, 5 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> >> index 3f8831d..5b931da 100644
> >> --- a/libavformat/movenc.c
> >> +++ b/libavformat/movenc.c
> >> @@ -1103,6 +1103,11 @@ static int mov_write_video_tag(AVIOContext *pb, MOVTrack *track)
> >>      avio_wb32(pb, 0); /* Data size (= 0) */
> >>      avio_wb16(pb, 1); /* Frame count (= 1) */
> >>  
> >> +    /* If field_order has set by the coder to indicate interlace coding
> >> +     * update value to reflect current coded top_field_first status */
> >> +    if (track->enc->coded_frame && (track->enc->field_order > AV_FIELD_PROGRESSIVE))
> >> +            track->enc->field_order = track->enc->coded_frame->top_field_first ? AV_FIELD_TB:AV_FIELD_BT;
> > 
> > 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
?

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

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- 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/20121101/d1d61576/attachment.asc>