[MPlayer-dev-eng] Re: [PATCH] Make all subtitles availiable

[Reimar Döffinger]

Hello,
On Thu, Feb 15, 2007 at 04:29:40PM +0100, Lehel Bernadt wrote:
> On Thursday 15 February 2007 14:53, Reimar Döffinger wrote:
> > On Thu, Feb 15, 2007 at 02:17:21PM +0100, Lehel Bernadt wrote:
> > > I've tried that approach, and "might behave a bit weird" = "gets broken"
> > > in this case. I hadn't investigated why, I guess it would need to be
> > > checked in the sub switching code whether a sub id corresponds to a real
> > > sub or is a "hole".
> >
> > Huh? It just has some extra subtitles that have no language assigned and
> > probably won't display anything. Not beautiful, but really who cares?
> 
> I just wrote that when I tried it out, it broke the subtitle switching. That's 
> all.

Can you provide details? We might have different standards for
"breaking" ;-). Note also that I said the function returning the number
of subtitles must be adjusted, and different than in the original patch.

> > > The other problem with your idea is, that returning with the number of
> > > the physical subpic streams (i.e. all of them) instead of the number of
> > > the logical subs (as listed in the DVD title) breaks the sub
> > > identification, since that info is read from the DVD title, and not from
> > > the MPEG2 stream. That's why I think that my patch is OK as it is.
> >
> > Huh??? The number in the DVD title description is the id in the MPEG-PS
> > stream. 
> 
> The number in the DVD title description is _not_ the stream id, they are two 
> different things. See the attached screenshots.

We might be talking about completely different things, so thanks for the
pictures.
I guess "in the DVD title description" was a stupid expression, the
MPEG-PS ids are available and already read from the DVD stream layer was
basically what I wanted to say.

> > Actually the thing that currently gets passed around is some 
> > made-up number that does not really have anything to do with what is
> > stored in the DVD title description AFAICT.
> 
> It's not "made up", it's actually what is stored in the DVD title description.

Now I do not have the specs so can not say if this is valid, but looking
at your first screenshot, it is possible that in-between two "real" dvd
sub ids there is one with "none", in which case MPlayer's counting would
not fit anymore.

> > A strong indication for this interpretation is also the fact that WinDVD
> > allows you to select subtitles that are not marked as present - meaning
> > that WinDVD completely ignores this flag, whereas in MPlayer it even
> > changes the subtitle numbering.
> 
> There are no interpretations, just one DVD spec. Please try out a DVD editing 
> program if you need to clear up things for yourself.

A DVD editing program isn't the spec either. And that spec itself unfortunately
is not available to me. So I don't have that much choice except guessing...

> If you want mplayer to be able to select all the spu streams in the MPEG-PS, 
> it's possible, but it needs a rethink/rewrite of the subtitle handling logic.

No, being able to select at least most of them (an more than with you
patch) does not - displaying the language in the OSD for all of them
does though.

> If you disagree with this statement, I really don't want to argue, I just 
> wanted to help with my patch. I tried out quite a few possible variations 
> until I arrived at what I got, which I think is the most simple working 
> solution.

And I am sorry that things got stuck in a (at least in parts useless)
discussion. I just don't want to apply a patch I don't fully understand
and fear will further entangle a broken architecture.

> Without wanting to offend you, suggesting unworkable fixes with 
> argumentation using  "probably", "might", "AFAICT" is not very professional 
> after this.

Well, as any developer with a clue (if existent) is not part of the
discussion as usual, what else would you expect? I wouldn't try to bite
anyone applying any of the proposed patches, but nobody has stated such
an intention...
But I agree, it's a PITA without having an actual patch to work with, which
is why I'll provide one although it needs further work.
It's almost the same as the first.
In the following with "variants" I mean 4:3, wide, letterbox and
pan&scan mpeg subtitles constituting one dvd sub.
Attached patch currently is supposed to make it possible to select at least
one variant of each "dvd subtitle" (e.g. 4:3 or letterbox), and in some
cases even all variants.
Displaying the language is supposed to work correctly for exactly one
variant of the DVD subtitles.

Now for why I currently prefer this approach, although the final solution
will be a bigger change:
1) MPlayer is more flexible than a DVD player, so IMO it would be better
if a final solution allowed selecting all variants
2) DVD subtitles are currently stored under SUB_SOURCE_DEMUX in
mplayer.c, so it seems preferable to actually use the demuxer IDs for
consistency
3) the -sid will be the same after -dumpstream (though my recently
proposed patch might be needed to make subtitles work with dumpstreamed
at all)
4) Not more dependencies on stream internals in mplayer.c

Things speaking against it:
1) Bigger change, esp for making language display work better
2) if mpeg stream ids are not consecutive we might end up with 31
"empty" subtitles before the real one in subtitle switching, making
it a real pain

A different option might be to add a SUB_SOURCE_STREAM with a different
keybind specially for switching dvd subtitles.
Does not sound too nice either though.

Greetings,
Reimar Döffinger
-------------- next part --------------
Index: stream/stream_dvd.c
===================================================================
--- stream/stream_dvd.c	(revision 22222)
+++ stream/stream_dvd.c	(working copy)
@@ -262,20 +262,26 @@
 }
 
 int dvd_number_of_subs(stream_t *stream) {
+  int i;
+  int maxid = -1;
   dvd_priv_t *d;
   if (!stream) return -1;
   d = stream->priv;
   if (!d) return -1;
-  return d->nr_of_subtitles;
+  for (i = 0; i < d->nr_of_subtitles; i++)
+    if (d->subtitles[i].id > maxid) maxid = d->subtitles[i].id;
+  return maxid + 1;
 }
 
 int dvd_lang_from_sid(stream_t *stream, int id) {
+  int i;
   dvd_priv_t *d;
   if (!stream) return 0;
   d = stream->priv;
   if (!d) return 0;
-  if (id >= d->nr_of_subtitles) return 0;
-  return d->subtitles[id].language;
+  for (i = 0; i < d->nr_of_subtitles; i++)
+    if (d->subtitles[i].id == id && d->subtitles[i].language) return d->subtitles[i].language;
+  return 0;
 }
 
 int dvd_sid_from_lang(stream_t *stream, unsigned char* lang) {
@@ -286,7 +292,7 @@
     for(i=0;i<d->nr_of_subtitles;i++) {
       if(d->subtitles[i].language==code) {
         mp_msg(MSGT_OPEN,MSGL_INFO,MSGTR_DVDsubtitleChannel, i, lang[0],lang[1]);
-        return i;
+        return d->subtitles[i].id;
       }
     }
     lang+=2; 
@@ -1066,10 +1072,10 @@
           sub_stream->id = pgc->subp_control[i] >> 8 & 31;
 #endif
 
-        mp_msg(MSGT_OPEN,MSGL_STATUS,MSGTR_DVDsubtitleLanguage, d->nr_of_subtitles, tmp);
-        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_SUBTITLE_ID=%d\n", d->nr_of_subtitles);
+        mp_msg(MSGT_OPEN,MSGL_STATUS,MSGTR_DVDsubtitleLanguage, sub_stream->id, tmp);
+        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_SUBTITLE_ID=%d\n", sub_stream->id);
         if(language && tmp[0])
-          mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_SID_%d_LANG=%s\n", d->nr_of_subtitles, tmp);
+          mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_SID_%d_LANG=%s\n", sub_stream->id, tmp);
         d->nr_of_subtitles++;
       }
       mp_msg(MSGT_OPEN,MSGL_STATUS,MSGTR_DVDnumSubtitles,d->nr_of_subtitles);