1308438 - Implement negative playbackRate for AudioBufferSourceNode

Status: NEW

(Core :: Web Audio, enhancement, P3)

Description

[Paul Adenot (:padenot)]

This simply plays the buffer backward at a rate corresponding to the absolute value of the playbackRate.

Spec changes at https://github.com/webaudio/web-audio-api/commit/8306a69d946e2c08bcb2f09378cc24945a1e64ed.

Comment 1

[Bao Quan [:beekill]]

Hi Dan, I would like to work on this bug as well.

Can you explain specifically what I have to do?

Comment 2

[Dan Minor [:dminor]]

Hi Bao,

This one is a bit more complicated than what you've been looking at so far. The relevant code is in this file: http://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioBufferSourceNode.cpp. 

I'd suggest starting with a playback rate of -1. To get this working, you'll have to remove the check for negative playback rate at line 469 and add implement a version of CopyFromInputBuffer that reverses the samples in the buffer. You'll also need to change the check at line 428 to make sure we don't call BorrowFromInputBuffer if there is a negative playback rate. Make sure the sample rate of the audio source you are using matches the sample rate of the web audio context so you don't hit the resampler code path by accident.

Once you get a playback rate of -1 working, you'll need to update the CopyFromInputBufferWithResampling code path to also work with negative play back rates. That might just be a matter of making a reversed copy of inputData at line 295.

All of this copying and reversing isn't ideal, but I think it is the best we can do without modifying the resampler itself, which only supports positive playback rates at the moment.

Comment 3

[Bao Quan [:beekill]]

Sorry for the delayed response!

Thank Dan for the detailed explanation. I now understand what I have to do. I'm currently looking at the source code relating to this. I'll submit a patch asap.

Comment 4

[Bao Quan [:beekill]]

Hi Dan, 

I did what you said in comment 2 but I haven't modified the CopyFromInputBufferWithResampling yet. 

I tested my work but it failed. I recently found out (through debugging) that when the playback rate is changed to -1, the check at line 391 returns true and the CopyFromInputBufferWithResampling is executed.

I'll look closer at the code and will post back asap.

AudioBufferSourceNode.cpp: http://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioBufferSourceNode.cpp

Comment 5

[Bao Quan [:beekill]]

Attachment: negative_playback_copyWithResampling.patch

Hi Dan, I modified the CopyFromInputBufferWithResampling. The patch isn't workable yet. The creating and deleting of bufferDataReversed look clumsy. Do you have any suggestion to improve that? 

I wonder that do we need to modify line 305 [1]: instead of increment, we decrease by inSamples?

About the problem in comment 4, I found out that when the playbackRate is changed, the sampleRate is also changed (in my case, it changed to -44100). This causes a problem with speex_resampler_init [2] which only accepts positive sample rate. I will find a way to deal with negative sample rate.

If there is something wrong, please let me know.

[1]: http://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioBufferSourceNode.cpp#305
[2]: http://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioBufferSourceNode.cpp#180

Comment 6

[Dan Minor [:dminor]]

Hi Bao,

Sorry for the delay, I'll have a look at this today.

Dan

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8838947 [details] [diff] [review]
negative_playback_copyWithResampling.patch

Instead of creating and freeing bufferDataReversed, I think you can modify CopyFromInputBuffer to take a boolean that if set, will make it reverse the samples as the last step before returning.

Yes, you're right, if we're playing backwards, we'll want to start at the end of the buffer, and decrease by inSamples until we reach the beginning.

For sampleRate, I think you can just divide it by -1 if playbackRate is less than zero.

Again, sorry about the delay in getting back to you!

Comment 8

[Bao Quan [:beekill]]

Attachment: negative_playback_p1.patch

Comment 9

[Bao Quan [:beekill]]

Attachment: negative_playback_p2.patch

Comment 10

[Bao Quan [:beekill]]

Attachment: negative_playback_p3.patch

Comment 11

[Bao Quan [:beekill]]

Attachment: test_negative_playback_rate.zip

Hi Dan,

I test my patches with this. It can play the source in backward. But there's a problem with it. When the playback is finished (with negative playback rate), the sound icon in the tab header sometimes won't disappear. This makes Firefox hang when closing.

I haven’t found what causes the problem. Could you have a look at this?

Comment 12

[Bao Quan [:beekill]]

Attachment: negative_playback_p4.patch

Hi Dan, I reimplemented the CopyFromInputBufferWithResampling to support negative playback rate. I stick with allocating and freeing bufferDataReversed as I don't believe we can use CopyFromInputBuffer here.

I don't think we can check whether the output sound is correct. Can you suggest how I should write a test case?

Comment 13

[Dan Minor [:dminor]]

Hi Bao,

You can use Audacity to reverse audio (look for the 'reverse' effect). I'd suggest reversing something and then playing it with a negative playback rate in Firefox and seeing if it matches the original. For an automated test, you could maybe use a short bit of reversed audio in an OfflineAudioContext and play it with a negative rate and load the original and compare them sample by sample.

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8840847 [details] [diff] [review]
negative_playback_p3.patch

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

This is looking good so far.

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +403,5 @@
> +                         mPlaybackRateTimeline.GetValueAtTime(*aCurrentPosition);
> +
> +    uint32_t availableInOutput = playbackRate >= 0.0f ?
> +                                 std::min<StreamTime>(WEBAUDIO_BLOCK_SIZE - *aOffsetWithinBlock, mStop - *aCurrentPosition) :
> +                                 std::min<StreamTime>(WEBAUDIO_BLOCK_SIZE - *aOffsetWithinBlock, *aCurrentPosition); 

aCurrentPosition, mStart and mStop are in stream time, so aCurrentPosition should always advance and you don't to have a special case here for negative playback  rate. At the moment, aCurrentPosition can be zero when the buffer begins playing and so availableInOutput will be zero, and nothing will happen.

@@ +461,5 @@
> +    if (playbackRate >= 0.0f) {
> +      *aCurrentPosition += numFrames;
> +      mBufferPosition += numFrames;
> +    } else {
> +      *aCurrentPosition -= numFrames;

aCurrentPosition will still advance here as it is elapsed time. mBufferPosition will decrease, because we're playing backwards.

@@ +462,5 @@
> +      *aCurrentPosition += numFrames;
> +      mBufferPosition += numFrames;
> +    } else {
> +      *aCurrentPosition -= numFrames;
> +      mBufferPosition -= numFrames; 

nit: be careful about trailing whitespace.

@@ +552,5 @@
>      // We've finished if we've gone past mStop, or if we're past mDuration when
>      // looping is disabled.
>      if (streamPosition >= mStop ||
> +        (!mLoop && mBufferPosition >= mBufferEnd && !mRemainingResamplerTail) ||
> +        (!mLoop && mBufferPosition <= 0 && !mRemainingResamplerTail)) {

I think this should be mBufferPosition < 0

@@ +557,1 @@
>        *aFinished = true;

While I was experimenting, I saw aFinished be set to true, but the buffer continued trying to produce output. I think this is why you're seeing the "audio playing" indicator in the tab and Firefox hang on exit.

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8840845 [details] [diff] [review]
negative_playback_p1.patch

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

This looks good, but I'd prefer to see this combined with a larger patch, it's hard to review on it's own.

Comment 16

[Dan Minor [:dminor]]

Comment on attachment 8840846 [details] [diff] [review]
negative_playback_p2.patch

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

lgtm

Comment 17

[Dan Minor [:dminor]]

Comment on attachment 8841224 [details] [diff] [review]
negative_playback_p4.patch

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

This looks good so far, I'll look it over in more detail once we the non-resampling code path is finished.

Comment 18

[Bao Quan [:beekill]]

Attachment: negative_playback_part1.patch

I have to implement a new function to copy from the input buffer in reverse buffer. If we set the mBufferPosition to the end of buffer, then CopyFromInputBuffer will copy some garbage values to output buffer, which causes the test failed.

Comment 19

[Bao Quan [:beekill]]

Attachment: negative_playback_part2.patch

Comment 20

[Bao Quan [:beekill]]

Attachment: negative_playback_part3.patch

Comment 21

[Bao Quan [:beekill]]

Attachment: negative_playback_part4.patch

Comment 22

[Bao Quan [:beekill]]

Attachment: negative_playback_part5.patch

Comment 23

[Bao Quan [:beekill]]

Attachment: negative_playback_part6.patch

Comment 24

[Bao Quan [:beekill]]

Attachment: negative_playback_part7.patch

These test cases I copied from [1], [2] and [3]. Do you think I need to write more test cases?

[1]: http://searchfox.org/mozilla-central/source/dom/media/webaudio/test/test_audioBufferSourceNode.html
[2]: http://searchfox.org/mozilla-central/source/dom/media/webaudio/test/test_audioBufferSourceNodeLoop.html
[3]: http://searchfox.org/mozilla-central/source/dom/media/webaudio/test/test_audioBufferSourceNodeRate.html

Comment 25

[Paul Adenot (:padenot)]

I lack time to review intermediate patches at the moment, but I can have a look at the finished patches later.

Comment 26

[Dan Minor [:dminor]]

Comment on attachment 8842903 [details] [diff] [review]
negative_playback_part2.patch

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

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +540,2 @@
>        } else {
> +        if ((playbackRate < 0 && mBufferPosition) || 

nit: remove trailing whitespace

@@ +540,3 @@
>        } else {
> +        if ((playbackRate < 0 && mBufferPosition) || 
> +            (playbackRate >= 0 && mBufferPosition < mBufferEnd) || 

nit: remove trailing whitespace

Comment 27

[Dan Minor [:dminor]]

Comment on attachment 8842904 [details] [diff] [review]
negative_playback_part3.patch

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

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +455,5 @@
>        if (*aOffsetWithinBlock == 0) {
>          aOutput->AllocateChannels(aChannels);
>        }
> +      if (playbackRate >= 0) {
> +        MOZ_ASSERT(mBufferPosition < aBufferMax);

Please move this assertion outside of this if statement, mBufferPosition should always be less than aBufferMax, even if the playback rate is negative.

@@ +464,4 @@
>      }
>      *aOffsetWithinBlock += numFrames;
>      *aCurrentPosition += numFrames;
> +    mBufferPosition += playbackRate >= 0 ? numFrames : -numFrames;

There's a number of places where you're subtracting from mBufferPosition, which is an unsigned int. I think it would be best to add an assertion that this subtraction won't cause the value to wrap around to a large position number:

MOZ_ASSERT(numFrames <= mBufferPosition);

Please do something like this every place where you subtract from mBufferPosition.

Comment 28

[Dan Minor [:dminor]]

Comment on attachment 8842905 [details] [diff] [review]
negative_playback_part4.patch

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

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +319,5 @@
>          uint32_t inSamples = inputLimit;
>          const float* inputData = mBuffer->GetData(i) + mBufferPosition;
> +        float* bufferDataReversed = nullptr;
> +        if (playbackRate < 0) {
> +          bufferDataReversed = new float[inSamples];

Rather than allocating and freeing this buffer inside the loop, I think it would be a bit more efficient to allocate the buffer just once outside the loop. You could then delete[] it just before the return call below.

Comment 29

[Dan Minor [:dminor]]

Comment on attachment 8842911 [details] [diff] [review]
negative_playback_part7.patch

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

::: dom/media/webaudio/test/test_audioBufferSourceNodeNegativePlaybackRateResampling.html
@@ +21,5 @@
> +var buf = off.createBuffer(1, 2048, rate);
> +var buf2 = off2.createBuffer(1, 2048, rate);
> +for (var i = 0; i < 2048; ++i) {
> +  buf.getChannelData(0)[i] = Math.sin(i * 440 * 2 * Math.PI / rate);
> +  buf2.getChannelData(0)[2047 - i] = Math.sin(i * 440 * 2 * Math.PI / rate);

Rather than repeat the calculation you could compute the value once and assign it to both buffers.

Comment 30

[Dan Minor [:dminor]]

Comment on attachment 8842907 [details] [diff] [review]
negative_playback_part6.patch

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

::: dom/media/webaudio/test/test_audioBufferSourceNodeNegativePlaybackRateLoop.html
@@ +21,5 @@
> +
> +    var source = context.createBufferSource();
> +    source.buffer = buffer;
> +    source.start(0);
> +	source.playbackRate.value = -1;

nit: looks like you have a tab here instead of spaces

Comment 31

[Dan Minor [:dminor]]

This is looking good so far.

Comment 32

[Bao Quan [:beekill]]

(In reply to Dan Minor [:dminor] from comment #27)
> Comment on attachment 8842904 [details] [diff] [review]
> negative_playback_part3.patch
> 
> Review of attachment 8842904 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webaudio/AudioBufferSourceNode.cpp
> @@ +455,5 @@
> >        if (*aOffsetWithinBlock == 0) {
> >          aOutput->AllocateChannels(aChannels);
> >        }
> > +      if (playbackRate >= 0) {
> > +        MOZ_ASSERT(mBufferPosition < aBufferMax);
> 
> Please move this assertion outside of this if statement, mBufferPosition
> should always be less than aBufferMax, even if the playback rate is negative.

When we set the playback rate to negative value and the buffer position to the end of the buffer, shouldn't the mBufferPosition == aBufferMax? In that case, can we do this?
> if (playbackRate >= 0) {
>   MOZ_ASSERT(mBufferPosition < aBufferMax);
> } else {
>   MOZ_ASSERT(mBufferPosition <= aBufferMax);
> }

Comment 33

[Dan Minor [:dminor]]

(In reply to Bao Quan [:beekill] from comment #32)
> (In reply to Dan Minor [:dminor] from comment #27)
> > Comment on attachment 8842904 [details] [diff] [review]
> > negative_playback_part3.patch
> > 
> > Review of attachment 8842904 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/webaudio/AudioBufferSourceNode.cpp
> > @@ +455,5 @@
> > >        if (*aOffsetWithinBlock == 0) {
> > >          aOutput->AllocateChannels(aChannels);
> > >        }
> > > +      if (playbackRate >= 0) {
> > > +        MOZ_ASSERT(mBufferPosition < aBufferMax);
> > 
> > Please move this assertion outside of this if statement, mBufferPosition
> > should always be less than aBufferMax, even if the playback rate is negative.
> 
> When we set the playback rate to negative value and the buffer position to
> the end of the buffer, shouldn't the mBufferPosition == aBufferMax? In that
> case, can we do this?
> > if (playbackRate >= 0) {
> >   MOZ_ASSERT(mBufferPosition < aBufferMax);
> > } else {
> >   MOZ_ASSERT(mBufferPosition <= aBufferMax);
> > }

Good point. I guess it's best to add a separate assertion for the negative playback rate.

Comment 34

[Bao Quan [:beekill]]

Attachment: negative_playback_part2.patch

Comment 35

[Bao Quan [:beekill]]

Attachment: negative_playback_part3.patch

Comment 36

[Bao Quan [:beekill]]

Attachment: negative_playback_part4.patch

Comment 37

[Bao Quan [:beekill]]

Attachment: negative_playback_part5.patch

Comment 38

[Bao Quan [:beekill]]

Attachment: negative_playback_part6.patch

Comment 39

[Bao Quan [:beekill]]

Attachment: negative_playback_part7.patch

Comment 40

[Bao Quan [:beekill]]

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3a7a1340d50067f683da6c40b971e87c4b999a3

Comment 41

[Dan Minor [:dminor]]

These all look good to me.

Comment 42

[Bao Quan [:beekill]]

Hi Dan,

The Try server reported a build failed in Linux x64 QuantumRender opt. Is this ok?

:sheppy added keyword dev-doc-needed. Does that mean I have to write a documentation?

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3a7a1340d50067f683da6c40b971e87c4b999a3

Comment 43

[Paul Adenot (:padenot)]

(In reply to Bao Quan [:beekill] from comment #42)
> Hi Dan,
> 
> The Try server reported a build failed in Linux x64 QuantumRender opt. Is
> this ok?

You can ignore it, I had a look and it's not your fault.

> :sheppy added keyword dev-doc-needed. Does that mean I have to write a
> documentation?

That means someone will write the docs.

Comment 44

[Paul Adenot (:padenot)]

Comment on attachment 8842902 [details] [diff] [review]
negative_playback_part1.patch

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

r+ with comments addressed of course.

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +242,5 @@
> +  // but the input buffer is copied in reverse order
> +  void ReverseCopyFromInputBuffer(AudioBlock* aOutput,
> +                                   uint32_t aChannels,
> +                                   uintptr_t aOffsetWithinBlock,
> +                                   uint32_t aNumberOfFrames) {

Indentation is a bit off here.

@@ +244,5 @@
> +                                   uint32_t aChannels,
> +                                   uintptr_t aOffsetWithinBlock,
> +                                   uint32_t aNumberOfFrames) {
> +    for (uint32_t i = 0; i < aChannels; ++i) {
> +      float* baseChannelData = aOutput->ChannelFloatsForWrite(i) + aOffsetWithinBlock;

There should be an assert or something that guarantees that we're not accessing something out of the arrays, here.

Comment 45

[Paul Adenot (:padenot)]

Comment on attachment 8844473 [details] [diff] [review]
negative_playback_part2.patch

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

r+ with comments addressed.

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +156,5 @@
>  
>    void UpdateResampler(int32_t aOutRate, uint32_t aChannels)
>    {
> +    // Only work with non negative aOutRate
> +    aOutRate = aOutRate >= 0 ? aOutRate : -aOutRate;

This is better written as:

> aOutRate = std::abs(aOutputRate);

Comment 46

[Paul Adenot (:padenot)]

Comment on attachment 8844475 [details] [diff] [review]
negative_playback_part3.patch

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

r+ with the style fix.

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +465,5 @@
>      }
>      *aOffsetWithinBlock += numFrames;
>      *aCurrentPosition += numFrames;
> +    if (playbackRate >= 0)
> +      mBufferPosition += numFrames;

Braces here.

Comment 47

[Paul Adenot (:padenot)]

Comment on attachment 8844476 [details] [diff] [review]
negative_playback_part4.patch

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

This needs a bit more work, but it should not be too hard.

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +316,5 @@
>        inputLimit = std::min(inputLimit, availableInInputBuffer);
>  
> +      float* bufferDataReversed = nullptr;
> +      if (playbackRate < 0)
> +        bufferDataReversed = new float[inputLimit];

We don't want to allocate here, we're in a real-time thread, and allocation can be blocking.

Since we already have a setup where we can perform the rendering of the 128 output floats in multiple passes, we can make it so that we use a fixed-size buffer on the stack.

Also, the style guide makes braces mandatory, even with single-statement ifs and such.

@@ +322,5 @@
>        for (uint32_t i = 0; true; ) {
>          uint32_t inSamples = inputLimit;
>          const float* inputData = mBuffer->GetData(i) + mBufferPosition;
> +        if (playbackRate < 0) {
> +          for (uint32_t i = 0; i < inSamples; ++i)

Braces.

@@ +323,5 @@
>          uint32_t inSamples = inputLimit;
>          const float* inputData = mBuffer->GetData(i) + mBufferPosition;
> +        if (playbackRate < 0) {
> +          for (uint32_t i = 0; i < inSamples; ++i)
> +            bufferDataReversed[i] = *(inputData - i - 1);

Don't mix array subscript notation with pointer arithmetic. I think it's better to just use array subscript notation here.

@@ +325,5 @@
> +        if (playbackRate < 0) {
> +          for (uint32_t i = 0; i < inSamples; ++i)
> +            bufferDataReversed[i] = *(inputData - i - 1);
> +          inputData = bufferDataReversed;
> +        }

Can't you use the function you wrote in patch 1 to do this?

@@ +336,4 @@
>                                               inputData, &inSamples,
>                                               outputData, &outSamples);
>          if (++i == aChannels) {
> +          if (playbackRate >= 0)

Braces.

@@ +352,4 @@
>              mRemainingResamplerTail =
>                2 * speex_resampler_get_input_latency(resampler) - 1;
>            }
> +          if (bufferDataReversed)

Braces.

@@ +352,5 @@
>              mRemainingResamplerTail =
>                2 * speex_resampler_get_input_latency(resampler) - 1;
>            }
> +          if (bufferDataReversed)
> +            delete[] bufferDataReversed;

Bare new/delete calls are to be avoided, but we're removing the `new` here so this can go away.

Comment 48

[Paul Adenot (:padenot)]

I'd like to see a test where we set the playbackRate to 0, and a test where the playback rate goes from -2 to 2, crossing zero. I think those are really must-have to ship this.

Comment 49

[Paul Adenot (:padenot)]

Bao Quan, good work on this, let me know if you need anything to bring this to the finish line !

Also, note that, when you have all your patches applied, you can run `./mach clang-format` to fix some style issues, it's better than having to track everything by hand !

Comment 50

[Bao Quan [:beekill]]

(In reply to Paul Adenot (:padenot) from comment #44)
> @@ +244,5 @@
> > +                                   uint32_t aChannels,
> > +                                   uintptr_t aOffsetWithinBlock,
> > +                                   uint32_t aNumberOfFrames) {
> > +    for (uint32_t i = 0; i < aChannels; ++i) {
> > +      float* baseChannelData = aOutput->ChannelFloatsForWrite(i) + aOffsetWithinBlock;
> 
> There should be an assert or something that guarantees that we're not
> accessing something out of the arrays, here.

Did you mean this line:
> baseChannelData[j] = bufferData[mBufferPosition - j - 1];
as we did aOutput->ChannelFloatsForWrite(i) + aOffsetWithinBlock here: http://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioBufferSourceNode.cpp#235.

In that case, I think the assertion would be like this:
> MOZ_ASSERT(mBufferPosition - 1 < WEBAUDIO_BLOCK_SIZE)

Comment 51

[Bao Quan [:beekill]]

(In reply to Paul Adenot (:padenot) from comment #47)
> @@ +325,5 @@
> > +        if (playbackRate < 0) {
> > +          for (uint32_t i = 0; i < inSamples; ++i)
> > +            bufferDataReversed[i] = *(inputData - i - 1);
> > +          inputData = bufferDataReversed;
> > +        }
> 
> Can't you use the function you wrote in patch 1 to do this?

I don't think that the function in patch 1 can be used here as the function accepts AudioBlock* instead of float*.

Comment 52

[Bao Quan [:beekill]]

(In reply to Paul Adenot (:padenot) from comment #48)
> I'd like to see a test where we set the playbackRate to 0, and a test where
> the playback rate goes from -2 to 2, crossing zero. I think those are really
> must-have to ship this.

Did you mean that I should write a test like this:
1. Set the start to the end of the buffer
2. Set the playback rate to -2
3. Set the playback rate to 2 when it reaches halfway point
4. Check the output is produced correctly

Comment 53

[Bao Quan [:beekill]]

(In reply to Paul Adenot (:padenot) from comment #49)
> Bao Quan, good work on this, let me know if you need anything to bring this
> to the finish line !
> 
> Also, note that, when you have all your patches applied, you can run `./mach
> clang-format` to fix some style issues, it's better than having to track
> everything by hand !

Thank you for the tip. I'll let you know if I need something.

Comment 54

[Paul Adenot (:padenot)]

(In reply to Bao Quan [:beekill] from comment #52)
> (In reply to Paul Adenot (:padenot) from comment #48)
> > I'd like to see a test where we set the playbackRate to 0, and a test where
> > the playback rate goes from -2 to 2, crossing zero. I think those are really
> > must-have to ship this.
> 
> Did you mean that I should write a test like this:
> 1. Set the start to the end of the buffer
> 2. Set the playback rate to -2
> 3. Set the playback rate to 2 when it reaches halfway point
> 4. Check the output is produced correctly

You can make the playbackRate automatically follow a curve:

> var ac = new AudioContext();
> var source = ac.createBufferSource();
> source.buffer = someBuffer;
> source.connect(ac.destination);
> source.playbackRate.setValueAtTime(2.0, ac.currentTime);
> source.playbackRate.linearRampToValueAtTime(-2.0, ac.currentTime + 5.0);
> source.start();

Also, what should happen when the playback rate is zero ?

Comment 55

[Bao Quan [:beekill]]

(In reply to Paul Adenot (:padenot) from comment #54)
> (In reply to Bao Quan [:beekill] from comment #52)
> > (In reply to Paul Adenot (:padenot) from comment #48)
>
> ...
> 
> Also, what should happen when the playback rate is zero ?

I'm not sure if this is a suitable testcase for this.
When the playback rate is zero then its behaviour is similar to pausing the playback. In that case, I think I'll set the playback to 0 and wait for sometimes. After the waiting, I'll check whether the playback is finished. If it is, then the test will fail.

Comment 56

[Paul Adenot (:padenot)]

(In reply to Bao Quan [:beekill] from comment #55)
> I'm not sure if this is a suitable testcase for this.
> When the playback rate is zero then its behaviour is similar to pausing the
> playback. In that case, I think I'll set the playback to 0 and wait for
> sometimes. After the waiting, I'll check whether the playback is finished.
> If it is, then the test will fail.

Yep, I think it would work.

Comment 57

[Bao Quan [:beekill]]

Hi Dan,

I'm working on a test case to test the playback rate goes from 2 to -2. The playback rate is set as Paul suggested:
> var ac = new AudioContext();
> var source = ac.createBufferSource();
> source.buffer = someBuffer;
> source.connect(ac.destination);
> source.playbackRate.setValueAtTime(2.0, ac.currentTime);
> source.playbackRate.linearRampToValueAtTime(-2.0, ac.currentTime + 5.0);
> source.start();

Instead of using AudioContext, I use OfflineAudioContext and the startRendering method [1]. The method will return a renderedBuffer. I then split the buffer in half: the first half is when the playback rate is positive and the second half is negative playback rate. The two buffers are then compared (with one buffer is reversed) with each other.

But the test fails. Is my approach correct?

[1]: https://developer.mozilla.org/en-US/docs/Web/API/OfflineAudioContext/startRendering%28promise%29

Comment 58

[Dan Minor [:dminor]]

How large is your buffer? Is it possible that there is not enough time for the ramp to complete before you fill the buffer and so you never get all the way to -2.0 before it stops rendering?

Comment 59

[Bao Quan [:beekill]]

I think I solve the problem. I didn't know that the rendered buffer will be the same length as the length of OfflineAudioContext. I'll push my patches asap.

Comment 60

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 1: Implement ReverseCopyFromInputBuffer to perform copy from input buffer in reverse order.

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

Comment 61

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 2: Support negative playback rate in ProcessBlock and add parameter aBufferMin to CopyFromBuffer's signature.

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

Comment 62

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 3: Modify CopyFromBuffer to work with negative playback rate.

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

Comment 63

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 4: Add parameter aBufferMin to CopyFromInputBufferWithResampling and modify the function to work with negative playback rate.

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

Comment 64

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 5: Add a test case to test AudioBufferSourceNode with negative playback rate.

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

Comment 65

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 6: Test AudioBufferSourceNode with negative playback rate and looping.

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

Comment 66

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 7: Test AudioBufferSourceNode with negative playback rate and resampling.

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

Comment 67

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 8: Test AudioBufferSourceNode with zero playback rate.

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

Comment 68

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 9: Test AudioBufferSourceNode with playback rate goes from 2 to -2.

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

Comment 69

[Bao Quan [:beekill]]

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=713ae3890daaae460580e4b691fef3a61d3cea3c&selectedJob=85392277

Comment 70

[Dan Minor [:dminor]]

Comment on attachment 8849956 [details]
Bug 1308438 - Part 1: Implement ReverseCopyFromInputBuffer to perform copy from input buffer in reverse order.

https://reviewboard.mozilla.org/r/122712/#review124878

Comment 71

[Dan Minor [:dminor]]

Comment on attachment 8849957 [details]
Bug 1308438 - Part 2: Support negative playback rate in ProcessBlock and add parameter aBufferMin to CopyFromBuffer's signature.

https://reviewboard.mozilla.org/r/122714/#review124956

Comment 72

[Dan Minor [:dminor]]

Comment on attachment 8849958 [details]
Bug 1308438 - Part 3: Modify CopyFromBuffer to work with negative playback rate.

https://reviewboard.mozilla.org/r/122716/#review124958

Comment 73

[Dan Minor [:dminor]]

Comment on attachment 8849959 [details]
Bug 1308438 - Part 4: Add parameter aBufferMin to CopyFromInputBufferWithResampling and modify the function to work with negative playback rate.

https://reviewboard.mozilla.org/r/122718/#review124964

::: dom/media/webaudio/AudioBufferSourceNode.cpp:325
(Diff revision 1)
>          uint32_t inSamples = inputLimit;
> -        const float* inputData = mBuffer->GetData(i) + mBufferPosition;
> +        const float* inputData;
> +        if (playbackRate >= 0) {
> +          inputData = mBuffer->GetData(i) + mBufferPosition;
> +        } else {
> +          const float* bufferData = mBuffer->GetData(i);

Please add an assertion that inSamples <= WEBAUDIO_BLOCK_SIZE

Comment 74

[Dan Minor [:dminor]]

Comment on attachment 8849960 [details]
Bug 1308438 - Part 5: Add a test case to test AudioBufferSourceNode with negative playback rate.

https://reviewboard.mozilla.org/r/122720/#review124966

Comment 75

[Dan Minor [:dminor]]

Comment on attachment 8849961 [details]
Bug 1308438 - Part 6: Test AudioBufferSourceNode with negative playback rate and looping.

https://reviewboard.mozilla.org/r/122722/#review124968

Comment 76

[Dan Minor [:dminor]]

Comment on attachment 8849962 [details]
Bug 1308438 - Part 7: Test AudioBufferSourceNode with negative playback rate and resampling.

https://reviewboard.mozilla.org/r/122724/#review124970

Comment 77

[Dan Minor [:dminor]]

Comment on attachment 8849963 [details]
Bug 1308438 - Part 8: Test AudioBufferSourceNode with zero playback rate.

https://reviewboard.mozilla.org/r/122726/#review124972

Comment 78

[Dan Minor [:dminor]]

Comment on attachment 8849964 [details]
Bug 1308438 - Part 9: Test AudioBufferSourceNode with playback rate goes from 2 to -2.

https://reviewboard.mozilla.org/r/122728/#review124974

Comment 79

[Dan Minor [:dminor]]

Comment on attachment 8849964 [details]
Bug 1308438 - Part 9: Test AudioBufferSourceNode with playback rate goes from 2 to -2.

https://reviewboard.mozilla.org/r/122728/#review124976

::: dom/media/webaudio/test/test_audioBufferSourceNodePlaybackRateFromTwotoNegativeTwo.html:26
(Diff revision 1)
> +  buf.getChannelData(0)[i] = Math.sin(i * 440 * 2 * Math.PI / rate);
> +}
> +
> +source.buffer = buf;
> +source.playbackRate.setValueAtTime(2.0, off.currentTime);
> +source.playbackRate.linearRampToValueAtTime(-2.0, off.currentTime);

Should this be off.currentTime + some constant?

Comment 80

[Paul Adenot (:padenot)]

Comment on attachment 8849964 [details]
Bug 1308438 - Part 9: Test AudioBufferSourceNode with playback rate goes from 2 to -2.

https://reviewboard.mozilla.org/r/122728/#review126328

::: dom/media/webaudio/test/test_audioBufferSourceNodePlaybackRateFromTwotoNegativeTwo.html:26
(Diff revision 1)
> +  buf.getChannelData(0)[i] = Math.sin(i * 440 * 2 * Math.PI / rate);
> +}
> +
> +source.buffer = buf;
> +source.playbackRate.setValueAtTime(2.0, off.currentTime);
> +source.playbackRate.linearRampToValueAtTime(-2.0, off.currentTime);

currentTime is meaningless here.

Use 0.0 on line 25 and 1.0 on line 26, so that the ramp lasts a second, which is the duration of the OfflineAudioContext. You can also use (off.length / off.sampleRate) so that it's more generic.

Comment 81

[Paul Adenot (:padenot)]

Comment on attachment 8849956 [details]
Bug 1308438 - Part 1: Implement ReverseCopyFromInputBuffer to perform copy from input buffer in reverse order.

https://reviewboard.mozilla.org/r/122712/#review126336

Comment 82

[Paul Adenot (:padenot)]

Comment on attachment 8849957 [details]
Bug 1308438 - Part 2: Support negative playback rate in ProcessBlock and add parameter aBufferMin to CopyFromBuffer's signature.

https://reviewboard.mozilla.org/r/122714/#review126338

Comment 83

[Paul Adenot (:padenot)]

Comment on attachment 8849958 [details]
Bug 1308438 - Part 3: Modify CopyFromBuffer to work with negative playback rate.

https://reviewboard.mozilla.org/r/122716/#review126340

Comment 84

[Paul Adenot (:padenot)]

Comment on attachment 8849960 [details]
Bug 1308438 - Part 5: Add a test case to test AudioBufferSourceNode with negative playback rate.

https://reviewboard.mozilla.org/r/122720/#review126346

Comment 85

[Paul Adenot (:padenot)]

Comment on attachment 8849961 [details]
Bug 1308438 - Part 6: Test AudioBufferSourceNode with negative playback rate and looping.

https://reviewboard.mozilla.org/r/122722/#review126332

::: dom/media/webaudio/test/test_audioBufferSourceNodeNegativePlaybackRateLoop.html:4
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Bug 1308438 - Test AudioBufferSourceNode looping with negative playback rate</title>

Good test. We should add another test case where loop start and end are specified.

Comment 86

[Paul Adenot (:padenot)]

Comment on attachment 8849962 [details]
Bug 1308438 - Part 7: Test AudioBufferSourceNode with negative playback rate and resampling.

https://reviewboard.mozilla.org/r/122724/#review126348

Comment 87

[Paul Adenot (:padenot)]

Comment on attachment 8849963 [details]
Bug 1308438 - Part 8: Test AudioBufferSourceNode with zero playback rate.

https://reviewboard.mozilla.org/r/122726/#review126334

::: dom/media/webaudio/test/test_audioBufferSourceNodeZeroPlaybackRate.html:32
(Diff revision 1)
> +var isSourceNotEnded = true;
> +source.onended = function(event) {
> +  isSourceNotEnded = false;
> +};
> +
> +var interval = setInterval(function() {

This is a bit frowned upon, because it will lead to intermittents. However, I don't really have a better idea.

Comment 88

[Paul Adenot (:padenot)]

I've done a try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54f074aba80900413c6cbfea82e33c9776eb0db1

Comment 89

[Paul Adenot (:padenot)]

Comment on attachment 8849959 [details]
Bug 1308438 - Part 4: Add parameter aBufferMin to CopyFromInputBufferWithResampling and modify the function to work with negative playback rate.

https://reviewboard.mozilla.org/r/122718/#review126366

Comment 90

[Paul Adenot (:padenot)]

Comment on attachment 8849959 [details]
Bug 1308438 - Part 4: Add parameter aBufferMin to CopyFromInputBufferWithResampling and modify the function to work with negative playback rate.

https://reviewboard.mozilla.org/r/122718/#review126368

Comment 91

[Bao Quan [:beekill]]

Attachment: playback rate from 2 to -2 gdb output

I'm sorry for not being active lately. I've got a lot of homework from my school.
I'm trying to get the playback rate goes from 2 to -2 test to work but it keeps failing. 

I printed the playback rate to the file. The playback rates were not zero-symmetric. And from line 464 to line 496, the playback rate jumped from 0.938188 to -0.937324. Maybe these are why the test keeps failing?

I used gdb log to output the playback rate to the file. I'm not sure how it affects the playback rate.

Comment 92

[Dan Minor [:dminor]]

Hi Bao, Don't worry about not being active, you've made great progress on this bug! I'll check out that test today and let you know if I find anything.

Comment 93

[Dan Minor [:dminor]]

Comment on attachment 8849959 [details]
Bug 1308438 - Part 4: Add parameter aBufferMin to CopyFromInputBufferWithResampling and modify the function to work with negative playback rate.

https://reviewboard.mozilla.org/r/122718/#review127704

::: dom/media/webaudio/AudioBufferSourceNode.cpp:316
(Diff revision 1)
>          speex_resampler_set_skip_frac_num(resampler,
>                                    std::min<int64_t>(skipFracNum, UINT32_MAX));
>  
>          mBeginProcessing = -STREAM_TIME_MAX;
>        }
>        inputLimit = std::min(inputLimit, availableInInputBuffer);

inputLimit can be greater than WEBAUDIO_BLOCK_SIZE, leading to buffer overflows.

Please add:

inputLimit = std::min(inputLimit, WEBAUDIO_BLOCK_SIZE);

when playbackRate < 0.

I'm not familiar with the resampler, but it's possible restricting the input size like this will cause problems. Paul would know better than I. When I looked at the values when your test was running, they were all less than 2*WEBAUDIO_BLOCK_SIZE, so you might want to consider using that size for bufferDataReversed.

Comment 94

[Dan Minor [:dminor]]

Hi Bao,

If I add a printf for playbackRate in ProcessBlock, I see that when your test is running (with the suggestions Paul made above applied) the playback rates are not symmetrical around zero. Here's a short excerpt:

>>>> playbackRate: 0.003637
>>>> playbackRate: 0.003057
>>>> playbackRate: 0.002476
>>>> playbackRate: 0.001896
>>>> playbackRate: 0.001315
>>>> playbackRate: 0.000735
>>>> playbackRate: 0.000154
>>>> playbackRate: -0.000426
>>>> playbackRate: -0.001007
>>>> playbackRate: -0.001587
>>>> playbackRate: -0.002168
>>>> playbackRate: -0.002748
>>>> playbackRate: -0.003329
>>>> playbackRate: -0.003909

I'm not sure if these differences are large enough to explain the errors you're seeing in your test, but it does look like it could cause problems. You might be able to play around with the start and end values for playback rate and buffer size and make things symmetrical, or at least close enough that the errors are within tolerance

There might still be another problem with your test, I'm not sure.

Comment 95

[Bao Quan [:beekill]]

Attachment: Bug 1308438 - Part 10: Test AudioBufferSourceNode with negative playback rate and looping with specified start and end.

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

Comment 96

[Bao Quan [:beekill]]

Comment on attachment 8849959 [details]
Bug 1308438 - Part 4: Add parameter aBufferMin to CopyFromInputBufferWithResampling and modify the function to work with negative playback rate.

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

Comment 97

[Bao Quan [:beekill]]

Comment on attachment 8849960 [details]
Bug 1308438 - Part 5: Add a test case to test AudioBufferSourceNode with negative playback rate.

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

Comment 98

[Bao Quan [:beekill]]

Comment on attachment 8849961 [details]
Bug 1308438 - Part 6: Test AudioBufferSourceNode with negative playback rate and looping.

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

Comment 99

[Bao Quan [:beekill]]

Comment on attachment 8849962 [details]
Bug 1308438 - Part 7: Test AudioBufferSourceNode with negative playback rate and resampling.

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

Comment 100

[Bao Quan [:beekill]]

Comment on attachment 8849963 [details]
Bug 1308438 - Part 8: Test AudioBufferSourceNode with zero playback rate.

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

Comment 101

[Bao Quan [:beekill]]

Comment on attachment 8849964 [details]
Bug 1308438 - Part 9: Test AudioBufferSourceNode with playback rate goes from 2 to -2.

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

Comment 102

[Bao Quan [:beekill]]

Sorry for my delayed response!

I couldn't get the test 9 to work so I changed it to something similar to test 4. I'll look closely why did the old test keep failing.
Here's is try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59356fa9ef11fc62db92ee9cb4ad169df580a0b8

Comment 103

[Dan Minor [:dminor]]

Hi Bao,

Sorry for not getting to this right away. I'll have a look today. Paul is away until Thursday, so he might not have a chance to have a look until the end of the week.

Comment 104

[Dan Minor [:dminor]]

Comment on attachment 8849959 [details]
Bug 1308438 - Part 4: Add parameter aBufferMin to CopyFromInputBufferWithResampling and modify the function to work with negative playback rate.

https://reviewboard.mozilla.org/r/122718/#review138118

Comment 105

[Dan Minor [:dminor]]

Comment on attachment 8849964 [details]
Bug 1308438 - Part 9: Test AudioBufferSourceNode with playback rate goes from 2 to -2.

https://reviewboard.mozilla.org/r/122728/#review138122

Please remove the use of currentTime in your linearRampToValueAtTime calls and use constants like Paul suggested.

Comment 106

[Dan Minor [:dminor]]

Comment on attachment 8862473 [details]
Bug 1308438 - Part 10: Test AudioBufferSourceNode with negative playback rate and looping with specified start and end.

https://reviewboard.mozilla.org/r/134388/#review138138

::: dom/media/webaudio/test/test_audioBufferSourceNodeNegativePlaybackRateLoopStartEnd.html:29
(Diff revision 1)
> +    source.playbackRate.value = -1;
> +    return source;
> +  },
> +  createExpectedBuffers: function(context) {
> +    var expectedBuffer = context.createBuffer(1, 2048 * 4, context.sampleRate);
> +    for (var i = 0; i < 1536; ++i) {

Why is this 1536? Shouldn't the whole buffer play once prior to the first loop ocurring?

Comment 107

[Bao Quan [:beekill]]

(In reply to Dan Minor [:dminor] from comment #106)
> Comment on attachment 8862473 [details]
> Bug 1308438 - Part 10: Test AudioBufferSourceNode with negative playback
> rate and looping with specified start and end.
> 
> https://reviewboard.mozilla.org/r/134388/#review138138
> 
> :::
> dom/media/webaudio/test/
> test_audioBufferSourceNodeNegativePlaybackRateLoopStartEnd.html:29
> (Diff revision 1)
> > +    source.playbackRate.value = -1;
> > +    return source;
> > +  },
> > +  createExpectedBuffers: function(context) {
> > +    var expectedBuffer = context.createBuffer(1, 2048 * 4, context.sampleRate);
> > +    for (var i = 0; i < 1536; ++i) {
> 
> Why is this 1536? Shouldn't the whole buffer play once prior to the first
> loop ocurring?

I set the initial buffer position to the end of the buffer. And when it hit the loopStart, which is 25% of the buffer's duration, it'll return to the loopEnd position. When it hit the loopStart position, the output length should be 75% of original buffer, which is 1536.

Comment 108

[Bao Quan [:beekill]]

I found why the test with looping and specified start end failed.
When we're looping with specified start end, the function CopyFromBuffer is called
with mBufferMax is mLoopEnd (line 578 in [1]). In the test, the mLoopEnd is set to 0.75 of buffer's 
duration and the beginning position is at end of the buffer. In that case, the mBufferPosition is greater than aBufferMax
which will make the test failed at line 487 in [1]:
> MOZ_ASSERT(mBufferPosition <= aBufferMax)

I think we should pass an additional parameter to specify the end of the buffer and compare mBufferPosition against that.
Here is the try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=75ff2926d89c69758b256d81bb4972fbd5a68a5e:  in which I changed
> MOZ_ASSERT(mBufferPosition <= aBufferMax)
to
> MOZ_ASSERT(mBufferPosition >= aBufferMin)

[1]: https://reviewboard.mozilla.org/r/122718/diff/2#1.13

Comment 109

[Dan Minor [:dminor]]

Hi Bao, thanks for continuing to work on this! I wanted to let you know that Paul is away until June, so you won't be able to get a final review on this until then.

Comment 110

[Paul Adenot (:padenot)]

Comment on attachment 8849961 [details]
Bug 1308438 - Part 6: Test AudioBufferSourceNode with negative playback rate and looping.

https://reviewboard.mozilla.org/r/122722/#review150120

Comment 111

[Paul Adenot (:padenot)]

Comment on attachment 8849964 [details]
Bug 1308438 - Part 9: Test AudioBufferSourceNode with playback rate goes from 2 to -2.

https://reviewboard.mozilla.org/r/122728/#review150122

::: dom/media/webaudio/test/test_audioBufferSourceNodePlaybackRateFromTwotoNegativeTwo.html:39
(Diff revision 2)
> +source.start(0);
> +
> +off.startRendering().then(function(buffer1) {
> +  source2.buffer = buf2;
> +  source2.playbackRate.setValueAtTime(-2, off2.currentTime);
> +  source2.playbackRate.linearRampToValueAtTime(2, off2.currentTime + 1);

Yeah this does not really make sense. `currentTime` has no meaning in an `OfflineContext`.

Comment 112

[Paul Adenot (:padenot)]

Comment on attachment 8862473 [details]
Bug 1308438 - Part 10: Test AudioBufferSourceNode with negative playback rate and looping with specified start and end.

https://reviewboard.mozilla.org/r/134388/#review150128

::: dom/media/webaudio/test/test_audioBufferSourceNodeNegativePlaybackRateLoopStartEnd.html:29
(Diff revision 1)
> +    source.playbackRate.value = -1;
> +    return source;
> +  },
> +  createExpectedBuffers: function(context) {
> +    var expectedBuffer = context.createBuffer(1, 2048 * 4, context.sampleRate);
> +    for (var i = 0; i < 1536; ++i) {

In general, we don't like "magic" numbers, can you make it so that we understand why ?

Comment 113

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 114

[Paul Adenot (:padenot)]

I'm picking this up. The patch largely worked, but were bit rotten by Karl's patches that made it possible to use int16_t samples. I've rebased on top of them, but I'll re-review because it was a non-trival rebase. The patch set seems to work still.

Comment 115

[Paul Adenot (:padenot)]

I need clarification on https://github.com/WebAudio/web-audio-api/issues/2072 before continuing here (or I can strictly implement the current text).

Comment 116

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:padenot, could you have a look please?
For more information, please visit auto_nag documentation.