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

[Reimar Döffinger]

Hello,
On Fri, Dec 22, 2006 at 06:55:55PM +0200, Uoti Urpala wrote:
> 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).

Well, most of it is splitting it into lines. One place seems as bad as
the other to me, and currently splitting it seems overcomplicating
things for now. If you expect a complete cleanup/rewrite in one patch
from me, there is no chance you'll get that.

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

Huh? Moving the line means that when ass_enabled is set then at the
start of the function vo_sub->lines will be 0 and afterwards it will be
0, too. So no subtitle displayed. Of course, setting vo_sub = NULL in
addition would be possible, too.
Or giving these two function in sub.c an additional subtitle * argument,
doing that seemed a bit obfuscating to me at first though.

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

Well, I don't find that so much different from "using a different API if
the stream is recognized as a subtitle one". Also I always thought that
the current fill_buffer semantics are broken, esp. that once such a
buffer has gone over-full things are just completely broken.

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

True, though given how demux_mov currently behaves over e.g. http I have some doubts
it can be made any worse...
Anyway
1) is there noone else who cares? I don't think there is much chance of
getting a decision if only we discuss
2) I don't think I'll write such a new API (and if we don't want to
exclude lavf subtitles forever it would need to be added to that, too),
so the questions is: will someone else?
3) Can we at least agree on moving the vo_sub setting code to sub.c so
it at least isn't duplicated? It can still be moved to a more
appropriate place after we decided where and create all the stuff
necessary, i.e. not much after hell froze over...

Greetings,
Reimar Döffinger