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

[Aurelien Jacobs]

Uoti Urpala wrote:

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

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.
I simply have never seen a use case for this.

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

It's not the reason why this field was created.

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

It has technical problems !
It can't be muxed in other container than Matroska.

> Nor do I see a reason for other containers to use a format different
> from Matroska

Then you missed the origin of this change:
https://roundup.mplayerhq.hu/roundup/ffmpeg/issue588
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.
Hopefully standard ASS events line contains this duration and can
thus be muxed in containers which don't have a duration field.

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

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.

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

As explained above, it's technically impossible.

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

But libass *already* support another format. Namely the standard ass
format. And I indeed think there isn't much reason for libass to support
any other format than the standard one.

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

OK. Don't call it fixing, call it removing a partial and broken
workaround.

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

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.

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

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

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 ?

Aurel