1264199 - Allow mechanism to change audio configuration mid-stream

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect)

Description

[Jean-Yves Avenard [:jya]]

There may very well be an API for this, but not knowing if it does, I will create this task.

Aim is to have native handling of audio change mid-stream without having to work around in the AudioSink to ensure that cubeb only ever sees a single content.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

This will allow to easily detect audio configuration change prior immediate playback.

Review commit: https://reviewboard.mozilla.org/r/46059/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46059/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

The audio is automatically converted to always match the format of the first processed sample.
This is a temporary approach, as it would be preferred to use a final sampling rate not causing too much quality loss.

Review commit: https://reviewboard.mozilla.org/r/46061/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46061/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

We attempt to avoid unnecessary resampling of 44.1kHz and 48kHz content, for all others we use cubeb's preferred sampling rate as final sampling rate.

Review commit: https://reviewboard.mozilla.org/r/46063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46063/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review commit: https://reviewboard.mozilla.org/r/46065/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46065/

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Performing all audio processing operations in the same place, allows to simplify the code.

Additionally, if accessibility.monoaudio.enable is not set, we always upmix mono to stereo so that if the first audio stream seen was mono, we aren't stuck playing all future streams in mono.

Review commit: https://reviewboard.mozilla.org/r/46067/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46067/

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

https://reviewboard.mozilla.org/r/46065/#review42621

r=me with issues addressed.

::: dom/media/AudioConverter.cpp:294
(Diff revision 1)
> +  MOZ_ASSERT(mIn.Channels() < mOut.Channels());
> +  MOZ_ASSERT(mIn.Channels() == 1, "Can only upmix mono for now");
> +  MOZ_ASSERT(mOut.Channels() == 2, "Can only upmix to stereo for now");
> +
> +  if (mOut.Channels() == 2) {
> +    static const float m3db = 0.7071067811865476f; // -3dB = SQRT(1/2)

You already asserted this. Either return immediately, or use a MOZ_RELEASE_ASSERT() above.

::: dom/media/AudioConverter.cpp:295
(Diff revision 1)
> +  MOZ_ASSERT(mIn.Channels() == 1, "Can only upmix mono for now");
> +  MOZ_ASSERT(mOut.Channels() == 2, "Can only upmix to stereo for now");
> +
> +  if (mOut.Channels() == 2) {
> +    static const float m3db = 0.7071067811865476f; // -3dB = SQRT(1/2)
> +    // This is a very dumb mono to stereo upmixing, power levels are preserved

You can just `const float m3db = std::sqrt(0.5f);` and the compiler will evaluate it at the build time. Easier to understand and gets you the correct number of digits. :)

::: dom/media/AudioConverter.cpp:300
(Diff revision 1)
> +    // This is a very dumb mono to stereo upmixing, power levels are preserved
> +    // following the calculation: left = right = -3dB*mono.
> +    if (mIn.Format() == AudioConfig::FORMAT_FLT) {
> +      const float* in = static_cast<const float*>(aIn);
> +      float* out = static_cast<float*>(aOut);
> +      for (uint32_t fIdx = 0; fIdx < aFrames; ++fIdx) {

`auto` or `size_t` fIdx.

::: dom/media/AudioConverter.cpp:311
(Diff revision 1)
> +    } else if (mIn.Format() == AudioConfig::FORMAT_S16) {
> +      const int16_t* in = static_cast<const int16_t*>(aIn);
> +      int16_t* out = static_cast<int16_t*>(aOut);
> +      for (uint32_t fIdx = 0; fIdx < aFrames; ++fIdx) {
> +        int16_t sample = in[fIdx] * m3db;
> +        // The samples of the buffer would be interleaved.

Doesn't this convert to float and back again every sample? Would be much faster to convert m3db to fixed point outside the loop and mul+shift.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Ralph Giles (:rillian) from comment #6)
> Comment on attachment 8740882 [details]
> MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to
> AudioConverter. r?rillian
> 
> https://reviewboard.mozilla.org/r/46065/#review42621
> 
> r=me with issues addressed.
> 
> ::: dom/media/AudioConverter.cpp:294
> (Diff revision 1)
> > +  MOZ_ASSERT(mIn.Channels() < mOut.Channels());
> > +  MOZ_ASSERT(mIn.Channels() == 1, "Can only upmix mono for now");
> > +  MOZ_ASSERT(mOut.Channels() == 2, "Can only upmix to stereo for now");
> > +
> > +  if (mOut.Channels() == 2) {
> > +    static const float m3db = 0.7071067811865476f; // -3dB = SQRT(1/2)
> 
> You already asserted this. Either return immediately, or use a
> MOZ_RELEASE_ASSERT() above.
> 
> ::: dom/media/AudioConverter.cpp:295
> (Diff revision 1)
> > +  MOZ_ASSERT(mIn.Channels() == 1, "Can only upmix mono for now");
> > +  MOZ_ASSERT(mOut.Channels() == 2, "Can only upmix to stereo for now");
> > +
> > +  if (mOut.Channels() == 2) {
> > +    static const float m3db = 0.7071067811865476f; // -3dB = SQRT(1/2)
> > +    // This is a very dumb mono to stereo upmixing, power levels are preserved
> 
> You can just `const float m3db = std::sqrt(0.5f);` and the compiler will
> evaluate it at the build time. Easier to understand and gets you the correct
> number of digits. :)
> 
> ::: dom/media/AudioConverter.cpp:300
> (Diff revision 1)
> > +    // This is a very dumb mono to stereo upmixing, power levels are preserved
> > +    // following the calculation: left = right = -3dB*mono.
> > +    if (mIn.Format() == AudioConfig::FORMAT_FLT) {
> > +      const float* in = static_cast<const float*>(aIn);
> > +      float* out = static_cast<float*>(aOut);
> > +      for (uint32_t fIdx = 0; fIdx < aFrames; ++fIdx) {
> 
> `auto` or `size_t` fIdx.
> 
> ::: dom/media/AudioConverter.cpp:311
> (Diff revision 1)
> > +    } else if (mIn.Format() == AudioConfig::FORMAT_S16) {
> > +      const int16_t* in = static_cast<const int16_t*>(aIn);
> > +      int16_t* out = static_cast<int16_t*>(aOut);
> > +      for (uint32_t fIdx = 0; fIdx < aFrames; ++fIdx) {
> > +        int16_t sample = in[fIdx] * m3db;
> > +        // The samples of the buffer would be interleaved.
> 
> Doesn't this convert to float and back again every sample? Would be much
> faster to convert m3db to fixed point outside the loop and mul+shift.

And how do you plan to multiply by sqrt(1/2) in fixed point? :)

Can't see how you can do without good through float, and I'm fairly certain doing it via integer (require a multiplicand then division) will be slower than going through float regardless of the platform.

The downmix above (which doesn't preserve power level btw as it reduce each channel by -6dB) also goes through float.

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Ralph Giles (:rillian) from comment #6)


> You can just `const float m3db = std::sqrt(0.5f);` and the compiler will
> evaluate it at the build time. Easier to understand and gets you the correct
> number of digits. :)

only gcc appears to do that at default compiler option.
https://godbolt.org/g/qoOBXU

clang load of 0.5 then call sqrt function

Interesting...

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to Ralph Giles (:rillian) from comment #6)
> 
> Doesn't this convert to float and back again every sample? Would be much
> faster to convert m3db to fixed point outside the loop and mul+shift.

Now that is interesting.
Intel:
using float:
https://godbolt.org/g/XiRX5k

using int16_t (extending to 32 bits integer), I used timing from cortex 4 ARM doc.
https://godbolt.org/g/UaAN96

Much better to use int.

However, on arm:
float: https://godbolt.org/g/34vUWS
int: https://godbolt.org/g/lZiuhy

the benefit aren't that obvious. The generated code is smaller using float.
so it becomes a matter of cycles per instructions.

looking at the integer code:
        ldrh    r4, [r0, r2, lsl #1]  <- 2 cycles
        adds    r3, r3, #1  <- 1
        cmp     r3, r1  <- 1
        smulbb  r4, r4, r7 <- 1
        smull   lr, r5, r6, r4 <- 1
        asr     r4, r4, #31 <- 1
        rsb     r4, r4, r5, asr #14 <- 1
        strh    r4, [r0, r2, lsl #1]    @ movhi <- 2
        mov     r2, r3 <- 1

total for an iteration: 11 cycles

using floats:
        ldrsh   r4, [r0, r2, lsl #1] <- 2
        adds    r3, r3, #1 <- 1
        cmp     r3, r1 <- 1
        fmsr    s15, r4 @ int <- 1 cycle (FPU register load)
        fsitos  s15, s15 <- 1 cycle (vfp11 coprocessor)
        fmuls   s15, s15, s14 <- 1 cycle on vfp11, and 10 cycles on vfp3!)
        ftosizs s15, s15 <- 1 cycle
        fmrs    r4, s15 @ int <- 1
        strh    r4, [r0, r2, lsl #1]    @ movhi <- 2
        mov     r2, r3 <- 1

so on ARM11 it's identical to using integer, on older vfp3 (cortex3) it's 9 cycles slower.
The S16 code path is only used on ARM (android and B2G)

I like the code with the float better tbh, it's easier to read

Comment 10

[Jean-Yves Avenard [:jya]]

doh, I should have read your suggestion closer.
Of course: int16_t sample = ((int32_t)in[fIdx] * 11585) >> 14;

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

https://reviewboard.mozilla.org/r/46061/#review42815

Comment 12

[Matthew Gregan [:kinetik]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

https://reviewboard.mozilla.org/r/46063/#review42817

::: dom/media/AudioStream.h:291
(Diff revision 1)
>    // Returns true when the audio stream is paused.
>    bool IsPaused();
>  
> +  static uint32_t GetPreferredRate()
> +  {
> +    return CubebUtils::PreferredSampleRate();

Need to call CubebUtils::InitPreferredSampleRate as well.

::: dom/media/mediasink/DecodedAudioDataSink.cpp:52
(Diff revision 1)
>  {
>    bool resampling = gfxPrefs::AudioSinkResampling();
> -  uint32_t resamplingRate = gfxPrefs::AudioSinkResampleRate();
> -  mOutputRate = resampling ? resamplingRate : mInfo.mRate;
> +
> +  if (resampling) {
> +    mOutputRate = gfxPrefs::AudioSinkResampleRate();
> +  } else if (mInfo.mRate == 44100 || mInfo.mRate == 48000) {

I think I'd just take GetPreferredRate here without the special case, but maybe it's not a big deal.

Comment 13

[Matthew Gregan [:kinetik]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

https://reviewboard.mozilla.org/r/46067/#review42819

::: dom/media/AudioStream.cpp:549
(Diff revision 1)
>    MonitorAutoLock mon(mMonitor);
>    return mState == STOPPED;
>  }
>  
>  bool
> -AudioStream::Downmix(Chunk* aChunk)
> +AudioStream::CheckAudio(Chunk* aChunk)

Maybe call this IsValidAudioFormat or something?

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

https://reviewboard.mozilla.org/r/46059/#review42821

::: dom/media/mediasink/DecodedAudioDataSink.cpp:333
(Diff revision 1)
> +  }
> +  if (!mProcessingThread->IsCurrentThreadIn()) {
> +    nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethod(
> +      this, &DecodedAudioDataSink::NotifyAudioNeeded);
> +    mProcessingThread->Dispatch(r.forget());
> +  }

Missing return?

Comment 15

[Jean-Yves Avenard [:jya]]

Thanks for the reviews. Good spotting for the missing return. Thank you!

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review commit: https://reviewboard.mozilla.org/r/46317/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46317/

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46059/diff/1-2/

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46061/diff/1-2/

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46063/diff/1-2/

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46065/diff/1-2/

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46067/diff/1-2/

Comment 22

[Matthew Gregan [:kinetik]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

https://reviewboard.mozilla.org/r/46317/#review42883

I don't know the speex resampler well enough to be sure this is right, so it'd be best to double check with padenot or someone else with more experience.

::: dom/media/AudioConverter.h:166
(Diff revision 1)
>  
>      // At this point, temp1 contains the buffer reordered and downmixed.
>      // If we are downsampling we can re-use it.
>      AlignedBuffer<Value>* outputBuffer = &temp1;
>      AlignedBuffer<Value> temp2;
> -    if (mOut.Rate() > mIn.Rate()) {
> +    if (!frame || mOut.Rate() > mIn.Rate()) {

frames? There's no "frame" that I can find...

Comment 23

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P7. Reduce likelihood of integer overflow. r?gerald

If frame start time was very high (like seen with some BBC videos) it could easily overflow. Now we can only overflow after playing 2^63 / sampling_rate microseconds of audio (~6 years @ 48kHz)

Review commit: https://reviewboard.mozilla.org/r/46337/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46337/

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46317/diff/1-2/

Comment 25

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8741252 [details]
MozReview Request: Bug 1264199: P7. Reduce likelihood of integer overflow. r?gerald

https://reviewboard.mozilla.org/r/46337/#review42909

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46059/diff/2-3/

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46061/diff/2-3/

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46063/diff/2-3/

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46065/diff/2-3/

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46067/diff/2-3/

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46317/diff/2-3/

Comment 32

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/46059/#review43589

I've reworked the stack so that most of the work is in P1.

There are two major changes with the original patch stack that was reviewed:
1- Silence detection was moved from the cubeb callback to the audio processing task. This is because as the speex resampler introduce a latency, we can't use the original AudioData timestamp as it would insert silence between packets.
2- We can't rely on the number of frames written to the AudioStream to determine the current position. AAC packets are made of 1024 frames. At 44.1kHz, that means that the duration of a packet is 23219.95464853us. However, as store them with int64_t that becomes 23219us. the rounding errors caused silent frames to always be inserted which caused displeasing listening.

So I moved back to using a time using a frame unit. We just have to recalculate whenever changing sampling rate so the position stays accurate.

Comment 33

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/46059/#review42821

> Missing return?

good catch !

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b15c10c42148
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b82e89a490e
https://hg.mozilla.org/integration/mozilla-inbound/rev/04751f3545ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/1086b48deaa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f323c504b5a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e02fc1cbd35

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d139b7422c8

Comment 36

[Carsten Book [:Tomcat]]

sorry had to back this out since we still crash on android like https://treeherder.mozilla.org/logviewer.html#?job_id=26013631&repo=mozilla-inbound

Comment 37

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a0ad4f6ac0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9390125f842
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab22d41a450
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7f3dc00a17
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70d680844d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f76601f135
https://hg.mozilla.org/integration/mozilla-inbound/rev/a777bd20ea00

Comment 38

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik

Fix submitted upstream.

Review commit: https://reviewboard.mozilla.org/r/47385/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47385/

Comment 39

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik

The speex resampler can never return an error in its current state. But just in case and to handle any future changes to the speex resampler.
Also ensure that we can never access a stale speex resampler.

Review commit: https://reviewboard.mozilla.org/r/47387/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47387/

Comment 40

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46059/diff/3-4/

Comment 41

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46061/diff/3-4/

Comment 42

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46063/diff/3-4/

Comment 43

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46065/diff/3-4/

Comment 44

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46067/diff/3-4/

Comment 45

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46317/diff/3-4/

Comment 46

[Matthew Gregan [:kinetik]]

Comment on attachment 8742641 [details]
MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik

https://reviewboard.mozilla.org/r/47385/#review44087

These look OK (assuming upstream agrees), but until the changes go upstream and back into the Gecko tree through that method, they need to be applied as a separate patch applied with update.sh like media/libspeex_resampler/set-skip-frac.patch and the others.

Comment 47

[Matthew Gregan [:kinetik]]

Comment on attachment 8742642 [details]
MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik

https://reviewboard.mozilla.org/r/47387/#review44091

Comment 48

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46059/diff/4-5/

Comment 49

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46061/diff/4-5/

Comment 50

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46063/diff/4-5/

Comment 51

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46065/diff/4-5/

Comment 52

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46067/diff/4-5/

Comment 53

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46317/diff/4-5/

Comment 54

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742641 [details]
MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47385/diff/1-2/

Comment 55

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742642 [details]
MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47387/diff/1-2/

Comment 56

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/46059/#review44365

I have removed for now the multi-threaded side of things and instead now work in the MDSM taskqueue. Concurrent access on the AudioQueue is just not possible in the current architecture without a synchronous step which I didn't want to add. I will rework things in bug 1265932

Comment 57

[Matthew Gregan [:kinetik]]

Comment on attachment 8742641 [details]
MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik

https://reviewboard.mozilla.org/r/47385/#review44367

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d08288654ec9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3677ae4c8ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/02f781a573b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f587df589b69
https://hg.mozilla.org/integration/mozilla-inbound/rev/b79b62146120
https://hg.mozilla.org/integration/mozilla-inbound/rev/f484b42cbdda
https://hg.mozilla.org/integration/mozilla-inbound/rev/90351d2719be
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f20f2080824

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d08288654ec9
https://hg.mozilla.org/mozilla-central/rev/d3677ae4c8ec
https://hg.mozilla.org/mozilla-central/rev/02f781a573b4
https://hg.mozilla.org/mozilla-central/rev/f587df589b69
https://hg.mozilla.org/mozilla-central/rev/b79b62146120
https://hg.mozilla.org/mozilla-central/rev/f484b42cbdda
https://hg.mozilla.org/mozilla-central/rev/90351d2719be
https://hg.mozilla.org/mozilla-central/rev/0f20f2080824

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/30c5dbcee7dd for apparently making media mochitests on OSX 10.6 permafail like https://treeherder.mozilla.org/logviewer.html#?job_id=26185066&repo=mozilla-inbound

Comment 61

[Jean-Yves Avenard [:jya]]

not permafailing here, more retries show that this mochitest is just as intermittent.

Comment 63

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46059/diff/5-6/

Comment 64

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46061/diff/5-6/

Comment 65

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46063/diff/5-6/

Comment 66

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46065/diff/5-6/

Comment 67

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46067/diff/5-6/

Comment 68

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46317/diff/5-6/

Comment 69

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742641 [details]
MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47385/diff/2-3/

Comment 70

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742642 [details]
MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47387/diff/2-3/

Comment 71

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/198add1ad218
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01ac4659cb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/697006992f04
https://hg.mozilla.org/integration/mozilla-inbound/rev/22004be7c5f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/93ab5d1b04b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83be2d0614b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9bfe66b235c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b52399a4a175

Comment 72

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=26300432&repo=mozilla-inbound

Comment 73

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c845a225f1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ebd803604bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/95384829747b
https://hg.mozilla.org/integration/mozilla-inbound/rev/161e9333c85d
https://hg.mozilla.org/integration/mozilla-inbound/rev/807c2d68da28
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8ff02690b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ea6efa54bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/acaefb4a5fc8

Comment 74

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P0. Fix nsDequeue/MediaQueue methods constness. r?jwwang,r?froydn

Review commit: https://reviewboard.mozilla.org/r/48699/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48699/

Comment 75

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46059/diff/6-7/

Comment 76

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46061/diff/6-7/

Comment 77

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46063/diff/6-7/

Comment 78

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46065/diff/6-7/

Comment 79

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46067/diff/6-7/

Comment 80

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46317/diff/6-7/

Comment 81

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742641 [details]
MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47385/diff/3-4/

Comment 82

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742642 [details]
MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47387/diff/3-4/

Comment 83

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8744837 [details]
MozReview Request: Bug 1264199: P0. Fix nsDequeue/MediaQueue methods constness. r?jwwang,r?froydn

https://reviewboard.mozilla.org/r/48699/#review45487

Comment 84

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P0.1. Export SaferMultDiv method. r?gerald

Review commit: https://reviewboard.mozilla.org/r/48879/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48879/

Comment 85

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P9. Include pending frames in HasUnplayedFrames calculation. r?jwwang

Review commit: https://reviewboard.mozilla.org/r/48881/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48881/

Comment 86

[Jean-Yves Avenard [:jya]]

Comment on attachment 8744837 [details]
MozReview Request: Bug 1264199: P0. Fix nsDequeue/MediaQueue methods constness. r?jwwang,r?froydn

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48699/diff/1-2/

Comment 87

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46059/diff/7-8/

Comment 88

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46061/diff/7-8/

Comment 89

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46063/diff/7-8/

Comment 90

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46065/diff/7-8/

Comment 91

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46067/diff/7-8/

Comment 92

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46317/diff/7-8/

Comment 93

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742641 [details]
MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47385/diff/4-5/

Comment 94

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742642 [details]
MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47387/diff/4-5/

Comment 97

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8745160 [details]
MozReview Request: Bug 1264199: P0.1. Export SaferMultDiv method. r?gerald

https://reviewboard.mozilla.org/r/48879/#review45703

::: dom/media/VideoUtils.h:134
(Diff revision 1)
> +// Perform aValue * aMul / aDiv, removing the possibility of overflow due to
> +// aValue * aMul overflowing.

I don't think you can claim to "remove" the overflow, just reduce it to a minimum. Just imagine SaferMultDiv(max64, 2, 1).

In fact, looking at the code in VideoUtils.cpp, it could go unchecked!
> int64_t major = aValue / aDiv;
> int64_t remainder = aValue % aDiv;
> return CheckedInt64(remainder) * aMul / aDiv + major * aMul;

'major*aMul' is not checked for overflow before adding it to the CheckedInt64.

Comment 98

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1264199: P10. renable test now that core issue likely fixed. r?gerald

Reversal of change from bug 1258922.

Review commit: https://reviewboard.mozilla.org/r/48887/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48887/

Comment 99

[Jean-Yves Avenard [:jya]]

Comment on attachment 8745160 [details]
MozReview Request: Bug 1264199: P0.1. Export SaferMultDiv method. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48879/diff/1-2/

Comment 100

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740879 [details]
MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46059/diff/8-9/

Comment 101

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740880 [details]
MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46061/diff/8-9/

Comment 102

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740881 [details]
MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46063/diff/8-9/

Comment 103

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740882 [details]
MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46065/diff/8-9/

Comment 104

[Jean-Yves Avenard [:jya]]

Comment on attachment 8740883 [details]
MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46067/diff/8-9/

Comment 105

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741233 [details]
MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46317/diff/8-9/

Comment 106

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742641 [details]
MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47385/diff/5-6/

Comment 107

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742642 [details]
MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47387/diff/5-6/

Comment 108

[Jean-Yves Avenard [:jya]]

Comment on attachment 8745161 [details]
MozReview Request: Bug 1264199: P9. Include pending frames in HasUnplayedFrames calculation. r?jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48881/diff/1-2/

Comment 109

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8745160 [details]
MozReview Request: Bug 1264199: P0.1. Export SaferMultDiv method. r?gerald

https://reviewboard.mozilla.org/r/48879/#review45713

Comment 110

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/48879/#review45703

> I don't think you can claim to "remove" the overflow, just reduce it to a minimum. Just imagine SaferMultDiv(max64, 2, 1).
> 
> In fact, looking at the code in VideoUtils.cpp, it could go unchecked!
> > int64_t major = aValue / aDiv;
> > int64_t remainder = aValue % aDiv;
> > return CheckedInt64(remainder) * aMul / aDiv + major * aMul;
> 
> 'major*aMul' is not checked for overflow before adding it to the CheckedInt64.

good spotting ! fixed in 2nd revision

Comment 111

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8745165 [details]
MozReview Request: Bug 1264199: P10. renable test now that core issue likely fixed. r?gerald

https://reviewboard.mozilla.org/r/48887/#review45717

Comment 112

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8745161 [details]
MozReview Request: Bug 1264199: P9. Include pending frames in HasUnplayedFrames calculation. r?jwwang

https://reviewboard.mozilla.org/r/48881/#review45729

Comment 113

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f338dbd14ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e1803a031bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d9af3d9ceb
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c5d4e0779b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c92dc4fbb2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/314f54c31a55
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca918dcf2dfc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1435aa2cf1ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/14e0996c0dc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/1139388df304
https://hg.mozilla.org/integration/mozilla-inbound/rev/917bec2031e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/92d6662bdf4f

Comment 114

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f338dbd14ff
https://hg.mozilla.org/mozilla-central/rev/0e1803a031bc
https://hg.mozilla.org/mozilla-central/rev/c1d9af3d9ceb
https://hg.mozilla.org/mozilla-central/rev/d6c5d4e0779b
https://hg.mozilla.org/mozilla-central/rev/1c92dc4fbb2b
https://hg.mozilla.org/mozilla-central/rev/314f54c31a55
https://hg.mozilla.org/mozilla-central/rev/ca918dcf2dfc
https://hg.mozilla.org/mozilla-central/rev/1435aa2cf1ca
https://hg.mozilla.org/mozilla-central/rev/14e0996c0dc5
https://hg.mozilla.org/mozilla-central/rev/1139388df304
https://hg.mozilla.org/mozilla-central/rev/917bec2031e2
https://hg.mozilla.org/mozilla-central/rev/92d6662bdf4f