[MPlayer-dev-eng] [PATCH] DVD subpicture/audio stream mapping

[Reimar Döffinger]

Hi,
btw. in your first post you said some subtitles couldn't be selected
even with -sid. Did -vobsubid work? And does this patch change how
-vobsubid behaves?

On Tue, Sep 20, 2005 at 11:41:50AM +0200, Lehel Bernadt wrote:
> Well I'm glad you asked because nobody has been interested in applying it :) 

Hey, we are just a bit slow, especially when it comes to unmaintained
code nobody understands *g*

> Since then I did a lot of tests, made some changes, and I think now the 
> patched mplayer works correctly in every case.

With the kind of crap you get on some DVDs I almost doubt this is
possible. But sure better than the current thing (though I haven't
tested it yet).

A few small comments...

> --- MPlayer-cvs-20050920/libmpdemux/stream_dvd.c	2005-05-23 00:35:44.000000000 +0200
> +++ Mplayer-mine/libmpdemux/stream_dvd.c	2005-09-20 11:33:34.000000000 +0200
> @@ -1,5 +1,3 @@
> -
> -
>  #include <ctype.h>

cosmetics, remove.

> @@ -581,21 +581,17 @@
>              d->audio_streams[d->nr_of_channels].id=0;
>              switch(audio->audio_format) {
>                case 0: // ac3
> -                d->audio_streams[d->nr_of_channels].id=ac3aid;
> -                ac3aid++;
> +                d->audio_streams[d->nr_of_channels].id=ac3aid+vts_file->vts_pgcit->pgci_srp[ttn].pgc->audio_control[i].s_audio;
>                  break;
>                case 6: // dts
> -                d->audio_streams[d->nr_of_channels].id=ac3aid+8;
> -                ac3aid++;
> +                d->audio_streams[d->nr_of_channels].id=ac3aid+8+vts_file->vts_pgcit->pgci_srp[ttn].pgc->audio_control[i].s_audio;
>                  break;
>                case 2: // mpeg layer 1/2/3
>                case 3: // mpeg2 ext
> -                d->audio_streams[d->nr_of_channels].id=mpegaid;
> -                mpegaid++;
> +                d->audio_streams[d->nr_of_channels].id=mpegaid+vts_file->vts_pgcit->pgci_srp[ttn].pgc->audio_control[i].s_audio;
>                  break;
>                case 4: // lpcm
> -                d->audio_streams[d->nr_of_channels].id=pcmaid;
> -                pcmaid++;
> +                d->audio_streams[d->nr_of_channels].id=pcmaid+vts_file->vts_pgcit->pgci_srp[ttn].pgc->audio_control[i].s_audio;
>                  break;

IMHO you should clean this up a bit. I would suggest
d->audio_streams[d->nr_of_channels].id =
vts_file->vts_pgcit->pgci_srp[ttn].pgc->audio_control[i].s_audio;
outside the switch() and then inside the switch e.g.
d->audio_streams[d->nr_of_channels].id += 128;
And remove the *aid variables, if you want it for readability reason,
maybe add defines like
#define FIRST_AC3_AID 128
#define FIRST_DTS_AID 136
etc. Not sure if that really make sense though.

> -      if(vts_file->vts_pgcit->pgci_srp[0].pgc->subp_control[i] & 0x80000000) {
> -        subp_attr_t * subtitle = &vts_file->vtsi_mat->vts_subp_attr[i];
> +      if(vts_file->vts_pgcit->pgci_srp[ttn].pgc->subp_control[i].present) {
> +        subp_attr_t *subtitle = &vts_file->vtsi_mat->vts_subp_attr[i];
> +        video_attr_t *video = &vts_file->vtsi_mat->vts_video_attr;
> +	
cosmetics (I think) you only removed the space after subp_attr_t *,
right?
Not sure if I like that "empty" line either, esp. since it contains
whitespace.

>          d->subtitles[ d->nr_of_subtitles ].language=language;
> -        d->subtitles[ d->nr_of_subtitles ].id=d->nr_of_subtitles;
> +	
> +        if(video->display_aspect_ratio == 0) /* 4:3 */
> +        {
> +                d->subtitles[d->nr_of_subtitles].id = vts_file->vts_pgcit->pgci_srp[ttn].pgc->subp_control[i].s_4p3;
> +        }
> +        else if(video->display_aspect_ratio == 3) /* 16:9 */
> +        {
> +                d->subtitles[d->nr_of_subtitles].id = vts_file->vts_pgcit->pgci_srp[ttn].pgc->subp_control[i].s_lbox;
> +        }
> +        else d->subtitles[d->nr_of_subtitles].id = 0; /* maybe some fallback here? */

Use the old code in the else part (without changing indentation).

>          mp_msg(MSGT_OPEN,MSGL_V,"[open] subtitle ( sid ): %d language: %s\n", d->nr_of_subtitles, tmp);
>          if(identify) {
> -          mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_SUBTITLE_ID=%d\n", d->nr_of_subtitles);
> +          mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_SUBTITLE_ID=%d\n", d->subtitles[d->nr_of_subtitles].id);
>            if(language && tmp[0])
>              mp_msg(MSGT_GLOBAL, MSGL_INFO, "ID_SID_%d_LANG=%s\n", d->nr_of_subtitles, tmp);

Shouldn't that be d->subtitles[d->nr_of_subtitles].id here, too?

> diff -ruN MPlayer-cvs-20050920/mplayer.c Mplayer-mine/mplayer.c
> --- MPlayer-cvs-20050920/mplayer.c	2005-09-20 08:59:58.000000000 +0200
> +++ Mplayer-mine/mplayer.c	2005-09-20 09:07:37.000000000 +0200
> @@ -339,6 +339,7 @@
>  static demuxer_t *demuxer=NULL;
>  static sh_audio_t *sh_audio=NULL;
>  static sh_video_t *sh_video=NULL;
> +static dvd_priv_t *dvdpriv=NULL;

That doesn't have to be a global variable, does it?

> @@ -3623,7 +3624,8 @@
>              if (d_dvdsub) {
>  #ifdef USE_DVDREAD
>                  if (vo_spudec && stream->type == STREAMTYPE_DVD) {
> -                    d_dvdsub->id = dvdsub_id;
> +                    dvdpriv = (dvd_priv_t*)stream->priv;
> +                    d_dvdsub->id = dvdpriv->subtitles[dvdsub_id].id;

actually d_dvdsub->id =
((dvd_priv_t*)stream->priv)->subtitles[dvdsub_id].id;
should work as well, although it is ugly.
But I really don't like d_dvdsub->id and dvdsub_id having different
values too much. Maybe add a comment that says which contains what...

Greetings,
Reimar Döffinger