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

[Uoti Urpala]

On Tue, 2008-09-09 at 16:32 +0200, Aurelien Jacobs wrote:
> Here we have 2 different formats. One which can contain the subtitle display
> duration through the mean of start time and end time (aka .ass format) and
> one which can't store this information (aka matroska's ass format).

Actually we have only one existing muxed format: the Matroska one. What
you called the ".ass" format and wanted to use internally in demuxer
packets is only used in complete .ass files that are read as a unit. It
is a bad choice as a demuxer packet format because it contains the start
and end time as absolute timestamps (and you even tried to use it so
that the timestamp inside the codec data overrode the demuxer packet
pts).

> Matroska is the only container which can store display duration by itself.
> So matroska's ass format can't be used in any other container. So it just
> can't be used as a kind of universal format.

Matroska's format is used in at least one type of file. Your proposed
format is used in zero types of files. Any "universality" is based on
the assumption that other containers would want to standardize on your
version. But given its problems it would be a bad choice. And even if
some container used that format it would still be preferable for MPlayer
to keep its internal packets in a format that has the timing information
in the demuxer packet fields instead of inside the codec data.

> The only format left is standard .ass format (unless you can show me another
> more popular format which can store display duration by itself).

As explained above your ".ass format" is only a popular format for
complete files, not for subtitle packets.

> I don't say that standard .ass is perfect or anything, but that's the least
> worst format we have which contains display duration, and no one came up
> with anything better up to now.

An obvious less-broken alternative for muxing into containers which
cannot keep duration at the container level is to modify the Matroska
format to add duration inside the packet. So instead of your absolute
start time and absolute stop time there would be only duration relative
to the start time, and start time would be specified by the
container-level pts. That would at least allow remuxing to a different
pts without breakage. But even with such a container-level format it
would still be preferable to keep MPlayer's internal packets in their
current format.

> > Why don't you keep the ReadOrder field in demux packets and strip it in .ass 
> > muxer?
> 
> That's not only .ass muxer. It's about all container muxers... And even if
> you strip ReadOrder in all the muxers, when you try demuxing the result
> you don't have any ReadOrder that you can use in libass.

A .ass muxer would use the ReadOrder field to order the lines in its
output so the information would be kept. You seem to assume that other
containers would not have the ReadOrder information; but do you have any
reason for such an assumption except that you proposed a format without
it yourself? Is there any container that actually specifies an ASS
storage format without ReadOrder? For all I know every format that adds
ASS support could have ReadOrder.

> > It indeed improves user experience. For instance, if I did not catch a phrase, 
> > I scroll 10 seconds back, and there is a rather high probability that I will 
> > get to a point after the subtitle packet but still in the subtitle time frame.
> 
> I absolutely agree about this. This is a desirable feature to have.
> But buffering is not the appropriate solution for this. That's the demuxer's
> job to provide the relevant subtitle packets after a seek.

Implementing that at the demuxer level is more work. Not that I'm
against you implementing it in the lavf demuxer, but it's still useful
to have it in libass too.

> So I've just improved the demuxer, which now achieve this. Unfortunately,
> it only works for indexed subtitle packets, and most matroska files don't
> contain subtitle index. So this will generally only works for the parts
> of file which were already played (and thus indexed). Note that it is not
> worse than your buffering solution.
> 
> So have a look at attached patch. It's a bit different from the previous
> one. It flush the libass buffer after a seek (to be sure that no subtitles
> are left, even if we seek to a very close position where the current
> subtitle still have to be displayed).

You're still using the packet format that keeps all timestamp
information inside the codec data. That's wrong; so at least the
mpcommon.c changes shouldn't be there.

With that fixed, the patch would still break buffering for the native
demuxer. Even if the lavf demuxer is fixed enough to be made the default
again you still shouldn't deliberately cause regressions for the
internal one. There have been quite a few regressions in the lavf
demuxer, and I doubt every last one of them is known yet. The internal
demuxer should be kept working at least at its current level for some
time before it would be safe to conclude that it won't be necessary any
more.

I'm also not convinced that the flush functionality is needed at this
time. At the moment all known SSA/ASS sources DO have the ReadOrder
field. It is only because of the deficiency of the lavf demuxer that
MPlayer can't access it. And you should fix that anyway for at least
Matroska->Matroska muxing (and some other usage too). Once MPlayer has
access to it the flush functionality doesn't have much benefit. That is
not a strong objection adding it, but I think it would be simpler to fix
the current issues with the lavf demuxer by adding support for the
ReadOrder field (which will be needed anyway) and considering more
complex things like this patch later.

> It avoids any possible duplicate subtitle, but when you seek back to a
> previously played position you will still get instantaneously the relevant
> subtitle. So there is no regression AFAICT.
> Note that you can also verify this by not using libass. The basic, text
> only, subtitle renderer has no kind of buffering and no duplicate detection,
> but it still display subtitles correctly after a back seek.

This is certainly a positive feature which adds useful functionality.
However if I understand correctly no MPlayer changes are required for
this (so it's not relevant to the patch).