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

[Stefano Sabatini]

On date Saturday 2009-04-11 00:43:35 +0200, Michael Niedermayer encoded:
> 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

Applied.
-- 
FFmpeg = Faboulous Fantastic Minimalistic Puritan Elastic Game