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 Chrome seeking issues with files produced by opus-recorder #248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthewwithanm
Copy link

@matthewwithanm matthewwithanm commented Aug 25, 2021

I recently discovered that Chrome has some issues with files created using this library. There are a few symptoms, including "ended" events being dispatched early and incorrect (and unstable) durations.

This codepen contains a simple demonstration of the issue.

I also opened this bug on the Chromium project.

I'm not super familiar with ogg/opus, but I believe this is a Chrome bug and not technically an issue with opus-recorder, however, the issue isn't reproducible with opus files I've created using other means (encoders on Android and iOS). So, even if the files are technically valid, there must be something that opus-recorder could do differently to avoid the Chrome issue. 🤔

Something that jumped out to me about the files Chrome liked is that they all had a start_pts/start_time (as reported by ffprobe) that matched the file's pre-skip, while affected files had a start_pts/start_time of 0. To try to confirm this was related, I modified an affected file so that its start_pts matched its pre-skip:

ffmpeg -i bad.ogg -acodec copy -copyts -muxdelay 0 -muxpreload 0 -output_ts_offset 0.08 copy.ogg

The result no longer exhibited the problem in Chrome! 🎉

I ran the command again on the copy with an output_ts_offset of 0, just to verify that the issue came back (and therefore it wasn't being fixed by some other random thing the command was doing).

To translate this change to the library, I set the initial granule position to the offset (pre-skip) when encoding. I think this tracks with the spec's section on PCM sample position:

The PCM sample position is determined from the granule position using the following formula:

'PCM sample position' = 'granule position' - 'pre-skip'

Interestingly, when I tried to set both the initial granule position and pre-skip to a value besides 3840, the issue returned! (It didn't repro on files created on other platforms that had matching, but different, start_pts and pre-skip values.) I'm not sure why this is; maybe the value is hardcoded into the compiled libopus?

Like I said, I'm not opus expert, so I'd love to hear your thoughts on this. Thanks!

@chris-rudmin
Copy link
Owner

@matthewwithanm Thanks for the thorough investigation. I'll take a look, and see if I can reproduce the issue.

@chris-rudmin
Copy link
Owner

@matthewwithanm I recently spotted this issue with ffmpeg opus pre-skip. Not sure if the fix made it upstream yet. Could it be related?

@matthewwithanm
Copy link
Author

@matthewwithanm I recently spotted this issue with ffmpeg opus pre-skip. Not sure if the fix made it upstream yet. Could it be related?

It seems like it could be given that the same things are involved, but I don't think I have the knowledge to say for certain 🙈

@matthewwithanm
Copy link
Author

@chris-rudmin Sorry to bug, but were you able to reproduce? Or do you have any more insight into the issue?

Even if my fix isn't the right one, I do think this should be addressed in the library since this isn't an issue I've experienced with other encoders—regardless of whether there's a bug with ffmpeg/Chrome.

@chris-rudmin
Copy link
Owner

@matthewwithanm I haven't had time to test it out. I'm hesitant to pull this change in before confirming that it adheres (or does not adhere) to the ogg specification.

@matthewwithanm
Copy link
Author

@chris-rudmin Makes total sense! Getting a spec-correct file that also makes Chrome happy is the goal! ❤️

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

Successfully merging this pull request may close these issues.

2 participants