Support Matroska CueDuration and CueRelativePosition for better subtitle handling on seeks

[GoogleCodeExporter]

See Matroska specs for the details on the elements.

The basic idea is to "cherry-pick" subtitle frames that are still valid at the 
target seek point out of the file, so that you immediately get subs after the 
seek.

A few samples are archived locally.

Original issue reported on code.google.com by h.lepp...@gmail.com on 12 Dec 2012 at 7:27

[GoogleCodeExporter]

Some notes on the samples:

Chapter Seek = Seek to chapter & subtitle line will be missing.
Karaoke Seek = Seek anywhere & subtitles lines from that frame will be missing.

assColorSpace: Subs start at 0.

Original comment by h.lepp...@gmail.com on 12 Dec 2012 at 7:28

[GoogleCodeExporter]

The samples I originally provided turned out to be invalid because of:

https://trac.bunkus.org/ticket/903

When MKVToolNix 6.4 is released, extract(?) and remux any such samples to fix.

Original comment by cyber.sp...@gmail.com on 24 Jul 2013 at 12:24

[GoogleCodeExporter]

I've attached a pair of patches which add this feature. This should work with 
files produced by MKVToolnix version >=5.9.0. It even works around a small bug 
that will be fixed in the next version of MKVToolnix.

I've also attached a test build of lav with this feature that people can try 
out if they're interested.

In terms of what each patch is for, the first patch cleans up the code in the 
mkv_seek function a little while trying to keep the behavior the same. The 
second patch adds the feature itself.

Original comment by johnp...@gmail.com on 26 May 2014 at 4:57

Attachments:

• 0001-libavformat-clean-up-the-seeking-function-mkv_seek-i.patch
• 0002-libavformat-add-support-for-displaying-pre-exisintg-.patch
• lavfilters.zip

[GoogleCodeExporter]

Wow, it works, what an awesome feature!

Original comment by wOxxOm on 26 May 2014 at 5:40

[GoogleCodeExporter]

I'm seeing missing lines when seeking in the middle of animation effects, like 
the attached sample. You can easily see the difference if you compare against 
seeking with the external script.

Original comment by cyber.sp...@gmail.com on 26 May 2014 at 6:13

Attachments:

• Karaoke_CueDuration.7z

[GoogleCodeExporter]

^ Probably because those lines span more than one Cue back.

Original comment by wOxxOm on 26 May 2014 at 6:18

[GoogleCodeExporter]

If implemented properly, that shouldn't matter though.

Original comment by cyber.sp...@gmail.com on 26 May 2014 at 7:36

[GoogleCodeExporter]

I should probably explain the bug that will be fixed in the next mkvmerge 
version. Basically, if two subtitles or more have the same start time, their 
CueDurations/CueRelativePositions will (usually) both be set to the 
duration/position of the last subtitle with that start time.

My patch works around the messed up position values, but there isn't a good way 
I'm aware of to work around the messed up duration values. In practice, many 
files playback fine even though I don't work around the duration part of the 
problem, because subtitles with the same start time often have the same 
duration.

The file you provided causes mkvmerge (versions <= 6.9.1) to write incorrect 
duration values, because there are subtitles with the same start times and 
different end times. If you remux it with a version of mkvmerge built from the 
master branch on github, the issues should go away.

Obviously, it would be great if we could perfectly play back files produced by 
older versions of mkvmerge too. Unfortunately, I don't think this is possible 
in any kind of efficient manner. However, I'm open to suggestions for how to do 
better when durations are incorrectly written by these mkvmerge versions.

Original comment by johnp...@gmail.com on 27 May 2014 at 2:23

[GoogleCodeExporter]

> If you remux it with a version of mkvmerge built from the master branch on 
github, 
> the issues should go away.

Attached file has now been remuxed with 
mkvtoolnix-amd64-6.9.1-build20140527-608-58667e5 from 
http://www.bunkus.org/videotools/mkvtoolnix/win32/pre/ and does indeed function 
correctly now with your LAV Filters patches.

> Basically, if two subtitles or more have the same start time,
> their CueDurations/CueRelativePositions will (usually) both be set 
> to the duration/position of the last subtitle with that start time.

Hmm... To workaround this, wouldn't you only need to load all subtitles within 
Cluster rather than Block which CueRelativePosition points to?

Original comment by cyber.sp...@gmail.com on 27 May 2014 at 4:36

Attachments:

• KaraokeCueDuration(mkvtoolnix_6.9.2pre).7z

[GoogleCodeExporter]

> wouldn't you only need to load all subtitles within Cluster rather than Block 
which CueRelativePosition points to?

Currently, the patch does the following with files muxed with mkvmerge <=6.9.1:
(1) Scan through the cues and identify ones which overlap with the given 
timecode using CueTime and CueDuration.
(2) For each cluster that contains a subtitle identified in (1), scan through 
the cluster and load all subtitles in the cluster that overlap with the given 
timecode.

The fact that it scans through entire clusters means that incorrect 
CueRelativePosition values don't cause problems. The issue is that in step (1), 
the CueDuration values we rely on may be incorrect. If the CueDuration values 
are incorrect, we can't tell the difference between subtitles that don't 
overlap the timecode and subtitles which have wrong CueDuration values.

Here's a concrete example of where there would be issues. Suppose we have two 
subtitles with start times of 1 second. The first subtitle has a duration of 10 
minutes, but the second subtitle has a duration of 1 second. In this case, the 
CueDuration of both subtitles will be (incorrectly) written as 1 second. So, if 
we try to seek to 9 minutes, we won't know that the first subtitle should be 
displayed because the cue data says it shouldn't be.

Original comment by johnp...@gmail.com on 27 May 2014 at 11:15

[GoogleCodeExporter]

Yeah, I don't think you could fix that without some kind of insane bruteforce 
preroll method. Removing the version check and workaround for old mkvmerge 
entirely may be the preferred choice here. From what I can see, your 
'compliant' implementation still has beneficial behavior with mkvmerge 5.9.0 to 
6.9.1. Anyone who cares about seeking quality with subtitles containing complex 
animation effects could just remux their files with mkvmerge 6.9.2+.

Not special casing broken mkvmerge, would likely also increase the chance that 
Nevcairiel would commit your patches without hassle.

Original comment by cyber.sp...@gmail.com on 28 May 2014 at 1:19

[GoogleCodeExporter]

I'd be fine with removing the code that special cases mkvmerge if that's what 
the lav maintainers prefer. It would cut out a fair amount of code from the 
patch, which would be nice.

Original comment by johnp...@gmail.com on 28 May 2014 at 1:39

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

MKVToolNix 7.0.0 containing your fix was released a couple days ago.

Nevcairiel has requested you create a GitHub pull request, in order to make 
reviewing your changes easier. He also seemed to agree with the mindset that 
having a partial hack for mkvmerge <=6.9.1 was bad, so you may as well go ahead 
and simplify it down to only the fully complaint implementation.

http://forum.doom9.org/showpost.php?p=1683501&postcount=17642

Original comment by cyber.sp...@gmail.com on 10 Jun 2014 at 10:12

[GoogleCodeExporter]

In general, keep the patch as simple as possible.
I'm not sure if your first seek refactoring patch is mandatory for this 
feature, as it makes reviewing harder as well.

Anyhow, I'm out of the country for a couple weeks still, and probably won't 
have all that much time until i'm back in July.

Original comment by h.lepp...@gmail.com on 10 Jun 2014 at 10:14

[GoogleCodeExporter]

The patched build posted above behaves worse for me than the official vanilla 
0.62.0 build when it comes to seeking in legacy mkv files (without CueDuration 
and CueRelativePosition).

I attached a legacy and a file with those elemets ("v2.mkv" and "v4.mkv" 
respectively). Seek to the first chapter to (not) see the "TEST" subtitle.

Original comment by sneaker...@googlemail.com on 19 Jun 2014 at 8:10

Attachments:

• v2.mkv
• v4.mkv

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I wonder if those issues with your legacy files are the result of applying the 
clean up seeking patch, can we test a build with just the second patch and see 
if that's a go? I tested the build and for the remuxed files it works 
beautifully.. 

Original comment by RytheSta...@gmail.com on 14 Jan 2015 at 7:06

[GoogleCodeExporter]

If there are issues seeking in legacy files, then it may very well be because 
of the "cleanup seeking" changes.

It seems that the consensus is that cleaning up the seeking code isn't really a 
priority. Eventually, I'll try to make a version of the patch which doesn't 
make any substantial changes to the existing seeking code. However, I'm going 
to be super busy for the foreseeable future, so it could be 6-12 months before 
that happens.

If someone wants to work on it in the interim, I'd be happy to offer advice.

Original comment by johnp...@gmail.com on 3 Feb 2015 at 9:42

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Thanks John for the patch. I'm very interested in this feature so I tried to 
simplify your patch without changing the mkv_seek function and without the 
cleanup changes.
Problem with legacy files was subs disappeared because the Queues were modified 
even when subPrequeues were empty, which happens when the matroska file didn't 
have CueDuration and CueRelativePosition elements.

Now with this patch it should work like this:

old file is made with mkvmerge <7.0.0 or without CueDuration and 
CueRelativePosition elements ---> same as always. displays subs only when 
seeking to chapter

file is made with mkvmerge >=7.0.0 and CueDuration and CueRelativePosition 
elements are present ---> displays pre-existing subtitles

There are no hacks, just making sure the file was made with mkvmerge version 
>=7.0.0 before executing the new code.

https://github.com/onomatopellan/FFmpeg/commit/54a6504

https://github.com/onomatopellan/FFmpeg/commit/54a6504.patch

I have attached a LAVFilters test build from latest MPC-HC with this patch 
applied.

Original comment by onomatop...@gmail.com on 8 Mar 2015 at 5:40

Attachments:

• LAVFilters.zip

[GoogleCodeExporter]

Cool.

One thing worth discussing is whether to use a blacklisting or whitelisting 
approach for displaying pre-existing subtitles. For example, the current 
development (and possibly release?) version of ffmpeg outputs mkv files with 
valid CueRelativePosition and CueDuration data. However, old versions may 
output incorrect data.

My thinking is that instead of doing the current whitelist approach, it makes 
more sense to just blacklist old versions of ffmpeg and mkvmerge.

However, if people think a whitelisting approach is better, we should make sure 
to add recent versions of ffmpeg (along with any other writing applications 
that output correct metadata) to that whitelist.

Original comment by johnp...@gmail.com on 8 Mar 2015 at 5:53

[GoogleCodeExporter]

I think you are right. Mkvtoolnix developer could change WritingApp's way of 
showing mkvmerge version in future builds. It's better to blacklist something 
that we already know than whitelisting something we can't know.

I'll put this back and make a new patch.

found = sscanf(mf->Seg.WritingApp, "mkvmerge v%2d.%2d.%2d", &a, &b, &c);
    if (found && a < 6 || (a == 6 && b < 9) || (a == 6 && b == 9 && c <= 1))

Original comment by onomatop...@gmail.com on 8 Mar 2015 at 7:15

[GoogleCodeExporter]

I don't want to do any kind of black or whitelisting. What is the worst thing 
that can happen if bad info is written? No subs are shown? Potentially wrong 
subs are shown? Some useless file reads are performed? I don't see the big deal.

Original comment by h.lepp...@gmail.com on 8 Mar 2015 at 7:27

[GoogleCodeExporter]

> I don't want to do any kind of black or whitelisting.

That's reasonable. If we don't do black/white-listing the worst thing that 
would happen is that during seeks, we might fail to display some of the 
pre-exiting subtitles that are supposed to be displayed at the time we seek to. 
That isn't so bad.

If we want to avoid black/white-listing while fixing the regression mentioned 
in #16, I suggest we check for the presence/absence of CueRelativePosition and 
CueDuration in the file and only try to show pre-existing subtitles if they are 
both present and nonempty for at least one Cue.

Original comment by johnp...@gmail.com on 8 Mar 2015 at 7:53

[GoogleCodeExporter]

The Cues are parsed anyway, and just iterating through a memory structure isn't 
expensive or slow. So I don't see how a straight-forward patch to just use the 
Cue elements when present would make it slower for files that dont have them 
present.

Original comment by h.lepp...@gmail.com on 8 Mar 2015 at 8:12

[GoogleCodeExporter]

This is the patch just adding the feature without caring about the file 
version. It always creates the subPreQueues and fill them if CueDuration and 
CueRelativePosition were present in the file. If they weren't then subPreQueues 
remains empty and nothing is erased from the Queues. (line 3426 in the code)
This fix problem with files like #16.

https://github.com/onomatopellan/FFmpeg/commit/5719931

https://github.com/onomatopellan/FFmpeg/commit/5719931.patch

While testing I saw really weird behaviors like lines appearing instead of 
signs or karaoke with repeated english lines when top line should be in 
japanese. (pic attached)
Of course this is "fixed" when the next subtitle line plays.

Original comment by onomatop...@gmail.com on 8 Mar 2015 at 8:25

Attachments:

• bug.png

[GoogleCodeExporter]

> I don't see how a straight-forward patch to just use the Cue elements when 
present would make it slower for files that dont have them present.

What performance issue are you referring to? If you're talking about comment 
#16, I thought the only issue that comment was talking about was that the patch 
didn't display certain pre-existing subtitles (when seeking) that were properly 
displayed without the patch.

Original comment by johnp...@gmail.com on 8 Mar 2015 at 8:27

[GoogleCodeExporter]

> While testing I saw really weird behaviors like lines appearing instead of 
signs or karaoke with repeated english lines

Is this on files that are muxed with new versions of mkvmerge/ffmpeg or old 
ones?

Original comment by johnp...@gmail.com on 8 Mar 2015 at 8:31

[GoogleCodeExporter]

Taking a closer look at the patch from comment #27, it looks like in the 
mkv_Seek() function, there isn't code for deallocating subPrequeues (and its 
individual elements) in the event we return before we deallocate it later on in 
the function.

More specifically, in any instances where we might return in mkv_Seek() after 
the call to GetSubtitlePreroll but before we deallocate subPrequeues later on 
in the function, we need to add code that deallocates subPrequeues it using 
QStructFree before the return so there isn't a memory leak. We also need to 
deallocate each of subPrequeues' individual elements with QFree before we 
deallocate subPrequeues itself.

Original comment by johnp...@gmail.com on 8 Mar 2015 at 8:36

[GoogleCodeExporter]

The one from the pic is from mkvmerge v6.0.0. I didn't see any problem with 
files from mkvmerge >=7.0.0.

Original comment by onomatop...@gmail.com on 8 Mar 2015 at 8:38

[GoogleCodeExporter]

Ooops, you are right. You had some "goto dealloc;" for that. I'd add it to the 
code.

Original comment by onomatop...@gmail.com on 8 Mar 2015 at 8:44

[GoogleCodeExporter]

The repeated english lines is expected behavior because of the (incorrect) way 
that mkvmerge 6.0.0 writes CueDuration and CueRelativePosition. The bug in that 
version of mkvmerge is that if two subtitles have the same start timecode, 
their cue entries will have the same BlockDuration and BlockRelativePosition. 
This means that if you have two different subtitles that are supposed to be 
displayed at the same time, you will instead see the same subtitle repeated 
twice when seeking.

Original comment by johnp...@gmail.com on 8 Mar 2015 at 8:46

[GoogleCodeExporter]

> Ooops, you are right. You had some "goto dealloc;" for that. I'd add it to 
the code.

FYI, I think the dealloc label I originally wrote still had a memory leak 
because it didn't deallocate the individual elements in subPrequeues using 
QFree before it called QStructFree on subPreques.

Original comment by johnp...@gmail.com on 8 Mar 2015 at 8:48

[GoogleCodeExporter]

Ok, I'll try adding that later.

Original comment by onomatop...@gmail.com on 8 Mar 2015 at 8:58

[GoogleCodeExporter]

New patch, now QsStructFree should deallocate subPreQueues elements before 
returning from mkv_Seek.

https://github.com/onomatopellan/FFmpeg/commit/9df155d

https://github.com/onomatopellan/FFmpeg/commit/9df155d.patch

Also another example of bad behavior. The white box and red box disappears when 
seeking to that point because of having the same start timecode. In this case 
it's pretty readable and the scene last for 5 seconds. But other cases could be 
annoying for the user. (file from mkvmerge 6.2.0)

Original comment by onomatop...@gmail.com on 8 Mar 2015 at 11:05

Attachments:

• CORRECT.png
• INCORRECT.png

[GoogleCodeExporter]

I think there is still a memory leak if you jump to dealloc before you've 
called QFree on the elements inside of subPrequeues because the memory that 
stores queue elements is allocated separately from the memory that stores the 
queue itself.

Original comment by johnp...@gmail.com on 8 Mar 2015 at 11:40

[GoogleCodeExporter]

Hendrik, are you okay with the behavior shown in #36 and and #27 when seeking 
in a file muxed with an old version of mkvmerge? (If you did the same seek 
using the current version of lav, you wouldn't see any subtitles at all in 
those examples.)

Original comment by johnp...@gmail.com on 8 Mar 2015 at 11:41

[GoogleCodeExporter]

I don't really care what happens on broken files, as long as it only influences 
the subs and nothing else. It will make it obvious that those files are muxed 
improperly, and thats usually good.

I just don't want to maintain a blacklist or even worse a whitelist, with 
regular additions and whatnot, thats just a rabbit whole I prefer not to go 
down into.

Original comment by h.lepp...@gmail.com on 8 Mar 2015 at 11:44

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I take back what I said about the memory leak in comment #37. I forgot that 
QStructFree calls QFree on each of the elements in the queue. Sorry about that.

Original comment by johnp...@gmail.com on 9 Mar 2015 at 12:15

[GoogleCodeExporter]

> Hendrik, are you okay with the behavior shown in #36 and and #27 when seeking 
in a file muxed with an old version of mkvmerge?
> (If you did the same seek using the current version of lav, you wouldn't see 
any subtitles at all in those examples.)

Personally I'd like to see the issue in Comment #27 resolved to show nothing, 
rather than send duplicates which don't actually exist in the script. Unless 
I'm misunderstanding something, I don't believe that 
CueDuration/CueRelativePosition are authoritative to the extent that they would 
allow such data duplication behavior for any valid purpose? I'd consider it a 
serious splitter bug anytime subtitle data is sent on seek which doesn't 
actually exist during normal playback parsing. I assume it would be as simple 
as performing a generic sanity check to ensure the splitter never sends data 
from a single matroska subtitle block multiple times at a seek point?

The issue in Comment #37 and similar which result in something being displayed 
or not displayed when seeking mid-line on invaild files, I don't see as a 
problem. Users are already familiar with some subtitles going missing when 
seeking in the middle of a line with mkv.

Original comment by cyber.sp...@gmail.com on 9 Mar 2015 at 6:26

[GoogleCodeExporter]

Good idea. I modified GetSubtitlePreroll so it doesn't read next block if it 
has the same Position and RelativePosition than previous read. It fixed both 
problems, the karaoke and the sign,  now it just shows one subtitle when 
seeking faulty files.

https://github.com/onomatopellan/FFmpeg/commit/da4eaed

https://github.com/onomatopellan/FFmpeg/commit/da4eaed.patch

Attached patched .dll for testing.

Original comment by onomatop...@gmail.com on 9 Mar 2015 at 6:14

Attachments:

• avformat-lav-56.dll

[GoogleCodeExporter]

I've noticed an issue, but I'm unsure if this is a bug in the LAV patch or 
VSFilter itself.

Occasionally when seeking the attached sample, subtitle output will hang for ~5 
seconds before recovering.

I can reproduce this with xy-VSFilter, XySubFilter, MPC-HC VSFilter, and MPC-HC 
ISR with both madVR and EVR-CP.

Original comment by cyber.sp...@gmail.com on 20 Mar 2015 at 7:50

Attachments:

• [GotWoot]_Magi_OP.7z

[GoogleCodeExporter]

I can't reproduce it. What I'm doing:
-download latest MPCHC from http://nightly.mpc-hc.org/
-replace dll in LAVFilters folder with patched dll from #43
-enable "fast seek`(on keyframe)" in MPCHC Options

This is an old PC (DualCore E2200) and it takes one second for every seek in 
that file.

Original comment by onomatop...@gmail.com on 20 Mar 2015 at 11:05

[GoogleCodeExporter]

It sometimes takes a few tries seeking to various locations in order to 
reproduce, and you'll need to disable "Fast Seek".

i5-3570K Win7
MPC-HC nightly
disabled fast seek
deleted MPC-HC LAV Filter folder
LAV Filters external (tested both your patch, and john's original patch)

It also seems a bit easier to reproduce with madVR set to a large CPU queue 
size, delay playback start disabled, and using XySubFilter. With xy-VSFilter, 
MPC-HC ISR or even using EVR-CP instead of madVR, it usually takes a bit more 
effort to reproduce.

Original comment by cyber.sp...@gmail.com on 20 Mar 2015 at 11:43

[GoogleCodeExporter]

Yes, I can reproduce it. Lowering the keyframe interval of the video to e.g. 1 
second mitigates the problem somehow.

Original comment by sneaker...@googlemail.com on 20 Mar 2015 at 11:49

Attachments:

• 24_v4.mkv

[GoogleCodeExporter]

I wonder if this problem is directly related to #302 at all. Maybe the patch 
just aggravates the problem, seeing how erratic it is.

Original comment by sneaker...@googlemail.com on 20 Mar 2015 at 12:11

[GoogleCodeExporter]

Yes, disabling fast seek now I can reproduce it too. And indeed 24_v4.mkv seeks 
way better without long hangs.

Original comment by onomatop...@gmail.com on 20 Mar 2015 at 12:32

[GoogleCodeExporter]

Seeking #44 file while being paused is also fast. So the problem is only while 
playing.

BTW this is with EVR-CP as I can't use madVR.

Original comment by onomatop...@gmail.com on 20 Mar 2015 at 12:42