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

[Michael Niedermayer]

On Fri, Apr 10, 2009 at 11:11:10PM +0200, Thilo Borgmann wrote:
> 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?

ok

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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090411/e16bd3b5/attachment.pgp>