[MPlayer-dev-eng] [PATCH] libass: fix parsing of tracks extracted from containers

[Uoti Urpala]

On Sun, 2008-09-07 at 15:33 +0200, Aurelien Jacobs wrote:
> Uoti Urpala wrote:
> > > > But it SHOULD get out of the container. That is information stored in
> > > > the file, and the application using the demuxer should be able to access
> > > > it. If it can't that is a deficiency in the demuxer.
> > > 
> > > Why ???
> > > There are many more information read by the demuxer that are never
> > > exported to the outside world. Because many information are dedicated
> > > to the demuxer itself, so that it can properly extract or relate other
> > > data.
> > > And ReadOrder is exactly one of those information dedicated to the
> > > demuxer itself. Its goal is to allow the demuxer to re-order sub
> > > packets in their original order. This is only useful for specific
> > > demuxers aiming at extracting a bit-exact copy of the original ass
> > > file.
> > 
> > Why do you think the *demuxer* would have to be a specific one created
> > for that task? Why can an application with that goal not be using the
> > lavf demuxer? Any reason except the lavf demuxer currently being too
> > limited for such tasks?
> 
> Well, if you think it can be useful to anyone, it could be implemented
> in the lavf demuxer. It would simply have to ensure that the packets
> it output are properly ordered.

Are you talking about changing the order of output packets instead of
giving the value of the ReadOrder field? I don't think that would work.

> I simply have never seen a use case for this.

I have compared .ass files extracted from an .mkv file with versions
that didn't go through muxing.

And even if you thought lavf users wouldn't need it, dropping the
information when you remux mkv->mkv is bad unless you can be sure nobody
anywhere has a use for it (which I think they do).

> Most containers don't have a duration field to specify the display
> duration of each subtitle packets. This duration have to be part
> of the packet data itself so that it can be muxed in any container.

Those containers do not have subtitle support as good as Matroska. I
think your proposal to move all timing information to the codec level to
avoid the container limitations is problematic. It is the opposite of
what is done with audio and video. Audio has an implicit duration based
on sample rate and video has an implicit duration until it is
overwritten by the next frame, so starting time gives full timing
information, and this information is handled on the container level
(even if the video codec has its own timestamp information that is
normally ignored and the container-level timing information is used
instead). Subtitles have no such implicit duration information so it has
to be given explicitly instead. Some containers can not represent this
natively; but IMO this is not a good reason to say it does not belong in
the demuxing layer. I think the timing information should be directly
available at the demuxing/muxing level when possible.

IMO a better solution would be to keep timing information in
demuxer-level fields and out of codec data. Conversion functions can be
provided for the case where you need to push the timing information to
the codec level because the container has no better place for it.

> Hopefully standard ASS events line contains this duration and can
> thus be muxed in containers which don't have a duration field.

Even if you use a format that stores the timing information at the codec
level that does not prevent you from keeping that information AND the
ReadOrder information. I think having the format exactly match lines in
monolithic .ass files does not have high value.

> > > Another possibility would be to write this in the AVPacket.data
> > > itself, thus producing a new kind of bitstream which is *not* ASS,
> > > so it should get a new codec name, a new CODEC_ID, and be decoded
> > > by an appropriate decoder (ie. not a standard ASS decoder). Sounds
> > > like overkill for no functional improvement.
> > > Any other idea ?
> > 
> > What is a "standard ASS decoder"?
> 
> One which follow the closest thing we have to a spec:
> http://moodub.free.fr/video/ass-specs.doc

That describes the syntax of monolithic .ass files which are meant to be
read as a unit. It doesn't tell how to convert them to muxable packets.

> Note this point from the general information:
> "4. SSA does not care what order events are entered in"
> This make the ReadOrder field practically useless.

It allows keeping track of the original order (useful exactly because
that order need not match the time order which is otherwise available),
and it also allows identifying a particular subtitle line.

> > > Which is a very inconsistent behavior. Because it won't work the same
> > > depending on whether you seek to an already played part or not.
> > > My patch fix this inconsistency.
> > 
> > Bullshit. Your patch does not "fix" anything. You break something that
> > worked before. That not everything works perfectly is not an excuse to
> > call breaking things "fixing", even if the state of nothing working
> > right is more "consistent"...
> 
> OK. Don't call it fixing, call it removing a partial and broken
> workaround.

Partial maybe, but working and not "broken". Your patch would concretely
worsen the functionality of MPlayer.

> Then you got me wrong. My proposal is to always do the right thing,
> instead of having a ugly workaround working only in some specific
> situations. This patch is a first step in the right direction.
> It would then be nice to have the demuxer fixed to send appropriate
> sub packets after seeking.

It's no step towards a a full solution, it only breaks what already
works.

> > > Moreover, if you think that some buffering could have any use,
> > > it should be done outside of libass, so that it benefit every
> > > subtitle decoders, and more importantly, it shouldn't have unbounded
> > > growing memory usage.
> > > Yes, right now, libass uses unbounded memory size. If libass is used
> > > to render a continuous network stream doing heavy use of subtitles
> > > and running for months, it would certainly exhaust memory...
> > 
> > This is not an issue in practice.
> 
> I wouldn't bet on this...
> FFmpeg had a similar issue. It adds every keyframe it encounter to an
> index. This has proved to exhaust memory in practical situations.
> Now FFmpeg has a function to reduce the index size when it's
> growing too big.
> Something similar would need to be added to libass if you insist on
> keeping buffering.

Even if something similar would be needed at some future point it could
be implemented by dropping oldest subtitles instead of all.

> > > My patch also fixes this.
> > 
> > It's the _only_ thing that could be called a "fix", and it fixes a
> > non-issue.
> 
> Hum... It also fixes the issue of subtitles appearing multiple
> times... ;-)

I prefer fixing that by going back to the internal demuxer if lavf
doesn't export the ReadOrder information. Besides issues related to
MPlayer there are at least two arguments why lavf should do that. Lavf
should export the data to allow mkv->mkv remuxing without losing it. And
your change of moving timing information from the demuxing layer to the
codec layer was bad from an architectural viewpoint. It would be
preferable to keep the codec level data in the Matroska format, store
timing information at the demuxer level, and provide conversion
functions for containers that need to move the timing information to the
codec level.

> Anyway, if you insist on keeping this ugly workaround (namely,
> buffering), then the obvious way to fix subtitles duplication is
> to check that new subtitle lines are not already in the buffer
> by simply comparing them.
> Do you want me to implement this ?

How would you implement it? It is valid to have multiple distinct
subtitle lines with the same timing and contents, so you cannot tell
whether a line is already in the buffer by simply checking whether one
of the existing lines is identical.