Closed Bug 1320705 Opened 7 years ago Closed 7 years ago

Opus decoding regression, long tail at the end of files since Firefox 50

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Version:
50 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

(Keywords: regression)

Attachments

(12 files)

Top: nigtly, incorrect. Bottom, Firefox 49, correct
52.75 KB, image/png
Details
test case, untar and open in a browser
9.89 KB, application/gzip
Details
Bug 1320705: P1. Fix function prototyping.
58 bytes, text/x-review-board-request
mozbugz
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1320705: P2. Add mDiscardPadding information.
58 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1320705: P3. Update Opus decoder to discared mDiscardPadding.
58 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1320705: P4. Pass discard padding information from webm container.
58 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1320705: P5. Pass discard padding information from ogg container.
58 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1320705: P6. Fix codec mimetypes.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1320705: P7. Ensure audio decoder is recreated when chaining encountered.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1320705: P8. Don't use stagefright to decode vorbis.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Add a test to check that decoding an Opus file does to produce a long tail
4.84 KB, patch
jya
: review+
Details | Diff | Splinter Review
Add a test to check that decoding an Opus file does to produce a long tail
14.15 KB, patch
jya
: review+
Details | Diff | Splinter Review 
Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e5859dfe0bcbd40f4e33f4a633f73ea3473a7849&tochange=ffac2798999c5b84f1b4605a1280994bb665a406

See attached test-case and screenshot of the test case.

`opusdec tiny-clip.opus out.wav` yields a file that does not have a long tail, as does Firefox <= 49 and Chrome Canary (they recently added support for Opus in `decodeAudioData`, and reported this bug by email).

This breaks seamless looping when using Opus files, it's quite a serious issue.
Has Regression Range: --- → yes
Has STR: --- → yes
The test case (and the screenshot of the test case) decode the file, print the duration, and draws the end of the file.
Jean-Yves, would you mind having a look? There are a bunch of OGG stuff from you in the range. Let me know if you have any question with regard to the Web Audio API.

The call into the media decoding infrastructure happens at [0] in our code. We don't do anything with the audio, apart from optionally re-sampling the audio to the AudioContext sample-rate. I've double checked, this is not the resampler filter tail, here, this happens even if we're not re-sampling.

[0]: http://searchfox.org/mozilla-central/source/dom/media/webaudio/MediaBufferDecoder.cpp#322
Flags: needinfo?(jyavenard)
Depends on: 1288329
Rank: 15
No longer depends on: 1288329
Priority: -- → P1
Depends on: 1288329
Sounds like something went wrong with sample accurate end-trimming? You might also be able to use <https://people.xiph.org/~greg/opus_testvectors/correctness_trimming_nobeeps.opus> to verify.
Assignee: nobody → jyavenard
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
Flags: needinfo?(jyavenard)
Keywords: regression
Comment on attachment 8815669 [details]
Bug 1320705: P1. Fix function prototyping.

https://reviewboard.mozilla.org/r/96510/#review96880
Attachment #8815669 - Flags: review?(gsquelart) → review+
Comment on attachment 8815670 [details]
Bug 1320705: P2. Add mDiscardPadding information.

https://reviewboard.mozilla.org/r/96512/#review96968
Attachment #8815670 - Flags: review?(kinetik) → review+
Comment on attachment 8815672 [details]
Bug 1320705: P4. Pass discard padding information from webm container.

https://reviewboard.mozilla.org/r/96516/#review96974

::: dom/media/webm/WebMDemuxer.cpp:700
(Diff revision 1)
>      if (discardPadding && i == count - 1) {
> -      uint8_t c[8];
> -      BigEndian::writeInt64(&c[0], discardPadding);
> -      sample->mExtraData = new MediaByteBuffer;
> -      sample->mExtraData->AppendElements(&c[0], 8);
> +      CheckedInt64 discardFrames = TimeUnitToFrames(
> +        media::TimeUnit::FromNanoseconds(sample->mDiscardPadding),
> +        mInfo.mAudio.mRate);
> +      if (discardFrames.isValid()) {
> +        sample->mDiscardPadding = discardFrames.value();

CheckedInt64 into uint32_t, need to check that this won't overflow
Attachment #8815672 - Flags: review?(kinetik) → review+
Comment on attachment 8815671 [details]
Bug 1320705: P3. Update Opus decoder to discared mDiscardPadding.

https://reviewboard.mozilla.org/r/96514/#review96970

::: dom/media/platforms/agnostic/OpusDecoder.cpp:247
(Diff revision 1)
> -      return MediaResult(
> -        NS_ERROR_DOM_MEDIA_FATAL_ERR,
> -        RESULT_DETAIL("Discard padding larger than packet"));
> -    }
>      OPUS_DEBUG("Opus decoder discarding %d of %d frames",
> -        int32_t(discardFrames.value()), frames);
> +               int32_t(aSample->mDiscardPadding), frames);

Drop the cast and log as %u here

::: dom/media/platforms/agnostic/OpusDecoder.cpp:253
(Diff revision 1)
>      // Padding discard is only supposed to happen on the final packet.
>      // Record the discard so we can return an error if another packet is
>      // decoded.
>      mPaddingDiscarded = true;
> -    int32_t keepFrames = frames - discardFrames.value();
> +    int32_t keepFrames =
> +      std::max<int32_t>(0, frames - int32_t(aSample->mDiscardPadding));

uint32_t into int32_t, check for overflow
Attachment #8815671 - Flags: review?(kinetik) → review+
Comment on attachment 8815673 [details]
Bug 1320705: P5. Pass discard padding information from ogg container.

https://reviewboard.mozilla.org/r/96518/#review96976

::: dom/media/ogg/OggCodecState.cpp:1256
(Diff revision 1)
> +  if (data->mEOS && mPrevPacketGranulepos != -1) {
> +    // If this is the last packet, perform end trimming.
> +    int64_t startFrame = mPrevPacketGranulepos;
> +    frames -= std::max<int64_t>(
> +      0, std::min(endFrame - startFrame, static_cast<int64_t>(frames)));
> +    data->mDiscardPadding = frames;

int64_t into uint32_t, check for overflow
Attachment #8815673 - Flags: review?(kinetik) → review+
Comment on attachment 8815672 [details]
Bug 1320705: P4. Pass discard padding information from webm container.

https://reviewboard.mozilla.org/r/96516/#review96974

> CheckedInt64 into uint32_t, need to check that this won't overflow

From the Opus decoder code it states:
// Maximum value is 63*2880, so there's no chance of overflow.

I'll add a test that if it's greater than this, it will be ignored then.
Comment on attachment 8815671 [details]
Bug 1320705: P3. Update Opus decoder to discared mDiscardPadding.

https://reviewboard.mozilla.org/r/96514/#review96970

> uint32_t into int32_t, check for overflow

may make the field an int64_t so remove all those cast/overflow issue then. at the cost of adding 4 bytes per sample.
Comment on attachment 8815671 [details]
Bug 1320705: P3. Update Opus decoder to discared mDiscardPadding.

https://reviewboard.mozilla.org/r/96514/#review96970

> may make the field an int64_t so remove all those cast/overflow issue then. at the cost of adding 4 bytes per sample.

actually, made all variables uint32_t instead. no more int32_t
See Also: → 1321511
Comment on attachment 8815673 [details]
Bug 1320705: P5. Pass discard padding information from ogg container.

https://reviewboard.mozilla.org/r/96518/#review96976

> int64_t into uint32_t, check for overflow

that can't overflow.

frames is a uint32_t

std::min(endFrame - startFrame, static_cast<int64_t>(frames)));

we have startFrame < endFrames always unless it's corrupted, the end result of this operation is at the most frames.

If we did have startFrame > endFrames, then std::max<int64_t>(0, result_above) will give us 0.

So the lowest possible value stored is 0, and the highest possible value is the original frames value (we would then dropped the entire packet)
Comment on attachment 8816343 [details]
Bug 1320705: P7. Ensure audio decoder is recreated when chaining encountered.

https://reviewboard.mozilla.org/r/97110/#review97346
Attachment #8816343 - Flags: review?(gsquelart) → review+
See Also: → 1322070
Comment on attachment 8816805 [details]
Bug 1320705: P8. Don't use stagefright to decode vorbis.

https://reviewboard.mozilla.org/r/97344/#review97650
Attachment #8816805 - Flags: review?(gsquelart) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/255480c6f63c
P1. Fix function prototyping. r=gerald
https://hg.mozilla.org/integration/autoland/rev/bea4ed2781a8
P2. Add mDiscardPadding information. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/18b5d19d0abc
P3. Update Opus decoder to discared mDiscardPadding. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/d330cc7e9094
P4. Pass discard padding information from webm container. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/7495e866529e
P5. Pass discard padding information from ogg container. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/05a07920b832
P6. Fix codec mimetypes. r=gerald
https://hg.mozilla.org/integration/autoland/rev/328475b7579c
P7. Ensure audio decoder is recreated when chaining encountered. r=gerald
https://hg.mozilla.org/integration/autoland/rev/52cc3c8b17c9
P8. Don't use stagefright to decode vorbis. r=gerald
I guess we want this to ride the trains with 53.
status-firefox50: affected → wontfix
status-firefox51: affected → wontfix
status-firefox52: affected → wontfix
Actually, was about to request uplift approval for those as it's an important regression for webaudio users
Blocks: 1288329
No longer depends on: 1288329
Comment 0 makes this sound like something we want to consider uplifting, though I'm not sure how far.
Though I have to say that I find it worrying that for as bad as comment 0 makes this sound, this wasn't discovered until 4 months after the regressing patch landed *and* this bug landed without any new tests that would have caught the problem.
We're nearly out of time to land anything on 51 at this point, so if you intend to request uplift on these patches, please do so soon.
Flags: needinfo?(jyavenard)
Flags: needinfo?(ajones)
I have no idea on how we could test this using mochitest or gtest... It's an audible change, while it may sound obvious, creating a regression test is less than trivial
Flags: needinfo?(jyavenard)
Flags: needinfo?(ajones)
Comment on attachment 8815669 [details]
Bug 1320705: P1. Fix function prototyping.

Approval request is for all changes
Approval Request Comment
[Feature/Bug causing the regression]:1288329
[User impact if declined]: invalid decoding using webaudio and opus.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: by myself
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: just the changes in this bug entry
[Is the change risky?]: no
[Why is the change risky/not risky?]: we re-introduce an earlier behaviour that got lost when bug 1288329 landed
[String changes made/needed]: none
Attachment #8815669 - Flags: approval-mozilla-aurora?
Here is a test that would have caught this.
Attachment #8827847 - Flags: review?(jyavenard)
Assignee: jyavenard → padenot
Comment on attachment 8827847 [details] [diff] [review]
Add a test to check that decoding an Opus file does to produce a long tail

Review of attachment 8827847 [details] [diff] [review]:
-----------------------------------------------------------------

Brownie points for adding an Opus Head test too :)
Attachment #8827847 - Flags: review?(jyavenard) → review+
Comment on attachment 8815669 [details]
Bug 1320705: P1. Fix function prototyping.

52 is now beta, updating uplift request.  I'm hoping the test can land before approving this.
Attachment #8815669 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Paul, can you please land this test when you get a chance? :)
Flags: needinfo?(padenot)
Ryan, it's orange for now, and I don't understand why just by looking at the code. It's orange everywhere (debug/opt, all platforms) with the same results, including on TC job, so I'm just going to debug that in a one-click-loaner. Of course, it's always green locally.

Unfortunately, other higher-priority stuff have come up, but I'm trying to get back to this as soon as possible.
Flags: needinfo?(padenot)
Comment on attachment 8815669 [details]
Bug 1320705: P1. Fix function prototyping.

Let's get this patch set into 52.0b2.
Attachment #8815669 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Too much issues with MediaRecorder, here is something simpler. This is green on
try.
Attachment #8832579 - Flags: review?(jyavenard)
Attachment #8832579 - Flags: review?(jyavenard) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23ab565c95bf
Add a test to check that decoding an Opus file does to produce a long tail. r=jya
Depends on: 1325707
You need to log in before you can comment on or make changes to this bug.