Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Opus audio in fragmented MP4 #1219

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

bvibber
Copy link

@bvibber bvibber commented Nov 3, 2021

The Opus box name is capitalized, but MediaSource expects a lowercase
"opus" codec string, which was causing unexpected failures in Chrome
and Firefox.

Now matches behavior of mux.js for audio tracks by forcing the box
type to lowercase before returning it as a codec string.

Description

HLS streams with Opus audio tracks in fragmented MP4 weren't working in Chrome and Firefox due to reading the codec string as "Opus" with a capital "O", which MediaSource rejects.

Specific Changes proposed

Forcing the box name string to lowercase resolves the specific problem with Opus; all other known codec strings are lowercase. mux.js appears to use the same logic when returning codec strings, so this is not expected to be problematic.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

The Opus box name is capitalized, but MediaSource expects a lowercase
"opus" codec string, which was causing unexpected failures in Chrome
and Firefox.

Now matches behavior of mux.js for audio tracks by forcing the box
type to lowercase before returning it as a codec string.
@@ -440,7 +440,8 @@ const handleSegmentBytes = ({
// if we have a audio track, with a codec that is not set to
// encrypted audio
if (tracks.audio && tracks.audio.codec && tracks.audio.codec !== 'enca') {
trackInfo.audioCodec = tracks.audio.codec;
// note Opus box name is capitalized, but things want lowercase for checks
trackInfo.audioCodec = tracks.audio.codec.toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We get this information from mux.js specifically lib/mp4/probe.js and we toLowerCase the codec there if it isn't one that we know. I guess i'm just not understanding how we can get here with a codec that isn't lowercase. Do you have a test source that we can look at?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, here's my test page that combines a few things:
https://brionv.com/misc/hls-test/ogv.html

The Opus audio track: https://brionv.com/misc/hls-test/caminandes-llamigos.webm.audio.opus.mp4

The actual codec fourCC for Opus in MP4 is "Opus" (0x7375704F) -- it is not lowercased in the actual binary file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll pull up the local source and double-check versions of everything when I get a chance -- feel free to shoot any questions on my usage and I'll try my best to answer. :)

I'm not 100% confident I'm correctly labeling everything in the m3u8 for the Opus track version. (iOS accepts the MP3 audio version.)

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #1219 (7a4844f) into main (bdd842a) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1219   +/-   ##
=======================================
  Coverage   85.40%   85.40%           
=======================================
  Files          40       40           
  Lines        9963     9963           
  Branches     2308     2308           
=======================================
  Hits         8509     8509           
  Misses       1454     1454           
Impacted Files Coverage Δ
src/media-segment-request.js 95.50% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants