[MPlayer-dev-eng] [RFC] subtitle cleanup

[Uoti Urpala]

On Fri, 2006-12-22 at 10:17 +0100, Reimar Doeffinger wrote:
> > I don't particularly like using libvo/sub.c for this. It has nothing to
> > do with vo. Combining it with find_sub.c/subreader.c would be a better
> > match IMO.
> 
> Since it is all about setting vo_sub it has all to do with vo.

vo_sub is the eventual output mechanism but I think this code is more
about decoding the subtitles (codec level rather than vo level).

> > This looks wrong, now if libass rendering for non-SSA subtitles is
> > enabled they'd be rendered twice. Either the libass rendering case

> Actually I think the clear_subtitle from above should just be moved into
> the ass_enabled (even without this patch I think this would make more
> sense, at least if it works, I did not test)

That wouldn't work. You're leaving both the libass and non-libass
rendering enabled in the patch; when libass rendering is wanted the
non-libass version must be disabled, and moving the line wouldn't do
that. I still think the best way to do that would be to move the "use
libass for non-SSA rendering" logic to the common subtitle handler.

> 
> > +    char type = d_dvdsub->sh ? ((sh_sub_t *)d_dvdsub->sh)->type : 'v';
> > 
> > The variable should probably be renamed from d_dvdsub to something else
> > if it's not specific to dvd subs any more.
> 
> Well, neither is dvdsub_id since a long, long time...

Yes it should be renamed too.

> > +        double nextpts = ds_get_next_pts(d_dvdsub);
> > 
> > As I explained on IRC using current ds_get_next_pts for this is wrong.
> 
> And I still think that fixing the demuxers to handle the problems with
> this is better than implementing yet another API that AFAICS will add a
> lot in terms complexity but little in new functionality.

You didn't include any proposed "fix" in the patch and it is certainly
broken with the current code. Using different semantics in fill_buffer
if the stream is recognized as a subtitle one is an ugly hack IMO. It at
least interacts badly with the MPlayer cache, causing a useless seek
forward in demuxers that do seek to read a subtitle that isn't actually
needed yet.

> > Also as is the patch would cause a regression as it converts demux_mkv.c
> > to use the new code even though it doesn't yet implement multiple
> > simultaneous subtitles.
> 
> This patch does support that, if not it's a bug. It does not yet support
> duration when passing the subtitle messages via demuxer packets, but

True, I had implicitly assumed that it would fix the timing but it only
uses the parsing code in demux_mkv.