[FFmpeg-devel] [PATCH] API changes in ffmpeg.c

[Thilo Borgmann]

Michael Niedermayer schrieb:
> On Fri, Apr 10, 2009 at 09:11:10PM +0200, Thilo Borgmann wrote:
>   
>> Michael Niedermayer schrieb:
>>     
>>> On Fri, Apr 10, 2009 at 08:29:25PM +0200, Thilo Borgmann wrote:
>>>   
>>>       
>>>> Michael Niedermayer schrieb:
>>>>     
>>>>         
>>>>>         
>>>>>           
>>>>>>>>            &picture, &got_picture, ptr, len);
>>>>>>>> +                    ret = avcodec_decode_video2(ist->st->codec,
>>>>>>>> +                                               &picture, 
>>>>>>>> &got_picture, pkt);
>>>>>>>>                         
>>>>>>>>                 
>>>>>>> Crash here.
>>>>>>>                   
>>>>>>>               
>>>>>> This is revision 1 which fixes the crash during make test.
>>>>>>             
>>>>>>             
>>>>> probably ok if tested
>>>>>
>>>>>         
>>>>>           
>>>> Although applied, I tested it again using ffmpeg for transcoding the 
>>>> corepng.avi into yuv format.
>>>>
>>>> This revealed a bug in my ffmpeg.c patch, but this bug breaks CorePNG 
>>>> decoding only, since this is the only one using more information of the 
>>>> provided packet than .data and .size attributes.
>>>>
>>>> FFplay is not broken as it uses no local AVPackets but the "original" 
>>>> one.
>>>>
>>>> I attached a little patch for the time being, but I would propose to add 
>>>> a "av_copy_packet" function into libavcodec/avpacket.c as future codecs 
>>>> will also use more attributes of the avpacket and even others than just 
>>>> the .flags attribute.
>>>>     
>>>>         
>>> patch ok
>>>   
>>>       
>> While I was thinking about a possible copy function, I realized that a 
>> shallow copy would be enough here, so a simple avpkt = *pkt will do. A 
>> function for a deep copy is not yet needed so I'm not going to add one.
>>
>> Reinspecting ffmpeg.c, revision 1 does solve it best in my eyes, so the 
>> last patch here has become obsolete, if Michael agrees.
>>
>> Of course, this new patch has been tested using "make test" and the 
>> transcoding into yuv.
>>
>> TB
>>     
>
>   
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index 68f84fe..fb841c4 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -1185,7 +1185,6 @@ static int output_packet(AVInputStream *ist, int ist_index,
>>      int got_subtitle;
>>      AVPacket avpkt;
>>  
>> -    av_init_packet(&avpkt);
>>  
>>      if(ist->next_pts == AV_NOPTS_VALUE)
>>          ist->next_pts= ist->pts;
>> @@ -1195,13 +1194,13 @@ static int output_packet(AVInputStream *ist, int ist_index,
>>          avpkt.data = NULL;
>>          avpkt.size = 0;
>>          goto handle_eof;
>> +    } else {
>> +        avpkt = *pkt;
>>      }
>>     
>
> i suspect that this can end with partially initialized avpkt
>
>   
In any case the av_init_packet() call is redundant or unneeded.
Ok for a "clean" code I can put it back in, may be time will come when 
ffmpeg.c makes use of more than .data and .size for the pkt==NULL case.
Although, I would like to put it back into the case where avpkt = *pkt 
is not called, not to use redundant assignments most of the time during 
decoding.
I think we all are happy with that?

See revision 2.

TB
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tb.ffmpegHotfix.rev2.patch
Type: text/x-patch
Size: 889 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090410/c81ea172/attachment.bin>