[FFmpeg-devel] [PATCH 4/9] lavf/oggdec: simplify destroying streams with chained audio streams.

[Reimar Döffinger]

On Sat, Sep 15, 2012 at 01:36:08AM +0200, Clément Bœsch wrote:
> On Sat, Sep 15, 2012 at 01:27:28AM +0200, Reimar Döffinger wrote:
> > On Sat, Sep 15, 2012 at 01:20:43AM +0200, Clément Bœsch wrote:
> > > nstreams is assumed to be 1 at that point, so the loop is pointless.
> > > ---
> > >  libavformat/oggdec.c | 9 +++------
> > >  1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > index 9d27393..05aeddd 100644
> > > --- a/libavformat/oggdec.c
> > > +++ b/libavformat/oggdec.c
> > > @@ -256,18 +256,15 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
> > >      idx = ogg_find_stream (ogg, serial);
> > >      if (idx < 0){
> > >          if (ogg->headers) {
> > > -            int n;
> > >  
> > >              if (ogg->nstreams != 1) {
> > >                  av_log_missing_feature(s, "Changing stream parameters in multistream ogg is", 0);
> > >                  return idx;
> > >              }
> > >  
> > > -            for (n = 0; n < ogg->nstreams; n++) {
> > > -                av_freep(&ogg->streams[n].buf);
> > > -                if (!ogg->state || ogg->state->streams[n].private != ogg->streams[n].private)
> > > -                    av_freep(&ogg->streams[n].private);
> > > -            }
> > > +            av_freep(&ogg->streams[0].buf);
> > > +            if (!ogg->state || ogg->state->streams[0].private != ogg->streams[0].private)
> > > +                av_freep(&ogg->streams[0].private);
> > 
> > The thing is that this is supposed to be only temporary until someone
> > implements the feature.
> > I don't think this simplifies the code enough to justify making the
> > code doing something semantically wrong ("free all streams" vs.
> > "free the first stream"), even if it has the same effect.
> 
> The problem I see with the current code is that the replace-code below
> really can't work properly if more than one stream. This is actually
> related to the issue I mentioned in the patchset introduction. And while
> trying to fix that issue, it's kind of hard to think about a situation
> (multiple streams) that can not happen at the moment.
> 
> So I'd prefer to make the code pretty explicit for the 1 stream situation,
> instead of making the developer who will implement the multistream feature
> think that it can mostly work, while not half of the following code is
> supposed to work in that situation: you have the current situation:
> 
>    1) if (nstreams != 1) not supported
>    2) for all streams do ...
>    3) ok now let's assume we only have one, but not explicitely.

Reading the code and your later patches I have a suspicion it's actually
semantically wrong.
It's probably only the stream that gets replaces that should be free'd,
not all of them.
Would be nice to understand it better before doing changes, but I am ok
with this patch anyway.