1262753 - Add resampling capabilities to AudioConverter

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

To track the task of adding resampling capabilities to the AudioConverter

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1262753: P1. Add AudioConfig == and != operator. r?gerald

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

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1262753: P2. Add AudioDataBuffer::operator=. r?gerald

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

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1262753: P3. Add AudioConverter::InputConfig/OutputConfig. r?gerald

Allow to avoid storing the original AudioConfig objects.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1262753: P4. Add resampling capabilities to AudioConverter. r?kinetik

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

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1262753: P6. Perform downmixing in DecodeAudioDataSink. r?kinetik

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 6

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

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.

Comment 7

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

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 8

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

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."?

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1262753: P7. Add debugging pref to enable/disable downmixer and resample. r?kinetik

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/

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1262753: P8. Remove useless assert. r?gerald

Fly by fix unrelated to bug 1262753.

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

Comment 11

[Jean-Yves Avenard [:jya]]

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 12

[Jean-Yves Avenard [:jya]]

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 13

[Jean-Yves Avenard [:jya]]

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 14

[Jean-Yves Avenard [:jya]]

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 15

[Jean-Yves Avenard [:jya]]

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/

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1262753: P5. Pass number of frames rather than the number of bytes. r?gerald

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/

Comment 17

[Jean-Yves Avenard [:jya]]

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 18

[Jean-Yves Avenard [:jya]]

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 19

[Jean-Yves Avenard [:jya]]

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 20

[Jean-Yves Avenard [:jya]]

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/

Comment 21

[Jean-Yves Avenard [:jya]]

sorry for the noise. during rebasing two commits got squashed together.. had to resplit them

Comment 22

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

Comment on attachment 8740343 [details]
MozReview Request: Bug 1262753: P8. Remove useless assert. r?gerald

https://reviewboard.mozilla.org/r/45731/#review42285

Comment 23

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

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 24

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

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

Comment 25

[Jean-Yves Avenard [:jya]]

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 26

[Jean-Yves Avenard [:jya]]

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 27

[Jean-Yves Avenard [:jya]]

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 28

[Jean-Yves Avenard [:jya]]

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 29

[Jean-Yves Avenard [:jya]]

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/

Comment 30

[Matthew Gregan [:kinetik]]

Comment on attachment 8740243 [details]
MozReview Request: Bug 1262753: P6. Perform downmixing in DecodeAudioDataSink. r?kinetik

https://reviewboard.mozilla.org/r/45675/#review42807

Comment 31

[Matthew Gregan [:kinetik]]

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.

Comment 32

[Matthew Gregan [:kinetik]]

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

Comment 33

[Jean-Yves Avenard [:jya]]

(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.

Comment 34

[Jean-Yves Avenard [:jya]]

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 35

[Jean-Yves Avenard [:jya]]

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 36

[Jean-Yves Avenard [:jya]]

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 37

[Jean-Yves Avenard [:jya]]

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 38

[Jean-Yves Avenard [:jya]]

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 39

[Jean-Yves Avenard [:jya]]

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 40

[Jean-Yves Avenard [:jya]]

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 41

[Jean-Yves Avenard [:jya]]

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 42

[Jean-Yves Avenard [:jya]]

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/

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7da7353d7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0434086421da
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ccaf7f6a120
https://hg.mozilla.org/integration/mozilla-inbound/rev/35cf8fc31a77
https://hg.mozilla.org/integration/mozilla-inbound/rev/a610786074e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed60dd41a220
https://hg.mozilla.org/integration/mozilla-inbound/rev/369a636d86f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc049aab3fa3

Comment 44

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

https://hg.mozilla.org/mozilla-central/rev/2c7da7353d7b
https://hg.mozilla.org/mozilla-central/rev/0434086421da
https://hg.mozilla.org/mozilla-central/rev/1ccaf7f6a120
https://hg.mozilla.org/mozilla-central/rev/35cf8fc31a77
https://hg.mozilla.org/mozilla-central/rev/a610786074e7
https://hg.mozilla.org/mozilla-central/rev/ed60dd41a220
https://hg.mozilla.org/mozilla-central/rev/369a636d86f4
https://hg.mozilla.org/mozilla-central/rev/dc049aab3fa3