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

[Uoti Urpala]

On Sun, 2008-09-07 at 01:23 +0200, Aurelien Jacobs wrote:
> Uoti Urpala wrote:
> > On Sat, 2008-09-06 at 00:55 +0200, Aurelien Jacobs wrote:
> > > event line, which means the ReadOrder don't get out of the
> > > matroska container.
> > 
> > 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?

Also I think the way MPlayer uses the field is a valid use.

>  But this is never useful for decoding or re-muxing, and honestly,

MPlayer uses it during playback, and if you mux to a new Matroska file
you should keep the information.

> BTW, if you really think this information should get out of the
> demuxer, could you propose a way to do it ?
> I don't think that putting it in a separate field in AVPacket is an
> acceptable idea, because this is a matroska specific field.

One possibility is to revert your changes to the packet format and go
back to the one Matroska natively uses. IMO there is no reason to think
your "each packet stored exactly like a line in monolithic .ass files"
is any more valid a format than the one used by Matroska. That format is
the one used for practically all embedded ASS subs, and AFAIK it does
not have any technical problems that your version would fix. You seem to
have just decided that you must change the way ASS is stored when muxed
as a packet stream, with little practical benefit or justification for
your change. You could supply a separate function to convert to your
desired format if you want.

I remember two arguments you've used to justify your format change: that
the Matroska version is "not real ASS" and that other container formats
wouldn't use the same one. I see no reason why each packet would have to
exactly match one line in an .ass file. Nor do I see a reason for other
containers to use a format different from Matroska (has any container
actually picked such a different format?); and even if they did, with
practically all existing files being in Matroska format wanting to use
another as the default seems weird.

> 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"? IMO the closest to a "standard" for
packetized ASS stored in multimedia containers is whatever Matroska
does.

> Anyway, even if the matroska demuxer would output ReadOrder, this
> wouldn't change the fact that libass must be able to decode ASS
> packets coming from a demuxer, without using this ReadOrder
> information. Because other containers (nut, etc...) just won't
> store ReadOrder when muxing standard ASS.

Why wouldn't they? Is there any practical reason to choose a format
different from the one Matroska uses? And I think there isn't much
reason for libass to support any other format as long as files using
such formats do not exist in practice.

> > > Anyway, this is quite easy to solve. Currently, libass stores all
> > > the events line it receives, forever. When the events are received
> > > out of demuxed packets, there's no reason to keep them in memory
> > > forever.
> > 
> > There is, they can be needed after seeking. Keeping them in memory
> > ensures that you see all subtitles if you seek within an already-played
> > file part.
> 
> 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"...

> > >  The demuxer will send them again as soon as they are needed.
> > 
> > No it won't if the packet occurred before the seeked-to position.
> 
> Sure, but libass buffering won't change this when seeking to a
> not-already-seen part, and it also won't change this when not using
> libass to display subs.

It doesn't make everything perfect, but it's better than your proposal
of always doing the wrong thing.

> If you think getting the sub which should be displayed just after
> a seek is really important, then the only way to fix it is to
> ensure the demuxer output the relevant subtitle packets after
> a seek (patch welcome). No kind of buffering can fix this.

Buffering cannot give a 100% perfect solution, but it can give an
improvement.

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

> My patch also fixes this.

It's the _only_ thing that could be called a "fix", and it fixes a
non-issue.

> > > So the simple attached patch will just free every events as soon
> > > as they are not displayed anymore (if they come from a demuxed packet).
> > > Is this OK ?
> > 
> > No.
> 
> Hopefully, you're not libass maintainer.

Not libass, but I can decide the MPlayer behavior.