Add support for Matroska's Ordered Chapters and Segment Linking

[mojo-hakase]

Description

Adds a timeline demuxer that holds a set of ffmpeg demuxer and transparently switch between them and seeks to right time in the real demuxer for playing matroska's ordered chapters correctly.
When the CDVDFactoryDemuxer successfully creates the CDVDDemuxFFmpeg from a file, the factory will try to create a CDemuxTimeline by calling CDemuxTimeline::CreateTimeline() with the created ffmpeg demuxer as parameter. That function will then check the file for ordered chapters. If the file doesn't use ordered chapters a nullptr is returned, and the factory returns the ffmpeg demuxer as usual.
If the file uses ordered chapters a CDemuxTimeline gets created. It takes over the ffmpeg demuxer as "primaryDemuxer", and gets returned by the factory instead of the ffmpeg demuxer. The timeline creation may involve searching for missing mkv segments in the same directory as the source file if necessary.

Motivation and Context

It implements an feature that is requested since 2009 and discussed in this thread in the kodi forum:
http://forum.kodi.tv/showthread.php?tid=55764

How Has This Been Tested?

As my patch affects the opening of the demuxer, i tested various files without ordered chapters for still being playable and various files with ordered chapters for now begin played correctly.

Screenshots (if appropriate):

Types of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the Code guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the CONTRIBUTING document
• I have added tests to cover my change
• All new and existing tests passed

[garbear]

This is a little hard to review in its current state. Can you squash the commits to create a sensible history? The file you added contains a LOT of stuff. Try to separate this into as many files as possible - this usually shows good modularization and can reveal some architecture mistakes.

[mojo-hakase]

All right. Will try to seperate the code.
How do i squash commits?

[mojo-hakase]

Nevermind. Just googled it.

[razzeee]

For safety reasons, you might want to backup the folder your working on. Just in case.

[jjd-uk]

If you need any advice with anything to get this PR into shape then #kodi-dev IRC channel is available

[ace20022]

@mojo-hakase do you still working on the squashing/restructuring or shall we start reviewing?

[mojo-hakase]

I am rewriting the whole matroska parsing part and need some more time. Probably next week. Will post a comment if I'm ready.

[mojo-hakase]

I would like you to review my code.
I am still not fully satisfied with my code and want to change some things, but i think its sufficient. I also think it will be useful to get some feedback and get to know what you expect from the code.
Is it okay if i do the commit squashing after the review is done and any further changes you want to be made are made or shall I do it prior?
Checks:
gcc fails because gcc 4.8.4 isn't able to expand template packs in lambda capture lists. gcc 4.9 and newer supports it. Shall I write a workaround?

[garbear]

Please squash prior to review, github used to lose comments when you force-pushed (I think now it just hides them)

[mojo-hakase]

Ok, squashed it.

[ace20022]

@mojo-hakase sorry for the long silence. I really want to have that feature reviewed (and merged sometimes). I hope I can find some time in the next days. One question beforehand: why don't you use libebml?

[mojo-hakase]

@ace20022 no problem, take your time.
I considered using libebml but i didn't get to like it. It seemed to be too complicated for what i wanted. I looked at some example code and realized that despite that huge library i would still need to write alot of code. I think my code is much more readable than it would be when using libeml. (JMO)

[ace20022]

I skimmed through the non-parser code. Nice work.
@Paxxi Could you please review the parser code when you have some time?

The parser classes/files need to go somewhere else, not sure where at the moment.

[mojo-hakase]

So...
How's it going with the review?

[jjd-uk]

I'm guessing this would also need review by @FernetMenta

[FernetMenta]

This has to be discussed with ffmpeg: https://ffmpeg.org/pipermail/ffmpeg-devel/2008-December/043813.html

Bypassing ffmpeg demuxer is the wrong strategy.

[mojo-hakase]

@FernetMenta: What do you mean by "bypassing" ffmpeg demuxer?
I hope you don't mean, that I open multiple ffmpeg demuxers and switch between them while playing.
If you mean that I run my own Matroska parser parallel to the ffmpeg demuxer to get the information i need, I totally agree with you.
I think ffmpeg must not do the segment linking because it would mean you could no longer demux the original streams and ffmpeg must not search for the segments by it self because i can't know where to search and therefor would need some kind of callback to ask the user code (which was already refused as a solution a long time ago).
I think ffmpeg should extract some more information (i.e. Editions, UUID) from the matroska file and expose it somehow to the user code. For me the key question is that "somehow" as ffmpeg demuxers are working transparent to the user code and there is no direct way for them to expose extra non-generic information except the metadata dictionary, which is not an option in this case.
Adding a field to AVFormatContext would be an easy option. Another way would be to add the field at the front of the matroskadec privdata, so casting the void *privdata would give you the exposed information.
I'll try to discuss this on the ffmpeg-devel irc channel.

This has to be discussed with ffmpeg: https://ffmpeg.org/pipermail/ffmpeg-devel/2008-December/043813.html

What does the link have to do with it?

[FernetMenta]

Writing an additional demuxer for Kodi is wrong. If you want to add this functionality, you have to implement it in ffmpeg.

[mojo-hakase]

I think not.
First of all, I haven't written an additional demuxer. I have written a timeline composer. You may say, "it doesn't matter how you call it". I'll explain what I mean further below.

I think the ffmpeg demuxer is not the right place to implement ordered chapters. Ordered Chapters are just metainformations meant as a hint for the player(!) in which order to play the media. Therefor it should not be implemented by a low level general purpose demuxer that is not dedicated to player applications only, since transmuxing and transcoding applications may not want this behaviour.
Consider following example:
You have a Video including a 5 minutes part that repeats 10 times somewhere in the middle using ordered chapters. Now you want to transcode your video from 1080p to 720p to save disk space. The demuxer is aware of ordered chapters and transparently seeks to the repeating part for you to display it every time it occurs in the timeline. But you don't want to display it, you want to transcode. What happens, is that you transcode and write the 5 minutes part 10 times instead of 1. That is definitely not what you wanted. You wanted to keep the deduplication solution to save disk space.

We could add an boolean field to AVFormatContext, that tells the matroska demuxer in the case of ordered chapters what to you. Although, it seems off, to set a parameter for a specifig demuxer, before knowing what kind demuxer will be used. But it wouldn't be that bad.
The bigger problem is, the demuxer often can't build the timeline, because it lacks information. That is, for example, when we have multiple editions. The demuxer could choose a default edition, but then why do we have multiple, if we don't offer the user to choose. But that would mean to introduce a new interface to the demuxer for user interaction. (User interaction with the demuxer?!)
Another example are interactive movies, that utilize the same technique. We also have chapter codecs, that define when to jump to a specific chapter. Should all of it be implemented in the demuxer?

In my previous comment I have already written about the problems and the need of breaking the API, when implementing segment linking in the demuxer.
But in the case of segment linking in the demuxer, we get further even bigger problems. The demuxer can't simply concatenate the streams. You may have Videos streams with different resolution or audio streams with different sample rate or channel count. You may say, "you shouldn't link those files in the first place". You may be right, although I have already seen those files. But what you typically link together, are files with ASS subtitle stream, which have there own priv data. You just can't simply concatenate these streams. If the demuxer would be in charge of this, it would need know about the ASS codec and how to concatenate them correctly, which is definitely NOT what a demuxer should do.

All these problems are because ordered chapters and segment linking aren't meant to be done by the demuxer but by an additional layer in the player application.
The easiest place for this additional layer, is directly after the demuxer. And that is what I have written, not a demuxer. In that way, for example, we have no problem concatenate the ASS streams as we can change the ASS context for the player. Also the segment linking can easily be done in that layer, as we know the origin of the source. There is no problem with extending the interface between that layer and the player gui for selecting the desired edition.

So, I think adding a new layer to the player, that is in charge of building the timeline and transparently seeking in the source, is the right way to implement this feature. Although direcly after the demuxer may not be the perfect place for it, but at first is the easiest.

I will discuss this on the ffmpeg-devel irc channel.

What do you think?

[FernetMenta]

Currently busy with real life job. Will replay later this week.

[FernetMenta]

So, I think adding a new layer to the player, that is in charge of building the timeline and transparently seeking in the source, is the right way to implement this feature.

Maybe, but this is not reflected by this code. I don't see this additional layer. You just implemented the demuxer interface.

Some weaknesses I noticed in this code:

• dependency to DemuxerFFmpeg. Only the factory should instantiate demuxers
• same for inputStreams. You should not bypass the factory. Further DVDInputStreamFile may not be the appropriate class needed
• Overlap with parsers (ebl, mkv) we already have in ffmpeg

[mojo-hakase]

ebml,mkv parser: that's right. As I wrote in a previous comment, I would like to make some changes in the ffmpeg demuxer, so I won't need that parser code anymore.
InputStreams: that's fixable. As I just accept files to be linked together I thought I may directly use DVDInputStreamFile but it's not necessary.

The Problem is with bypassing the demuxer factory. As I injected the TimelineDemuxer creation into the Factory, I could not prevent an endless recursiv call between TimelineDemuxer and DemuxerFactory. It's a result of my Code not really being an additional layer, as you correctly pointed out.

I will reconsider my code and think of another way to implement this.
But in the next year.

I wish you all a Happy New Year.

[rdnetto]

Is this still being worked on? It would be a shame for it to never get merged, given that the core logic has been implemented.

[jenkins4kodi]

@mojo-hakase this needs a rebase

[bigretromike]

This would be a great addition for anime lovers community (because its the only place I find those).
As its 2019 right now, and kodi 18 introduce everything that support external files as a addon system, could we please have this as a custom timeline parsers? maybe ? @mojo-hakase you did awesome job (hope he is still active), ffmpeg didn't want this as a patch, kodi didn't want it as a 'custom demuxer'.

Maybe in the addon system there is a place for this ?!

[phunkyfish]

PR has merit but will labelled PR Cleanup: Backburner and closed for now. Would be a candidate for inclusion down the line if someone wished to take it on.

If the author would like it kept open please just request so on the PR.