[FFmpeg-devel] [PATCH 1/2] avcodec/avpacket: Use av_copy_packet_side_data() in av_packet_copy_props()

[wm4]

On Wed, 3 May 2017 11:55:16 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, May 03, 2017 at 11:50:41AM +0200, Michael Niedermayer wrote:
> > On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:  
> > > On Wed, 3 May 2017 11:29:04 +0200
> > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > >   
> > > > On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:  
> > > > > On Wed,  3 May 2017 05:21:50 +0200
> > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > >     
> > > > > > Fixes timeout
> > > > > > Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
> > > > > > 
> > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > > ---
> > > > > >  libavcodec/avpacket.c | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > > > > > index 4bf830bb8a..ff7ee730a4 100644
> > > > > > --- a/libavcodec/avpacket.c
> > > > > > +++ b/libavcodec/avpacket.c
> > > > > > @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > > > >      dst->flags                = src->flags;
> > > > > >      dst->stream_index         = src->stream_index;
> > > > > >  
> > > > > > +    if (!dst->side_data_elems);
> > > > > > +        return av_copy_packet_side_data(dst, src);
> > > > > > +
> > > > > >      for (i = 0; i < src->side_data_elems; i++) {
> > > > > >           enum AVPacketSideDataType type = src->side_data[i].type;
> > > > > >           int size          = src->side_data[i].size;    
> > > > > 
> > > > > This doesn't look right...    
> > > > 
> > > > already fixed the ; locally
> > > > 
> > > > 
> > > > [...]  
> > > 
> > > I didn't see that, I was referring to the fact that you call
> > > av_copy_packet_side_data(), and only sometimes (at least by intention).
> > > That requires at least an explanation in the commit message.  
> > 
> > av_packet_copy_props() would add side data to the destination packet
> > it doesnt replace previously existing side data except in case of
> > error.
> > I dont know if that is intended but i didnt want to change it as that
> > would be unrelated to this patch  
> 
> added
> "av_copy_packet_side_data() is only used when it does not lead to a change in behaviour"
> to the commit message

Sorry, that seems all kinds of hacky, especially with the "suspected"
reason that the function is unexpectedly faster than the existing code
with some memory analyzers. It just doesn't make sense to me.