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

[Moritz Bunkus]

Hey,

some comments without having tried this out:

Matroska demuxer:

>  static mkv_track_t *
> @@ -1484,14 +1485,26 @@ display_tracks (mkv_demuxer_t *mkv_d)
>          {
>          case MATROSKA_TRACK_VIDEO:
>            type = "video";
> +          if (identify)
> +            mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_VIDEO_ID=%d\n", vid);
>            sprintf (str, "-vid %u", vid++);
>            break;
>          case MATROSKA_TRACK_AUDIO:
>            type = "audio";
> +          if (identify)
> +          {
> +            mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_AUDIO_ID=%d\n", aid);
> +            mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_AID_%d_LANG=%s\n", aid, mkv_d->tracks[i]->language);
> +          }
>            sprintf (str, "-aid %u, -alang %.5s",aid++,mkv_d->tracks[i]->language);
>            break;
>          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.

> +  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)?

Ogg demuxer:

>      else if (!strncasecmp(*cmt, "LANGUAGE=", 9))
>      {
>        val = *cmt + 9;
> +      if (identify)
> +      {
> +        if (ogg_d->subs[id].text)
> +          mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_SID_%d_LANG=%s\n", ogg_d->subs[id].id, val);
> +        else if (id != d->video->id)
> +          mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_AID_%d_LANG=%s\n", ogg_d->subs[id].id, val);
> +      }

-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.

> -      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:

> +      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;
>        n_audio++;

>  	  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. 

Mosu

-- 
If Darl McBride was in charge, he'd probably make marriage
unconstitutional too, since clearly it de-emphasizes the commercial
nature of normal human interaction, and probably is a major impediment
to the commercial growth of prostitution. - Linus Torvalds