Open GoogleCodeExporter opened 9 years ago
GoogleCodeExporter
commented
9 years ago 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
commented
9 years ago 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
commented
9 years ago 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:
GoogleCodeExporter
commented
9 years ago Wow, it works, what an awesome feature!Original comment by wOxxOm on 26 May 2014 at 5:40
GoogleCodeExporter
commented
9 years ago 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:
GoogleCodeExporter
commented
9 years ago ^ Probably because those lines span more than one Cue back.Original comment by wOxxOm on 26 May 2014 at 6:18
GoogleCodeExporter
commented
9 years ago If implemented properly, that shouldn't matter though.Original comment by cyber.sp...@gmail.com on 26 May 2014 at 7:36
GoogleCodeExporter
commented
9 years ago 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
commented
9 years ago > 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:
GoogleCodeExporter
commented
9 years ago > 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
commented
9 years ago 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
commented
9 years ago 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
commented
9 years ago [deleted comment]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=17642Original comment by cyber.sp...@gmail.com on 10 Jun 2014 at 10:12
GoogleCodeExporter
commented
9 years ago 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
commented
9 years ago 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:
GoogleCodeExporter
commented
9 years ago [deleted comment]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
commented
9 years ago 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
commented
9 years ago [deleted comment]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:
GoogleCodeExporter
commented
9 years ago 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
commented
9 years ago 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
commented
9 years ago 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
commented
9 years ago > 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
commented
9 years ago 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
commented
9 years ago 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:
GoogleCodeExporter
commented
9 years ago 
> 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
commented
9 years ago > 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
commented
9 years ago 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
commented
9 years ago 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
commented
9 years ago 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
commented
9 years ago 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
commented
9 years ago > 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
commented
9 years ago Ok, I'll try adding that later.Original comment by onomatop...@gmail.com on 8 Mar 2015 at 8:58
GoogleCodeExporter
commented
9 years ago 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:
GoogleCodeExporter
commented
9 years ago 
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
commented
9 years ago 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
commented
9 years ago 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
commented
9 years ago [deleted comment]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
commented
9 years ago > 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
commented
9 years ago 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:
Original issue reported on code.google.com by
h.lepp...@gmail.comon 12 Dec 2012 at 7:27