Fix m4a and mp4a playback #582

apolunar · 2024-06-01T22:46:19Z

Addresses issue #581 where all tests (including the new one) pass.

It includes the necessary symphonia features
Handles "end of stream" decoding errors
Selects only tracks with supported codecs
Adds test for m4pa file from YouTube (CC License)
Let me know what revisions need to be made to get this merged.

apolunar · 2024-06-01T22:48:54Z

Yikes, well it worked on my machine LOL
Edit: I did not in fact have the correct test command (should've been cargo test --features=symphonia-all --all-targets)
However, the new mp4a fix is successful, just not seeking on the RL.m4a, which is something I will fix later (for now maybe we can just disable the test again)

dvdsk · 2024-06-01T23:49:24Z

just not seeking on the RL.m4a, which is something I will fix later (for now maybe we can just disable the test again)

I strongly suspect that's a bug in symphonia, see: #577. I haven't been able to work on bringing that to the attention of the symphonia folk yet unfortunatly.

Addresses issue #581 where all tests (including the new one) pass.

Thank you so much for adding a test 👍 that makes reviewing sooo much easier. I'm gonna take a detailed look tomorrow! At a quick glance it seems great though!

dvdsk · 2024-06-02T14:55:08Z

Cargo.toml

@@ -15,7 +15,7 @@ claxon = { version = "0.4.2", optional = true }
 hound = { version = "3.3.1", optional = true }
 lewton = { version = "0.10", optional = true }
 minimp3_fixed = { version = "0.5.4", optional = true}
-symphonia = { version = "0.5.2", optional = true, default-features = false }
+symphonia = { version = "0.5.4", optional = true, default-features = false, features = ["all"] }


This increases compile time, is it needed to fix m4a? Might be better to adjust the features below.

yes we absolutely shouldn't enable all features by default

Sorry, this was a quick fix I forgot to actually investigate 😓

This increases compile time, is it needed to fix m4a? Might be better to adjust the features below.

I'm not really sure where to put them below, but it seems like you need features = ["aac", "isomp4"] for support

dvdsk · 2024-06-02T14:59:18Z

src/decoder/symphonia.rs

-            let current_frame = probed.format.next_packet()?;
+            let current_frame = match probed.format.next_packet() {
+                Ok(packet) => packet,
+                Err(_) => break decoder.last_decoded() // IoError end of stream is expected


might make sense to match on the IoError explicitly and return all the other errors.

dvdsk · 2024-06-02T15:01:29Z

src/decoder/symphonia.rs

+            .iter()
+            .find(|t| t.codec_params.codec != CODEC_TYPE_NULL).unwrap().id;
+
+        let decode_opts = Default::default();


might make sense to inline this down below

dvdsk · 2024-06-02T15:02:07Z

src/decoder/symphonia.rs

        let mut decoder = symphonia::default::get_codecs()
-            .make(&stream.codec_params, &DecoderOptions { verify: true })?;
+            .make(&track.codec_params, &decode_opts)?;


so this would be something like: .make(...., &DecoderOptions::default())?

dvdsk · 2024-06-02T15:04:12Z

running cargo fmt should fix that last failing test

dvdsk · 2024-06-02T15:17:41Z

Some nitpicks and a small mistake regarding the crate features otherwise looks great. With some tweaks we can get it merged!

apolunar · 2024-06-02T23:33:22Z

Some nitpicks and a small mistake regarding the crate features otherwise looks great. With some tweaks we can get it merged!

Yippee! I think I covered everything, thanks for pointing that stuff out.

dvdsk · 2024-06-03T10:00:32Z

src/decoder/symphonia.rs

-
-        let decode_opts = Default::default();
+            .find(|t| t.codec_params.codec != CODEC_TYPE_NULL)
+            .unwrap()


This panics if there is not track with a valid codec or if there are no tracks at all. It would be better to return a DecodeError I think. Since a music player (one of the usecases of rodio) might very well run into bad files.

I'm not sure what error to return / where to put the error string, as I think it's bad practice to just put it in the file as I did in the most recent commit.

dvdsk · 2024-06-03T10:01:44Z

src/decoder/symphonia.rs

-        let mut decoder = symphonia::default::get_codecs()
-            .make(&track.codec_params, &decode_opts)?;
+        let mut decoder =
+            symphonia::default::get_codecs().make(&track.codec_params, &Default::default())?;


another nitpick (feel free to ignore if you disagree). I personally find: Type::trait_fn clearer then Trait::trait_fn(). I think the former would be DecoderOpt::default() here.

dvdsk · 2024-06-03T10:02:42Z

src/decoder/symphonia.rs

@@ -97,7 +104,10 @@ impl SymphoniaDecoder {
        let decoded = loop {
            let current_frame = match probed.format.next_packet() {
                Ok(packet) => packet,
-                Err(_) => break decoder.last_decoded() // IoError end of stream is expected
+                Err(e) => match e {


You can inline this match so:
Err(Error::IoError(_)) => break etc etc,
Err(other) => return Err(other),

dvdsk · 2024-06-05T15:12:33Z

src/decoder/symphonia.rs

            .format
            .tracks()
            .iter()
            .find(|t| t.codec_params.codec != CODEC_TYPE_NULL)
-            .unwrap()
-            .id;
+        {


Looking good, thanks again for the investigation and fix!

One more tweak you could try (if you want to) is to replace the match with something using Option::ok_or, that would look something like this:

.find(...) .ok_or(symphonia::core::errors::Error::Unsupported(...))? .id;

But if you rather leave it thats fine too (let me know)! This code is better then most in rodio so we are good to merge it without any more changes :)

apolunar · 2024-06-06T21:32:28Z

YIPPEE! Thanks for your help with this :D

dvdsk · 2024-06-06T21:39:52Z

Cargo.toml

@@ -15,7 +15,7 @@ claxon = { version = "0.4.2", optional = true }
 hound = { version = "3.3.1", optional = true }
 lewton = { version = "0.10", optional = true }
 minimp3_fixed = { version = "0.5.4", optional = true}
-symphonia = { version = "0.5.2", optional = true, default-features = false }
+symphonia = { version = "0.5.4", optional = true, default-features = false, features = ["aac", "isomp4"] }


oops, these should be gone too, my bad, no worries ill do it after the merge

done see (a3ce660 and a3ce660)

possibly fixed m4a and mp4a playback

b712559

apolunar mentioned this pull request Jun 1, 2024

Specific mp4a codec panics rodio #581

Closed

apolunar added 2 commits June 1, 2024 20:55

re-disable symphonia-isomp4 test due to (upstream?) failure

d08d9e7

fix clippy complaint that the struct is overdeclared

6bbd95e

dvdsk requested changes Jun 2, 2024

View reviewed changes

fix formatting, error handling, and remove uneccesary features

3bc9647

apolunar closed this Jun 2, 2024

apolunar reopened this Jun 2, 2024

dvdsk reviewed Jun 3, 2024

View reviewed changes

improved error handling

5f1b057

dvdsk reviewed Jun 5, 2024

View reviewed changes

improved error handling pt 2

eac5a32

dvdsk approved these changes Jun 6, 2024

View reviewed changes

dvdsk reviewed Jun 6, 2024

View reviewed changes

dvdsk merged commit 23fe481 into RustAudio:master Jun 6, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix m4a and mp4a playback #582

Fix m4a and mp4a playback #582

apolunar commented Jun 1, 2024

apolunar commented Jun 1, 2024 •

edited

Loading

dvdsk commented Jun 1, 2024

dvdsk Jun 2, 2024

est31 Jun 2, 2024

apolunar Jun 2, 2024

dvdsk Jun 2, 2024

dvdsk Jun 2, 2024

dvdsk Jun 2, 2024

dvdsk commented Jun 2, 2024

dvdsk commented Jun 2, 2024

apolunar commented Jun 2, 2024

dvdsk Jun 3, 2024

apolunar Jun 5, 2024

dvdsk Jun 3, 2024

dvdsk Jun 3, 2024

dvdsk Jun 5, 2024 •

edited

Loading

apolunar commented Jun 6, 2024

dvdsk Jun 6, 2024 •

edited

Loading

dvdsk Jun 6, 2024 •

edited

Loading

Fix m4a and mp4a playback #582

Fix m4a and mp4a playback #582

Conversation

apolunar commented Jun 1, 2024

apolunar commented Jun 1, 2024 • edited Loading

dvdsk commented Jun 1, 2024

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

dvdsk commented Jun 2, 2024

dvdsk commented Jun 2, 2024

apolunar commented Jun 2, 2024

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

dvdsk Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

apolunar commented Jun 6, 2024

dvdsk Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

dvdsk Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

apolunar commented Jun 1, 2024 •

edited

Loading

dvdsk Jun 5, 2024 •

edited

Loading

dvdsk Jun 6, 2024 •

edited

Loading

dvdsk Jun 6, 2024 •

edited

Loading