[MPlayer-dev-eng] [PATCHES] VOBsub cleanup

[Reimar Döffinger]

On Mon, Jan 17, 2011 at 11:19:13PM +0100, Clément Bœsch wrote:
> On Mon, Jan 17, 2011 at 09:04:43PM +0100, Reimar Döffinger wrote:
> > > 2) load_vob_subtitle, just like the other hack, seems at the moment unable
> > >    to load more than one subtitle at a time; can anyone confirm that? It
> > >    seems not very problematic since there is generally a single vobsub
> > >    file for multiple language but just to know :)
> > 
> > In theory you should be able to load one subtitle per vo_vobsub struct.
> > However having more than one probably isn't tested at all, and changing
> > vo_vobsubs during playback might be real tricky.
> > However, it means it would be a good idea if load_vob_subtitle returned
> > the allocated vobsub struct instead of assigning it to vo_vobsub itself.
> > 
> 
> Why? Doesn't this mean in case we want to load more subtitles we would
> have to change the prototype? In the current situation (after the patch),
> we would simply have to change the assignation to an item in an
> array/linked list instead of vo_vobsub; just like the way load_subtitle is
> designed.
> 
> I'm not aware at all of the vobsub system, so I certainly miss your point
> here.

Well, loading != selecting the subtitle.
Assigning vo_vobsub means it gets selected for display, that's why assigning
it inside that functions seems wrong.
Your point is likely right as well though.

> > > 3) If this patch gets applied, I'll edit a bit my -sub-paths patch to
> > >    allow vobsub to be loaded in specified directories, but it will only
> > >    load the first find one (if you confirm (2)); it is fine?
> > 
> > It is fine (though as said I'd appreciate it if you try to ensure you make
> > doing this properly not harder, i.e. keep assinging vo_vobsub as much top-level
> > as reasonably possible).
> 
> Remember that load_vob_subtitle would have the task to load multiple
> subtitles (it will need rename to load_vob_subtitles btw), so what's the
> point of returning one? I mean, the "core" shouldn't be aware of what was
> loaded, just know that 0 or more vobsub are eventually loaded. And if so,
> it will just look into its vo_vobsub list. No?

Well, I guess in that case it might make sense to actually have a function
that loads a singe subtitle and returns the allocated memory and another function
that loads them all.
Note that this is what we do for normal subtitles, there is the "add_subtitle" function
that calls "sub_read_file", and to do it properly we should have that for vobsub as well
(and a slave command that allows loading them at run-time).

> > > 4) VOBsub has a not-working-that-much RAR support; is it fine to drop it
> > >    in case we move to FFmpeg decoders?
> > 
> > Dropping features is not appreciated much.
> > Also last I checked quite a few people seemed to be using it, so it shouldn't
> > be working that badly...
> 
> I will look into it deeply if I have time. I admit I haven't tested it for
> a while; I'm just remembering a few cases where subtitles were simply not
> loaded (only the .idx or only the .sub in the .rar I don't remember
> exactly).

That's likely not supported, but it seems like an unusual case to me...