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

[Clément Bœsch]

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.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120915/d226bc6e/attachment.asc>