Closed Bug 1262753 Opened 4 years ago Closed 4 years ago

Add resampling capabilities to AudioConverter

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(8 files)

58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
kinetik
: review+
Details
58 bytes, text/x-review-board-request
kinetik
: review+
Details
58 bytes, text/x-review-board-request
kinetik
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
58 bytes, text/x-review-board-request
gerald
: review+
Details
To track the task of adding resampling capabilities to the AudioConverter
Review commit: https://reviewboard.mozilla.org/r/45667/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45667/
Attachment #8740239 - Flags: review?(gsquelart)
Attachment #8740240 - Flags: review?(gsquelart)
Attachment #8740241 - Flags: review?(gsquelart)
Attachment #8740242 - Flags: review?(kinetik)
Attachment #8740243 - Flags: review?(gsquelart)
The Process API was originally planned to be used to also convert data type. However, this task is now the responsibility of the AudioDataBuffer class.
We can simplify the logic and use frames everywhere.

Review commit: https://reviewboard.mozilla.org/r/45675/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45675/
Comment on attachment 8740239 [details]
MozReview Request: Bug 1262753: P1. Add AudioConfig == and != operator. r?gerald

https://reviewboard.mozilla.org/r/45667/#review42199

r+ with fix, and optional change of implementation.

::: dom/media/MediaInfo.h:609
(Diff revision 1)
> +  bool operator!=(const AudioConfig& aOther) const
> +  {
> +    return mChannelLayout != aOther.mChannelLayout ||
> +      mRate != aOther.mRate || mFormat == aOther.mFormat ||
> +      mInterleaved != aOther.mInterleaved;

`mFormat == aOther.mFormat` should use `!=` instead.

I've seen operator!=() traditionally implemented by just calling operator== (e.g. `return !(*this == aOther);`)
It's safer to code, and easier to maintain both operators if members change.
Attachment #8740239 - Flags: review?(gsquelart) → review+
Attachment #8740240 - Flags: review?(gsquelart) → review+
Comment on attachment 8740240 [details]
MozReview Request: Bug 1262753: P2. Add AudioDataBuffer::operator=. r?gerald

https://reviewboard.mozilla.org/r/45669/#review42201

r+ assuming it compiles & works, but please consider these:

::: dom/media/AudioConverter.h:90
(Diff revision 1)
> +  AudioDataBuffer& operator=(AudioDataBuffer&& aOther)
> +  {
> +    this->~AudioDataBuffer();
> +    new (this) AudioDataBuffer(Move(aOther));

That's cute, but couldn't you just do a simpler `mBuffer = Move(aOther.mBuffer)`?

::: dom/media/AudioConverter.h:96
(Diff revision 1)
> +  AudioDataBuffer& operator=(const AudioDataBuffer& aOther)
> +  {
> +    mBuffer = aOther;

I don't understand how this assignment (from an AudioDataBuffer to an AlignedBuffer) can work.
Shouldn't it be `mBuffer = aOther.mBuffer`?
Comment on attachment 8740241 [details]
MozReview Request: Bug 1262753: P3. Add AudioConverter::InputConfig/OutputConfig. r?gerald

https://reviewboard.mozilla.org/r/45671/#review42205

In your commit description: "Allow to avoid storing the original AudioConfig objects."
Do you mean "Allow access to avoid having to store the original AudioConfig objects elsewhere."?
Attachment #8740241 - Flags: review?(gsquelart) → review+
Using the gfxprefs for the time being, in order to access the preferences outside of the main thread.

It will allow to easily test future cubeb changes to support change of sampling rate / multichannels.

Review commit: https://reviewboard.mozilla.org/r/45729/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45729/
Attachment #8740243 - Attachment description: MozReview Request: Bug 1262753: P5. Pass number of frames rather than the number of bytes. r?gerald → MozReview Request: Bug 1262753: P5. Perform downmixing in DecodeAudioDataSink. r?kinetik
Attachment #8740342 - Flags: review?(kinetik)
Attachment #8740343 - Flags: review?(gsquelart)
Attachment #8740243 - Flags: review?(gsquelart) → review?(kinetik)
Comment on attachment 8740239 [details]
MozReview Request: Bug 1262753: P1. Add AudioConfig == and != operator. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45667/diff/1-2/
Comment on attachment 8740240 [details]
MozReview Request: Bug 1262753: P2. Add AudioDataBuffer::operator=. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45669/diff/1-2/
Comment on attachment 8740241 [details]
MozReview Request: Bug 1262753: P3. Add AudioConverter::InputConfig/OutputConfig. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45671/diff/1-2/
Comment on attachment 8740242 [details]
MozReview Request: Bug 1262753: P4. Add resampling capabilities to AudioConverter. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45673/diff/1-2/
Comment on attachment 8740243 [details]
MozReview Request: Bug 1262753: P6. Perform downmixing in DecodeAudioDataSink. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45675/diff/1-2/
The Process API was originally planned to be used to also convert data type. However, this task is now the responsibility of the AudioDataBuffer class.
We can simplify the logic and use frames everywhere.

Review commit: https://reviewboard.mozilla.org/r/45733/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45733/
Attachment #8740243 - Attachment description: MozReview Request: Bug 1262753: P5. Perform downmixing in DecodeAudioDataSink. r?kinetik → MozReview Request: Bug 1262753: P6. Perform downmixing in DecodeAudioDataSink. r?kinetik
Attachment #8740342 - Attachment description: MozReview Request: Bug 1262753: P6. Add debugging pref to enable/disable downmixer and resample. r?kinetik → MozReview Request: Bug 1262753: P7. Add debugging pref to enable/disable downmixer and resample. r?kinetik
Attachment #8740343 - Attachment description: MozReview Request: Bug 1262753: P7. Remove useless assert. r?gerald → MozReview Request: Bug 1262753: P8. Remove useless assert. r?gerald
Attachment #8740351 - Flags: review?(gsquelart)
Comment on attachment 8740242 [details]
MozReview Request: Bug 1262753: P4. Add resampling capabilities to AudioConverter. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45673/diff/2-3/
Comment on attachment 8740243 [details]
MozReview Request: Bug 1262753: P6. Perform downmixing in DecodeAudioDataSink. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45675/diff/2-3/
Comment on attachment 8740342 [details]
MozReview Request: Bug 1262753: P7. Add debugging pref to enable/disable downmixer and resample. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45729/diff/1-2/
Comment on attachment 8740343 [details]
MozReview Request: Bug 1262753: P8. Remove useless assert. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45731/diff/1-2/
sorry for the noise. during rebasing two commits got squashed together.. had to resplit them
Comment on attachment 8740343 [details]
MozReview Request: Bug 1262753: P8. Remove useless assert. r?gerald

https://reviewboard.mozilla.org/r/45731/#review42285
Attachment #8740343 - Flags: review?(gsquelart) → review+
Attachment #8740351 - Flags: review?(gsquelart)
Comment on attachment 8740351 [details]
MozReview Request: Bug 1262753: P5. Pass number of frames rather than the number of bytes. r?gerald

https://reviewboard.mozilla.org/r/45733/#review42289

r+ with optional nit:

::: dom/media/AudioConverter.cpp:262
(Diff revision 1)
> -AudioConverter::ResampleAudio(void* aOut, const void* aIn, size_t aDataSize)
> +AudioConverter::ResampleAudio(void* aOut, const void* aIn, size_t aFrames)
>  {
>    if (!mResampler) {
>      return 0;
>    }
> -  uint32_t frames =
> +  uint32_t outframes = (int64_t)aFrames * mOut.Rate() / mIn.Rate() + 1;

`int64_t` -> `uint64_t`, to keep everything unsigned.
Comment on attachment 8740351 [details]
MozReview Request: Bug 1262753: P5. Pass number of frames rather than the number of bytes. r?gerald

https://reviewboard.mozilla.org/r/45733/#review42291
Attachment #8740351 - Flags: review+
Comment on attachment 8740242 [details]
MozReview Request: Bug 1262753: P4. Add resampling capabilities to AudioConverter. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45673/diff/3-4/
Comment on attachment 8740351 [details]
MozReview Request: Bug 1262753: P5. Pass number of frames rather than the number of bytes. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45733/diff/1-2/
Comment on attachment 8740243 [details]
MozReview Request: Bug 1262753: P6. Perform downmixing in DecodeAudioDataSink. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45675/diff/3-4/
Comment on attachment 8740342 [details]
MozReview Request: Bug 1262753: P7. Add debugging pref to enable/disable downmixer and resample. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45729/diff/2-3/
Comment on attachment 8740343 [details]
MozReview Request: Bug 1262753: P8. Remove useless assert. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45731/diff/2-3/
Attachment #8740243 - Flags: review?(kinetik) → review+
Comment on attachment 8740243 [details]
MozReview Request: Bug 1262753: P6. Perform downmixing in DecodeAudioDataSink. r?kinetik

https://reviewboard.mozilla.org/r/45675/#review42807
Attachment #8740342 - Flags: review?(kinetik) → review+
Comment on attachment 8740342 [details]
MozReview Request: Bug 1262753: P7. Add debugging pref to enable/disable downmixer and resample. r?kinetik

https://reviewboard.mozilla.org/r/45729/#review42811

::: gfx/thebes/gfxPrefs.h:140
(Diff revision 3)
>  
>    // It's a short time fix, and will be removed after landing bug 1206637.
>    DECL_GFX_PREF(Live, "accessibility.monoaudio.enable",        MonoAudio, bool, false);
> +  DECL_GFX_PREF(Live, "media.resampling.enabled",              AudioSinkResampling, bool, false);
> +  DECL_GFX_PREF(Live, "media.resampling.rate",                 AudioSinkResampleRate, uint32_t, 48000);
> +  DECL_GFX_PREF(Live, "media.forcestereo.enabled",             AudioSinkDownmixing, bool, true);

AudioSinkDownMixing might not be the best name if this (now or ever) also allows upmixing mono to stereo.
Attachment #8740242 - Flags: review?(kinetik) → review+
Comment on attachment 8740242 [details]
MozReview Request: Bug 1262753: P4. Add resampling capabilities to AudioConverter. r?kinetik

https://reviewboard.mozilla.org/r/45673/#review42805

::: dom/media/AudioConverter.cpp:43
(Diff revision 4)
> +                                      aOut.Rate(),
> +                                      SPEEX_RESAMPLER_QUALITY_DEFAULT,
> +                                      &error);
> +
> +    if (error == RESAMPLER_ERR_SUCCESS) {
> +      speex_resampler_skip_zeros(mResampler);

Do we also need to drain the resampler per bug 870174 comment 3/4 or is that unnecessary for some reason?

::: dom/media/AudioConverter.cpp:63
(Diff revision 4)
>  
>  bool
>  AudioConverter::CanWorkInPlace() const
>  {
> -  return mIn.Channels() * mIn.Rate() * AudioConfig::SampleSize(mIn.Format()) >=
> -    mOut.Channels() * mOut.Rate() * AudioConfig::SampleSize(mOut.Format());
> +  bool needDownmix = mIn.Channels() > mOut.Channels();
> +  bool canDowmixInPlace =

typo
(In reply to Matthew Gregan [:kinetik] from comment #32)
> Comment on attachment 8740242 [details]
> MozReview Request: Bug 1262753: P4. Add resampling capabilities to
> AudioConverter. r?kinetik
> 
> https://reviewboard.mozilla.org/r/45673/#review42805
> 
> ::: dom/media/AudioConverter.cpp:43
> (Diff revision 4)
> > +                                      aOut.Rate(),
> > +                                      SPEEX_RESAMPLER_QUALITY_DEFAULT,
> > +                                      &error);
> > +
> > +    if (error == RESAMPLER_ERR_SUCCESS) {
> > +      speex_resampler_skip_zeros(mResampler);
> 
> Do we also need to drain the resampler per bug 870174 comment 3/4 or is that
> unnecessary for some reason?
> 

I copied that from somewhere else tbh, now as you mention it, it probably serves no purpose whatsoever.
And should probably handle the error differently, like testing that mResampler isn't null when we do need to resample.
https://reviewboard.mozilla.org/r/45729/#review42811

> AudioSinkDownMixing might not be the best name if this (now or ever) also allows upmixing mono to stereo.

renamed AudioSinkForceStereo
Comment on attachment 8740239 [details]
MozReview Request: Bug 1262753: P1. Add AudioConfig == and != operator. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45667/diff/2-3/
Comment on attachment 8740240 [details]
MozReview Request: Bug 1262753: P2. Add AudioDataBuffer::operator=. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45669/diff/2-3/
Comment on attachment 8740241 [details]
MozReview Request: Bug 1262753: P3. Add AudioConverter::InputConfig/OutputConfig. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45671/diff/2-3/
Comment on attachment 8740242 [details]
MozReview Request: Bug 1262753: P4. Add resampling capabilities to AudioConverter. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45673/diff/4-5/
Comment on attachment 8740351 [details]
MozReview Request: Bug 1262753: P5. Pass number of frames rather than the number of bytes. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45733/diff/2-3/
Comment on attachment 8740243 [details]
MozReview Request: Bug 1262753: P6. Perform downmixing in DecodeAudioDataSink. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45675/diff/4-5/
Comment on attachment 8740342 [details]
MozReview Request: Bug 1262753: P7. Add debugging pref to enable/disable downmixer and resample. r?kinetik

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45729/diff/3-4/
Comment on attachment 8740343 [details]
MozReview Request: Bug 1262753: P8. Remove useless assert. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45731/diff/3-4/
You need to log in before you can comment on or make changes to this bug.