[FFmpeg-devel] [PATCH 4/5] ogg: allow streams to update metadata

[wm4]

On Sun, 20 Oct 2013 09:16:47 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> 
> 
> On 20.10.2013, at 09:13, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
> > On 19.10.2013, at 22:52, wm4 <nfxjfg at googlemail.com> wrote:
> >> On Sat, 19 Oct 2013 22:43:27 +0200
> >> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> >> 
> >>> On 19.10.2013, at 15:40, Ben Boeckel <mathstuf at gmail.com> wrote:
> >>>> 
> >>>> +        av_free(os->new_metadata);
> >>>> +        os->new_metadata      = NULL;
> >>> 
> >>> These (appearing in two places) can be merged into a single av_freep.
> >>> 
> >>>> @@ -639,6 +646,7 @@ static int ogg_read_close(AVFormatContext *s)
> >>>>           ogg->streams[i].codec->cleanup(s, i);
> >>>>       }
> >>>>       av_free(ogg->streams[i].private);
> >>>> +        av_free(ogg->streams[i].new_metadata);
> >>>>   }
> >>>>   av_free(ogg->streams);
> >>> 
> >>> Not really related to your patch, but I wonder why none of these use av_freep.
> >> 
> >> Why would they? The whole streams array is freed right after this.
> > 
> > Someone might have a stray pointer to them around?
> > So from my point of view it doesn't change things that much whether it is freed or not, av_freep should still be a (little) bit more robust.

I don't really appreciate this whole av_freep() thing, because it's
questionable C, and also it's way too easy to accidentally pass a
pointer instead of a pointer to a pointer. (Maybe av_freep() should
really be a macro?)

Not sure why you'd claim that it improves the situation with dangling
pointers, because at this point you already lost, and it'll probably be
harder to debug.

> I see that Michael changed it already so I guess the discussion is kind of moot though.

True.