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

[Uoti Urpala]

On Fri, 2006-12-22 at 18:19 +0100, Reimar Döffinger wrote:
> On Fri, Dec 22, 2006 at 06:55:55PM +0200, Uoti Urpala wrote:
> > 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.

What I suggested was combining it with find_sub.c/subreader.c, which
doesn't seem any more "complex" a location than sub.c. Those files
already have code to set vo_sub based on external subtitles.

> > That wouldn't work. You're leaving both the libass and non-libass

> Huh? Moving the line means that when ass_enabled is set then at the

I should have deleted that part of the mail as it doesn't matter as long
as the patch still leaves demux_mkv directly changing the subtitle
state.

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

This is still true though.

> Or giving these two function in sub.c an additional subtitle * argument,
> doing that seemed a bit obfuscating to me at first though.

Not sure what you mean by this, an extra argument for what?

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

Recognizing subtitle streams and assuming the caller must want different
semantics for such a stream is what I called ugly. On the caller side
the reason is not "because I recognize this as a subtitle stream" but
"here I do not actually want the next packet in this stream".

> the current fill_buffer semantics are broken, esp. that once such a
> buffer has gone over-full things are just completely broken.

The error recovery might not be good, but I think usually that's
something that "should not happen" and the real problems are elsewhere
in the code doing reads in such a way that the buffers fill up, or when
trying to work around broken files the buffers should just be bigger.
Also the recovery problems do not seem to have anything to do with the
per-demuxer fill_buffer() semantics but only higher-level functions.

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

There's no reason why it would exclude lavf subtitles. Lavf can be
treated like any linearly read format.

I suppose the "new API" would be a call to read next packet in demux
stream DS if any exist with timestamp less than X. Simplest
implementation would be to add a demuxer field for the function,
implement it for demux_mov, and add a function calling it in demuxer.c
(doing nothing for streams other than mov unless it's implemented for
proper sub delay support in those).

I suppose your suggestion is to change demux_mkv.c (at least, maybe
ogg?) to ignore fill_buffer() calls if the stream looks like a subtitle
stream and then use fill_buffer() to always try reading the next sub
packet, which would support sub_delay in seeking formats but OTOH cause
unnecessary seeks possibly far away even if sub_delay is not used.

An alternative implementation without adding to the API (yet?) or
changing the semantics of fill_buffer would be to make demux_mov create
packets for subtitles up to the current video position. It already has
the code to recognize such subtitles for in-demuxer sub parsing, so the
implementation would only require writing a packet where it now does sub
parsing.