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

[Tim Nicholson]

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


> [...]



-- 
Tim