Allow mechanism to change audio configuration mid-stream

RESOLVED FIXED in Firefox 49

Status

()

Core
Audio/Video: cubeb
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(12 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
kinetik
: review+
Details | Review
58 bytes, text/x-review-board-request
kinetik
: review+
Details | Review
58 bytes, text/x-review-board-request
kinetik
: review+
Details | Review
58 bytes, text/x-review-board-request
rillian
: review+
Details | Review
58 bytes, text/x-review-board-request
kinetik
: review+
Details | Review
58 bytes, text/x-review-board-request
kinetik
: review+
Details | Review
58 bytes, text/x-review-board-request
kinetik
: review+
Details | Review
58 bytes, text/x-review-board-request
kinetik
: review+
Details | Review
58 bytes, text/x-review-board-request
jwwang
: review+
Details | Review
58 bytes, text/x-review-board-request
gerald
: review+
Details | Review
58 bytes, text/x-review-board-request
jwwang
: review+
Details | Review
58 bytes, text/x-review-board-request
gerald
: review+
Details | Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1264200
(Assignee)

Comment 1

2 years ago
Created attachment 8740879 [details]
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/
Attachment #8740879 - Flags: review?(kinetik)
Attachment #8740880 - Flags: review?(kinetik)
Attachment #8740881 - Flags: review?(kinetik)
Attachment #8740882 - Flags: review?(giles)
Attachment #8740883 - Flags: review?(kinetik)
(Assignee)

Comment 2

2 years ago
Created attachment 8740880 [details]
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/
(Assignee)

Comment 3

2 years ago
Created attachment 8740881 [details]
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/
(Assignee)

Comment 4

2 years ago
Created attachment 8740882 [details]
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/
(Assignee)

Comment 5

2 years ago
Created attachment 8740883 [details]
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 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.
Attachment #8740882 - Flags: review?(giles) → review+
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
(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...
(Assignee)

Comment 9

2 years ago
(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
(Assignee)

Comment 10

2 years ago
doh, I should have read your suggestion closer.
Of course: int16_t sample = ((int32_t)in[fIdx] * 11585) >> 14;
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
Attachment #8740880 - Flags: review?(kinetik) → review+
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.
Attachment #8740881 - Flags: review?(kinetik) → review+
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?
Attachment #8740883 - Flags: review?(kinetik) → review+
Attachment #8740879 - Flags: review?(kinetik) → review+
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?
(Assignee)

Comment 15

2 years ago
Thanks for the reviews. Good spotting for the missing return. Thank you!
(Assignee)

Comment 16

2 years ago
Created attachment 8741233 [details]
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/
Attachment #8741233 - Flags: review?(kinetik)
(Assignee)

Comment 17

2 years ago
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/
(Assignee)

Comment 18

2 years ago
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/
(Assignee)

Comment 19

2 years ago
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/
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
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/
Attachment #8741233 - Flags: review?(kinetik) → review+
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...
(Assignee)

Comment 23

2 years ago
Created attachment 8741252 [details]
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/
Attachment #8741252 - Flags: review?(gsquelart)
(Assignee)

Comment 24

2 years ago
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/
Attachment #8741252 - Flags: review?(gsquelart) → review+
Comment on attachment 8741252 [details]
MozReview Request: Bug 1264199: P7. Reduce likelihood of integer overflow. r?gerald

https://reviewboard.mozilla.org/r/46337/#review42909
Assignee: nobody → jyavenard
(Assignee)

Comment 26

2 years ago
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/
(Assignee)

Comment 27

2 years ago
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/
(Assignee)

Comment 28

2 years ago
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/
(Assignee)

Comment 29

2 years ago
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/
(Assignee)

Comment 30

2 years ago
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/
(Assignee)

Comment 31

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8741252 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
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 hidden (obsolete)
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
Flags: needinfo?(jyavenard)
(Assignee)

Comment 38

2 years ago
Created attachment 8742641 [details]
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/
Attachment #8742641 - Flags: review?(kinetik)
Attachment #8742642 - Flags: review?(kinetik)
(Assignee)

Comment 39

2 years ago
Created attachment 8742642 [details]
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/
(Assignee)

Comment 40

2 years ago
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/
(Assignee)

Comment 41

2 years ago
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/
(Assignee)

Comment 42

2 years ago
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/
(Assignee)

Comment 43

2 years ago
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/
(Assignee)

Comment 44

2 years ago
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/
(Assignee)

Comment 45

2 years ago
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/
Attachment #8742641 - Flags: review?(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 on attachment 8742642 [details]
MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik

https://reviewboard.mozilla.org/r/47387/#review44091
Attachment #8742642 - Flags: review?(kinetik) → review+
(Assignee)

Updated

2 years ago
Blocks: 1265932
(Assignee)

Updated

2 years ago
Depends on: 1246051
(Assignee)

Comment 48

2 years ago
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/
Attachment #8742641 - Flags: review?(kinetik)
(Assignee)

Comment 49

2 years ago
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/
(Assignee)

Comment 50

2 years ago
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/
(Assignee)

Comment 51

2 years ago
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/
(Assignee)

Comment 52

2 years ago
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/
(Assignee)

Comment 53

2 years ago
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/
(Assignee)

Comment 54

2 years ago
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/
(Assignee)

Comment 55

2 years ago
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/
(Assignee)

Comment 56

2 years ago
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
Attachment #8742641 - Flags: review?(kinetik) → review+
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
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

2 years ago
status-firefox48: fixed → affected
(Assignee)

Comment 61

2 years ago
not permafailing here, more retries show that this mochitest is just as intermittent.
Flags: needinfo?(jyavenard)

Updated

2 years ago
Depends on: 1266253
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1266253

Updated

2 years ago
No longer depends on: 1266253
(Assignee)

Comment 63

2 years ago
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/
Attachment #8740879 - Attachment description: MozReview Request: Bug 1264199: P1. Perform audio conversion in the reader's taskqueue and ahead of use. r?kinetik → MozReview Request: Bug 1264199: P1. Perform audio conversion in the reader's taskqueue and ahead of use. r=kinetik
Attachment #8740880 - Attachment description: MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r?kinetik → MozReview Request: Bug 1264199: P2. Ensure the AudioStream only ever receive the same content format. r=kinetik
Attachment #8740881 - Attachment description: MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r?kinetik → MozReview Request: Bug 1264199: P3. Attempt to minimize audio quality loss and unnecessary processing. r=kinetik
Attachment #8740882 - Attachment description: MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r?rillian → MozReview Request: Bug 1264199: P4. Add mono to stereo upmix to AudioConverter. r=rillian
Attachment #8740883 - Attachment description: MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r?kinetik → MozReview Request: Bug 1264199: P5. Perform all downmixing operations in DecodedAudioDataSink. r=kinetik
Attachment #8741233 - Attachment description: MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r?kinetik → MozReview Request: Bug 1264199: P6. Drain resampler when changing format or reaching the end. r=kinetik
Attachment #8742641 - Attachment description: MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r?kinetik → MozReview Request: Bug 1264199: [speex] P7. Handle memory allocation failures during initialization. r=kinetik
Attachment #8742642 - Attachment description: MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r?kinetik → MozReview Request: Bug 1264199: P8. Handle potential resampling errors. r=kinetik
(Assignee)

Comment 64

2 years ago
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/
(Assignee)

Comment 65

2 years ago
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/
(Assignee)

Comment 66

2 years ago
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/
(Assignee)

Comment 67

2 years ago
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/
(Assignee)

Comment 68

2 years ago
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/
(Assignee)

Comment 69

2 years ago
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/
(Assignee)

Comment 70

2 years ago
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/
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=26300432&repo=mozilla-inbound
Flags: needinfo?(jyavenard)
(Assignee)

Comment 74

2 years ago
Created attachment 8744837 [details]
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/
Attachment #8740879 - Attachment description: MozReview Request: Bug 1264199: P1. Perform audio conversion in the reader's taskqueue and ahead of use. r=kinetik → MozReview Request: Bug 1264199: P1. Perform audio conversion in the MSDM taskqueue and ahead of use. r=kinetik
Attachment #8744837 - Flags: review?(jwwang)
(Assignee)

Comment 75

2 years ago
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/
(Assignee)

Comment 76

2 years ago
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/
(Assignee)

Comment 77

2 years ago
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/
(Assignee)

Comment 78

2 years ago
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/
(Assignee)

Comment 79

2 years ago
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/
(Assignee)

Comment 80

2 years ago
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/
(Assignee)

Comment 81

2 years ago
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/
(Assignee)

Comment 82

2 years ago
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 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
Attachment #8744837 - Flags: review?(jwwang) → review+
(Assignee)

Comment 84

2 years ago
Created attachment 8745160 [details]
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/
Attachment #8745160 - Flags: review?(gsquelart)
Attachment #8745161 - Flags: review?(jwwang)
(Assignee)

Comment 85

2 years ago
Created attachment 8745161 [details]
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/
(Assignee)

Comment 86

2 years ago
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/
(Assignee)

Comment 87

2 years ago
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/
(Assignee)

Comment 88

2 years ago
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/
(Assignee)

Comment 89

2 years ago
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/
(Assignee)

Comment 90

2 years ago
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/
(Assignee)

Comment 91

2 years ago
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/
(Assignee)

Comment 92

2 years ago
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/
(Assignee)

Comment 93

2 years ago
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/
(Assignee)

Comment 94

2 years ago
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/
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1266555
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1258922
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.
Attachment #8745160 - Flags: review?(gsquelart)
(Assignee)

Comment 98

2 years ago
Created attachment 8745165 [details]
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/
Attachment #8745165 - Flags: review?(gsquelart)
Attachment #8745160 - Flags: review?(gsquelart)
(Assignee)

Comment 99

2 years ago
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/
(Assignee)

Comment 100

2 years ago
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/
(Assignee)

Comment 101

2 years ago
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/
(Assignee)

Comment 102

2 years ago
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/
(Assignee)

Comment 103

2 years ago
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/
(Assignee)

Comment 104

2 years ago
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/
(Assignee)

Comment 105

2 years ago
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/
(Assignee)

Comment 106

2 years ago
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/
(Assignee)

Comment 107

2 years ago
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/
(Assignee)

Comment 108

2 years ago
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 on attachment 8745160 [details]
MozReview Request: Bug 1264199: P0.1. Export SaferMultDiv method. r?gerald

https://reviewboard.mozilla.org/r/48879/#review45713
Attachment #8745160 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 110

2 years ago
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 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
Attachment #8745165 - Flags: review?(gsquelart) → review+
Attachment #8745161 - Flags: review?(jwwang) → review+
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
(Assignee)

Updated

2 years ago
Flags: needinfo?(jyavenard)
(Assignee)

Updated

2 years ago
Depends on: 1269672
Depends on: 1266260
(Assignee)

Updated

2 years ago
Depends on: 1305826
See Also: → bug 1386478
You need to log in before you can comment on or make changes to this bug.