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

[Lehel Bernadt]

Hi!

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

Yes, -vobsubid does work, and I didn't touch it. 

This patch corrects the procedure figuring out which subpicture stream 
corresponds to which sid/slang for the current title; while -vobsubid 
directly selects the specified subpicture stream.

As an example, until now "-sid N" selected stream nr. N. 
Now I check in the IFO which stream is really mapped to sid N (let it be X), 
and store it in d->subtitles[N].id. 
This stream can be directly selected by "-vobsub X", but there should be no 
reason to do it, once sid/slang can select it correctly.

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

Of course, I only mean _theoretically_ :)

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

Done.

>
> > @@ -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_s
> >rp[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_s
> >rp[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.

Good ideas, I've rewritten this part.

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

Yes, to fix the style which is "type *pointer" throughout the whole file, 
except here.

> Not sure if I like that "empty" line either, esp. since it contains
> whitespace.

Right, it was unneeded.

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

Done.

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

No, it's correct this way because here we print the sid - slang map, and the 
sid value is d->nr_of_subtitles, while the corresponding subpicture number is 
in d->subtitles[d->nr_of_subtitles].id.

The message above I modified to print the subpic IDs so to check if the 
correct ones are selected.
Before it would just print 0...d->nr_of_subtitles, which has about 0 
informational value.

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

Not really :) It just ended up here because of the other similar variables.
I've moved it to the innermost code block.

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

I think we should keep it this way for readability reasons. I already 
inherited the variables, but I agree that the naming is unfortunate. I have 
added some explaining comment.

New patch is attached.

Regards,
Lehel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer-dvd-20050927.diff
Type: text/x-diff
Size: 12017 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20050928/9f0850e3/attachment.diff>