Add initial support for matroska / webm #66

darksv · 2021-11-11T14:55:39Z

Hi! This PR adds an initial support for matroska and webm containers that is intended to close #63. It is far from complete, but I decided to share what I have so far to get some feedback.

pdeljanov

I gave this a quick test by enabling mkv in symphonia-play and I can see it correctly exposing the right number of tracks for the file. This is really fantastic start!

In addition to the issues noted in the review, please also make sure to put yourself as the author in the Cargo.toml file for the demuxer, and add yourself to the CONTRIBUTORS file in the project root.

symphonia-format-mkv/src/ebml.rs

symphonia-core/src/meta.rs

symphonia-format-mkv/src/ebml.rs

symphonia-format-mkv/src/element_ids.rs

symphonia-format-mkv/src/lib.rs

darksv · 2021-11-17T20:29:31Z

I tested it using symphonia-play and it seems to work on a few files already... 😄

symphonia-format-mkv/src/codecs.rs

pdeljanov · 2021-11-18T14:29:00Z

symphonia-format-mkv/src/lib.rs

+                buffer.push_back((track, reader.read_boxed_slice_exact(frame_size)?));
+            }
+        }
+        _ => unreachable!(),


This catch-all arm shouldn't be needed since the match covers all cases. If we keep it, then any additions to the Lacing will fall through to this case and cause a panic in practice.

Right. I missed it when introducing an enum.

symphonia-format-mkv/src/lib.rs

pdeljanov · 2021-11-18T14:40:19Z

Hey @darksv,

The progress is looking good! Sadly, I haven't had much luck in getting anything playing on my end, but I think it's close. I've noted in the review comments where it's getting stuck on my end.

There are also some bugs that I need to fix as well. Particularly, FlacDecoder doesn't parse extra data and instead expects some CodecParameters fields to be set. This is something I will look that will hopefully get more files playing.

darksv · 2021-11-18T18:21:33Z

I added vorbis and remaining AAC codecs. Also fixed two off-by-one errors in extract_frames

pdeljanov · 2021-11-20T04:24:01Z

Having very good results with the latest set of changes! Many of the MKVs I have with Vorbis or AAC audio tracks are working pretty flawlessly, even ones with multiple tracks. Lot's of debug logs though 😅 .

I'll push a commit into the main branch that fixes the issue with FLAC tracks in the next day or so.

pdeljanov · 2021-11-20T18:54:59Z

I've fixed the issue with FlacDecoder, however I need some support in MkvReader to come to the optimal solution.

For the optimal solution, the extra data for FLAC should just contain the Stream Info block. However, the codec private data provided by MKV contains many of the FLAC metadata blocks which are useless for the decoder and may bloat the extra data substantially if there's stuff like album art embedded into the file.

Could you make it so that only the Stream Info block is provided in the extra data?

The following code should extract the Stream Info block from the MKV codec private data which you can then add to the CodecParameters.

use symphonia_utils_xiph::flac::metadata::{MetadataBlockHeader, MetadataBlockType};

fn get_stream_info_from_codec_private(codec_private: &[u8]) -> Result<Box<[u8]>> {
    let mut reader = BufReader::new(codec_private);

    let marker = reader.read_quad_bytes()?;

    if marker != *b"fLaC" {
        return unsupported_error("mkv (flac): missing flac stream marker");
    }

    let header = MetadataBlockHeader::read(&mut reader)?;

    loop {
        match header.block_type {
            MetadataBlockType::StreamInfo => {
                break Ok(reader.read_boxed_slice_exact(header.block_len as usize)?);
            }
            _ => reader.ignore_bytes(u64::from(header.block_len))?,
        }
    }
}

darksv · 2021-11-20T19:32:39Z

I've just pushed the commit with this change.

pdeljanov · 2021-11-21T02:44:35Z

I pushed the changes required for FLAC to work. It should be good the next time you rebase.

pdeljanov · 2021-12-04T04:02:31Z

Hey @darksv,

Let me know when you think this may be ready for an initial merge. I've been a bit busy with life and other bug reports lately so I haven't been able to keep up with these changes.

When you think it may be good to go I can do a more thorough review and suggest changes to get it ready for merging.

darksv · 2022-01-09T11:24:08Z

@pdeljanov I think it might be ready for a more detailed review. Right now it still has some rough edges, but it seems to work with the files I tested.

CRC validation is not implemented yet and I need to improve handling of broken files. I believe it could be done later, after merging this PR.

pdeljanov · 2022-01-09T19:12:30Z

Hey @darksv,

First off, thank you for your hard work on this. I'd love to publish this in the upcoming version v0.5.0 release.

Aside from the review, before I can merge this there are a few issues that need to be resolved:

With larger files, it takes a very long time before playback starts. This doesn't occur when I provide the file via. standard input. I'm guessing the reader is scanning all the clusters if the input is seekable? Can we avoid that or make it more optimized?
The code must compile without any warnings. You can use #[allow(dead_code)] on structs with fields you are planning to use later. Prefixing unused variables with _ is also acceptable.
There's a lot of "Ignored Element" warnings (10000+, it exceeded my terminal's scrollback). Particularly CueRelativePosition, CueDuration, TagLanguage, and TagDefault. These should be debug level logs to avoid spamming the end-user's logs. Just in general, warnings should only be used for things that have a good chance of impacting the user.
Not really an issue, but please fix any clippy lints.

While you look into those, I'll start working on a thorough review.

Thanks again!

EDIT: Don't worry about the rustfmt failure. Symphonia's rustfmt.toml uses unstable features so it requires the nightly toolchain. We can format the project right before we merge.

darksv · 2022-01-09T20:00:11Z

I've fixed all the warnings, including clippy lints. Also changed warnings about ignored elements to debug messages.

With larger files, it takes a very long time before playback starts. This doesn't occur when I provide the file via. standard input. I'm guessing the reader is scanning all the clusters if the input is seekable? Can we avoid that or make it more optimized?

Thanks for pointing that out. I will work on this soon.

pdeljanov

I'm probably going to have to do a second round, there's a lot of code here! 😅

Just some general comments:

In the current definition of the API, any error returned by FormatReaderis basically fatal to demuxing (aside from ResetRequired). Please make sure that if you return an error that it's not possible to continue by skipping/ignoring the damaged element, cluster, or track. Of course, there is balance to be had here. We shouldn't complicate the code to try and handle completely messed up files, but if it's possible to be more tolerant of damaged files we should try. I made comments in a few locations where I think this could be done.
Since Symphonia is a library, we should not do anything that can panic (e.g., assert, unwrap, etc.). Anything fatal should return an error. I've added comments where I think this may be a problem. Using debug_assert should be fine, but still should be reserved for truly exceptional circumstances.

Thanks!

symphonia-format-mkv/src/segment.rs

symphonia-format-mkv/src/demuxer.rs

symphonia-format-mkv/src/ebml.rs

pdeljanov · 2022-01-11T04:50:42Z

symphonia-format-mkv/src/ebml.rs

+                header
+            }
+        };
+        assert_eq!(header.etype, E::ID, "reading invalid element");


Please make sure this would never occur in production.

I rewrote this function so it doesn't read the EBML header implicitly and improved messages inside the assertions.

pdeljanov · 2022-01-11T04:51:56Z

symphonia-format-mkv/src/ebml.rs

+                // TODO: ignore crc for now
+                continue;
+            }
+            assert_eq!(header.etype, E::ID);


Please make sure this would never occur in production.

replaced with just a log::warn

symphonia-format-mkv/src/ebml.rs

pdeljanov · 2022-01-11T04:57:29Z

symphonia-format-mkv/src/ebml.rs

+            Some((ty, _)) => {
+                assert_eq!(header.data_pos, self.reader.pos(), "invalid stream position");
+                if let (Some(cur), Some(end)) = (self.current, self.end) {
+                    assert!(cur.pos + cur.len <= end, "invalid stream position");


Same as above.

replaced this assertion with a DecodeError

pdeljanov · 2022-01-11T14:55:53Z

Found a couple more issues:

On end-of-stream I get a DecodeError rather than an IoError.
I've run into some MKVs that don't play at all. I get the following error instead:

ERROR symphonia_play > file not supported. reason? end of stream

It's kind of weird because the file belongs to a series and atleast some of the other episodes play fine. The file is pretty large so not sure if I can share it with you. Let me know if there's any information I can provide.

pdeljanov · 2022-01-12T05:38:53Z

I've run into some MKVs that don't play at all. I get the following error instead:

ERROR symphonia_play > file not supported. reason? end of stream

It's kind of weird because the file belongs to a series and atleast some of the other episodes play fine. The file is pretty large so not sure if I can share it with you. Let me know if there's any information I can provide.

Oh, funny, after your changes, I now get ERROR symphonia_play > file not supported. reason? malformed stream: mkv: too long element ID for this file.

darksv · 2022-01-17T08:09:33Z

Sorry, last week I wasn't able to work on this. I'll try to get back to fixing remaining issues today.

Oh, funny, after your changes, I now get ERROR symphonia_play > file not supported. reason? malformed stream: mkv: too long element ID for this file.

Interesting... I'll look into it. This probably should be fixed by skipping data in the stream until a valid tag is found.

darksv · 2022-01-17T16:00:33Z

@pdeljanov I've pushed some changes with handling of clusters with unknown sizes. Let me know if this fixes your issue.

pdeljanov · 2022-01-18T13:56:28Z

Hi @darksv,

No problem, take your time!

I get a panic instead of an error now. Please see the backtrace below:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `562413392`,
 right: `562426428`: invalid position', symphonia-format-mkv/src/ebml.rs:286:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/02072b482a8b5357f7fb5e5637444ae30e423c40/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/02072b482a8b5357f7fb5e5637444ae30e423c40/library/core/src/panicking.rs:107:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/02072b482a8b5357f7fb5e5637444ae30e423c40/library/core/src/panicking.rs:145:5
   4: symphonia_format_mkv::ebml::ElementIterator<R>::read_header_no_consume
             at ./symphonia-format-mkv/src/ebml.rs:286:9
   5: symphonia_format_mkv::ebml::ElementIterator<R>::read_header
             at ./symphonia-format-mkv/src/ebml.rs:249:22
   6: <symphonia_format_mkv::segment::ClusterElement as symphonia_format_mkv::ebml::Element>::read
             at ./symphonia-format-mkv/src/segment.rs:490:34
   7: symphonia_format_mkv::ebml::ElementIterator<R>::read_element_data
             at ./symphonia-format-mkv/src/ebml.rs:310:23
   8: <symphonia_format_mkv::demuxer::MkvReader as symphonia_core::formats::FormatReader>::try_new
             at ./symphonia-format-mkv/src/demuxer.rs:333:35
   9: <symphonia_format_mkv::demuxer::MkvReader as symphonia_core::probe::QueryDescriptor>::query::{{closure}}
             at ./symphonia-core/src/probe.rs:351:65
  10: core::ops::function::FnOnce::call_once
             at /rustc/02072b482a8b5357f7fb5e5637444ae30e423c40/library/core/src/ops/function.rs:227:5
  11: symphonia_core::probe::Probe::format
             at ./symphonia-core/src/probe.rs:317:34
  12: symphonia_play::main
             at ./symphonia-play/src/main.rs:129:11
  13: core::ops::function::FnOnce::call_once
             at /rustc/02072b482a8b5357f7fb5e5637444ae30e423c40/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

darksv · 2022-01-22T12:32:41Z

@pdeljanov Please check whether recent changes fix this.

pdeljanov · 2022-01-23T04:46:20Z

Nice work @darksv, the file plays now!

pdeljanov · 2022-01-27T03:45:14Z

Hi @darksv,

I think for the most part all the comments have been addressed. There are two issues though:

It's very slow to open a file (for me it's taking ~30 seconds for a moderately sized video).
On end-of-stream the demuxer returns a DecodeError rather than an IoError.

Are those going to be easy to fix?

I'd like to publish v0.5 early next week. However, if you think it would take longer than that to fix those issues, then we can target v0.5.1 for MKV support instead. I feel the first issue may be a show-stopper for many users so it probably wouldn't make sense to include it in v0.5 as-is.

…o read the whole file in `MkvReader::try_new`

pdeljanov · 2022-01-29T18:01:17Z

Thank you @darksv! I, and I'm sure many other people, appreciate the work you've put into implementing this.

darksv marked this pull request as draft November 11, 2021 14:55

pdeljanov requested changes Nov 12, 2021

View reviewed changes

pdeljanov reviewed Nov 18, 2021

View reviewed changes

darksv force-pushed the master branch from 911affc to e9faee5 Compare November 18, 2021 18:15

darksv force-pushed the master branch from 6b7c45a to 5aa3b64 Compare November 19, 2021 15:48

FelixMcFelix mentioned this pull request Nov 25, 2021

Driver/Input: Migrate audio backend to Symphonia serenity-rs/songbird#89

Merged

7 tasks

pdeljanov mentioned this pull request Dec 7, 2021

add mkv feature to symphonia crate #80

Closed

darksv force-pushed the master branch from 0665723 to b147eb8 Compare January 9, 2022 09:56

pdeljanov requested changes Jan 11, 2022

View reviewed changes

pdeljanov added this to the v0.5 milestone Jan 11, 2022

pdeljanov mentioned this pull request Jan 23, 2022

MP3: Mismatch with max delta from 0.002 to 2.0 #78

Closed

darksv and others added 25 commits January 29, 2022 08:10

mkv: read tags

45bde71

mkv: calculate actual frame duration when possible

580f71b

mkv: various cleanups and some missing docs

4e4ca5f

mkv: fix offsets of blocks

7e2649f

mkv: minor code improvements

63df219

mkv: add license info

7f3e194

mkv: cleanup some unwraps

439198e

mkv: run clippy and fix all the warnings

bb32517

mkv: do not warn about ignored elements

0f4dbb9

mkv: make {writing,muxing}_app optional

8a0e268

mkv: do not panic for empty vorbis packets

8b37a31

mkv: validate EMBL tag length

3d578aa

mkv: truncate track number

86c345a

mkv: do not read EBML header implicitly inside read_element_data

68970d0

mkv: seek to next supported EBML tag

b54aa0d

mkv: improve handling of clusters with unknown size

15dcd05

mkv: improve EBML parsing to be more resilient

6b4f6c5

mkv: remove some decode errors from next_element

34e7bfa

mkv: removed a possible panic in ElementIterator::read_elements

8e5fce3

mkv: use SeekHead element when possible so it is no longer required t…

87ba1d4

…o read the whole file in `MkvReader::try_new`

mkv: return IoError when an end of stream is reached

0b5142d

mkv: use

ee90e46

mkv: update Cargo.toml

d2040d2

mkv: fix last clippy lints

2935f05

mkv: run rustfmt

1ffb601

darksv force-pushed the master branch from 2b7533a to 1ffb601 Compare January 29, 2022 07:29

darksv changed the title ~~[WIP] Add initial support for matroska / webm~~ Add initial support for matroska / webm Jan 29, 2022

darksv marked this pull request as ready for review January 29, 2022 07:31

pdeljanov merged commit 9780210 into pdeljanov:master Jan 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial support for matroska / webm #66

Add initial support for matroska / webm #66

darksv commented Nov 11, 2021

pdeljanov left a comment

darksv commented Nov 17, 2021

pdeljanov Nov 18, 2021

darksv Nov 18, 2021

pdeljanov commented Nov 18, 2021

darksv commented Nov 18, 2021

pdeljanov commented Nov 20, 2021

pdeljanov commented Nov 20, 2021

darksv commented Nov 20, 2021

pdeljanov commented Nov 21, 2021

pdeljanov commented Dec 4, 2021

darksv commented Jan 9, 2022

pdeljanov commented Jan 9, 2022 •

edited

darksv commented Jan 9, 2022

pdeljanov left a comment

pdeljanov Jan 11, 2022

darksv Jan 11, 2022

pdeljanov Jan 11, 2022

darksv Jan 23, 2022

pdeljanov Jan 11, 2022

darksv Jan 23, 2022

pdeljanov commented Jan 11, 2022

pdeljanov commented Jan 12, 2022

darksv commented Jan 17, 2022

darksv commented Jan 17, 2022

pdeljanov commented Jan 18, 2022

darksv commented Jan 22, 2022

pdeljanov commented Jan 23, 2022 •

edited

pdeljanov commented Jan 27, 2022

pdeljanov commented Jan 29, 2022

Add initial support for matroska / webm #66

Add initial support for matroska / webm #66

Conversation

darksv commented Nov 11, 2021

pdeljanov left a comment

Choose a reason for hiding this comment

darksv commented Nov 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdeljanov commented Nov 18, 2021

darksv commented Nov 18, 2021

pdeljanov commented Nov 20, 2021

pdeljanov commented Nov 20, 2021

darksv commented Nov 20, 2021

pdeljanov commented Nov 21, 2021

pdeljanov commented Dec 4, 2021

darksv commented Jan 9, 2022

pdeljanov commented Jan 9, 2022 • edited

darksv commented Jan 9, 2022

pdeljanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdeljanov commented Jan 11, 2022

pdeljanov commented Jan 12, 2022

darksv commented Jan 17, 2022

darksv commented Jan 17, 2022

pdeljanov commented Jan 18, 2022

darksv commented Jan 22, 2022

pdeljanov commented Jan 23, 2022 • edited

pdeljanov commented Jan 27, 2022

pdeljanov commented Jan 29, 2022

pdeljanov commented Jan 9, 2022 •

edited

pdeljanov commented Jan 23, 2022 •

edited