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

[Clément Bœsch]

On Mon, Jan 17, 2011 at 09:04:43PM +0100, Reimar Döffinger wrote:
> On Tue, Jan 11, 2011 at 01:38:26AM +0100, Clément Bœsch wrote:
> > 1) Since subreader.c tends to "load" instead of "read" subtitles files,
> >    is it ok to rename it to something like subloader.c? I'm not only
> >    referring to my load_* functions, but even sub_read_* functions are not
> >    really reading, but much more loading subtitles using buffers…
> 
> I don't see the point/difference. The do read subtitles (stream_read_line etc.),
> but of course they also load it. One without the other makes little sense.
> 

I recently moved a few functions which are "loading oriented" (they track
and load subtitles, without reading them), so the main file tend to load
subtitles more than reading them (from an external point of view). Not
that important though.

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

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

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

-- 
Clément B.