[MPlayer-cvslog] r26301 - in trunk: libmpdemux/demux_lavf.c?libmpdemux/demux_mkv.c libmpdemux/demuxer.c libmpdemux/demuxer.h?libmpdemux/stheader.h mencoder.c mpcommon.c mplayer.c

[Reimar Döffinger]

On Sat, Apr 05, 2008 at 01:36:36PM +0400, Evgeniy Stepanov wrote:
> On Saturday 05 April 2008 13:15:52 Reimar Döffinger wrote:
> > On Sun, Mar 30, 2008 at 06:55:46PM +0200, eugeni wrote:
> > > +int demuxer_default_audio_track(demuxer_t* d)
> > > +{
> > > +    int i;
> > > +    for (i=0; i < MAX_A_STREAMS; ++i) {
> > > +        sh_audio_t* sh = d->a_streams[i];
> > > +        if (sh && sh->default_track)
> > > +            return sh->aid;
> > > +    }
> > > +    for (i=0; i < MAX_A_STREAMS; ++i) {
> > > +        sh_audio_t* sh = d->a_streams[i];
> > > +        if (sh)
> > > +            return sh->aid;
> > > +    }
> >
> > What is this part supposed to be good for? Exactly this should happen
> > anyway when you just return -1. And while the real bug is elsewhere,
> > this part breaks audio for nsv files.
> 
> Yes, this is done in every demuxer, and this results in code duplication, 
> possible different behavior, unsupported features (for instance, lavf demuxer 
> did not support default tracks before this change).

The lower block is not necessary for default tracks though and is what
caused the problem. Probably it is a good idea to have to avoid code
duplication, but "smuggling" it in as a part of a patch to add support
for default tracks, to which it is not related in any way, was a bad
idea since it changed behaviour also for demuxers that do not support
default tracks and wasted about an additional 10 minutes of my time
since I had to re-read the whole patch in detail to understand what the
heck was going on.