[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 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.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/5cd621dc/attachment.asc>