[MPlayer-dev-eng] Re: [PATCH] -identify vids, aids, sids, alangs and slangs

[kiriuja]

...
> >          case MATROSKA_TRACK_SUBTITLE:
> >            type = "subtitles";
> > +          if (identify)
> > +          {
> > +            mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_SUBTITLE_ID=%d\n", sid);
> > +            mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_SID_%d_LANG=%s\n", sid, mkv_d->tracks[i]->language);
> > +          }
> >            sprintf (str, "-sid %u, -slang %.5s",sid++,mkv_d->tracks[i]->language);
> >            break;
> >          }
> 
> This is too early. The language might not be known at this place. The
> output should be moved to the end of the 'track entry' parsing.

Hmm... I don't think I understand. The only place I found where the
language is set is in demux_mkv_read_trackentry, which is called from
demux_mkv_read_tracks. And display_tracks is called only after
demux_mkv_open is done calling demux_mkv_read_tracks. So why is it
too early? Did I miss something?

> > +  demux_aid_vid_mismatch = 1; // don't identify in new_sh_* since ids don't match
> 
> I don't fully understand what this "ids don't match" means. Could you
> please clarify this somewhere (in the patch as comments for the code)?

The patch normally prints the ids demuxers pass to new_sh_audio and
new_sh_video, since they usually match aids and vids respectively.
Some demuxers use zero based ids (vid=0,1,... and aid=0,1,..., for
example demux_mpg), others use sequential ids, either zero based
(if vid=0 then aid=1,2,..., for example demux_avi and demux_mov)
or one based (vid=1, aid=2,... or aid=1, vid=2, for example demux_asf).
Whatever the numbering, what is passed to new_sh_* is the actual aid
or vid that it would accept from the command line, so that d->audio->id
matches the current aid and d->video->id matches the current vid.

However, demux_mkv and demux_ogg accept zero-based aids and vids but
use sequential numbering internally, and that is what gets passed to
new_sh_*, so generally d->audio->id does not match the aid and
d->video->id does not match the vid. For subtitles demux_mkv accepts
zero based sids from the command line, but demux_ogg accepts the actual
sid it uses internally. I was testing with an OGM file that has one
video, two audio and one subtitle stream, and the ids demux_ogg
accepted were -vid 0, -aid 0, -aid 1 and -sid 3.

The only easy way to fix this mess I could think of was to change the
accepted aids and vids to match the internal ones, but I understand
you (and Richard) object to that, so I am not going to argue. So the
demux_aid_vid_mismatch is the workaround that tells new_sh_* not to
print the ids it gets, in which case it is the demux module that prints
them. I will add a short comment to that effect. I also forgot to reset
the flag for each new file, will fix tomorrow.

> -alang doesn't work for Ogg (yet), just so you know, but this would be
> the right place to output it. So let's keep it.

I understand, but a language name is useful for displaying in a GUI.

> > -      n_audio++;
> > +      if (identify)
> > +        mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_AUDIO_ID=%d\n", n_audio);
> > +      ogg_d->subs[ogg_d->num_sub].id = n_audio++;
> 
> Just a nitpick: This could always be one line shorter:

The patch will be one line shorter, but the resulting code one line
longer. Whatever your preference. :-)

> >     ogg_d->subs[ogg_d->num_sub].samplerate= get_uint64(&st->time_unit)/10;
> >     ogg_d->subs[ogg_d->num_sub].text = 1;
> > +   if (identify)
> > +     mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_SUBTITLE_ID=%d\n", ogg_d->num_sub);
> > +          ogg_d->subs[ogg_d->num_sub].id = ogg_d->num_sub;
> >            if (demuxer->sub->id == ogg_d->n_text)
> >              text_id = ogg_d->num_sub;
> >            ogg_d->n_text++;
> 
> This is definitely wrong because what you store as the id is the number
> of streams encountered so far. You have to use ogg_d->n_text instead. 

As mentioned above, in my testing demux_ogg matches a -sid to the
index in the subs[] array rather than to n_text. I only got subtitles
when I passed -sid 3, and not with any other -sid.

Another minor problem in demux_ogg is that demux_ogg_check_comments
only gets called for streams that are actually going to be used, and
so the other stream languages are not known until they are actually
played. But I am guessing there is not easy fix for that.

Thanks for looking at the patch.

--
kiriuja