1414366 - ConstantSourceNode start/stop incorrect

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P3)

Description

[toy.raymond]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.75 Safari/537.36

Steps to reproduce:

See https://wpt.fyi/webaudio/chrome/the-constantsourcenode-interface/constant-source-output.html


Actual results:

ConstantSourceNode start/stop doesn't appear to start or stop at the correct frames.  The test starts the node at frame 10 so the first 10 frames of output should be 0 but they're 1.  LIkewise, stop didn't cause the node to stop at the desired frame.


Expected results:

The non-zero output of the node should have started and stopped according to the start and stop calls.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bug 1414366 - Don't assume aStart is aligned on block boundary in AudioParamTimeline::GetValuesAtTime.

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

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1414366 - Take into account start and end time when processing ConstantSource output.

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

Comment 3

[Paul Adenot (:padenot)]

Attachment: Bug 1414366 - Update WPT expectations.

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

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8993416 [details]
Bug 1414366 - Update WPT expectations.

https://reviewboard.mozilla.org/r/258168/#review265292

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8993414 [details]
Bug 1414366 - Don't assume aStart is aligned on block boundary in AudioParamTimeline::GetValuesAtTime.

https://reviewboard.mozilla.org/r/258164/#review266252

::: dom/media/webaudio/AudioParamTimeline.h:147
(Diff revision 1)
>    MOZ_ASSERT(aSize <= WEBAUDIO_BLOCK_SIZE);
>    MOZ_ASSERT(aSize == 1 || !HasSimpleValue());
>  
>    // Mix the value of the AudioParam itself with that of the AudioNode inputs.
>    BaseClass::GetValuesAtTime(aTime, aBuffer, aSize);
> +  uint32_t blockOffset = aTime % WEBAUDIO_BLOCK_SIZE;

Please place this inside the (mStream) block, if keeping this line.
Don't truncate when storing in blockOffset if this will be converted to size_t below.

I wonder whether a aStartOffset parameter may be simpler, given the caller already knows this, and the function is inline.

Switching from aStartOffset, aSize to aStartOffset, aEndOffset may also help.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8993415 [details]
Bug 1414366 - Take into account start and end time when processing ConstantSource output.

https://reviewboard.mozilla.org/r/258166/#review266254

::: dom/media/webaudio/ConstantSourceNode.cpp:89
(Diff revision 1)
>                      AudioBlock* aOutput,
>                      bool* aFinished) override
>    {
>      MOZ_ASSERT(mSource == aStream, "Invalid source stream");
>  
> -    StreamTime ticks = mDestination->GraphTimeToStreamTime(aFrom);
> +    StreamTime startTicks = mDestination->GraphTimeToStreamTime(aFrom);

I initially wondered (incorrectly) whether startTicks was the ticks point of the start time.

Does this need to change from "ticks"?

If so, would streamPosition be suitable?

::: dom/media/webaudio/ConstantSourceNode.cpp:104
(Diff revision 1)
>        float* output = aOutput->ChannelFloatsForWrite(0);
> +      uint32_t writeOffset = 0;
>  
> -      if (mOffset.HasSimpleValue()) {
> -        for (uint32_t i = 0; i < WEBAUDIO_BLOCK_SIZE; ++i) {
> -          output[i] = mOffset.GetValueAtTime(aFrom, 0);
> +      if (startTicks < mStart) {
> +        uint32_t count = mStart - startTicks;
> +        std::fill(output, output + count, 0.0);

0.0f

fill_n if using count.

::: dom/media/webaudio/ConstantSourceNode.cpp:109
(Diff revision 1)
> -          output[i] = mOffset.GetValueAtTime(aFrom, 0);
> +        std::fill(output, output + count, 0.0);
> +        writeOffset += count;
> -        }
> +      }
> +
> +      MOZ_ASSERT(startTicks + writeOffset >= mStart);
> +      uint32_t count =  std::min(startTicks + WEBAUDIO_BLOCK_SIZE, mStop) - (startTicks + writeOffset);

What happens when mStart > mStop?

Perhaps

  int32_t count =
    std::min<StreamTime>(mStop - ticks, WEBAUDIO_BLOCK_SIZE) - writeOffset;

but I wonder whether this might be easier to understand if working with
startOffset and stopOffset, similar to OscillatorNode, instead of writeOffset
and count.

::: dom/media/webaudio/ConstantSourceNode.cpp:120
(Diff revision 1)
> +      }
> +
> +      writeOffset += count;
> +
> +      if (startTicks + writeOffset >= mStop) {
> +        std::fill(output + writeOffset, output + WEBAUDIO_BLOCK_SIZE, 0.0);

0.0f

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8993414 [details]
Bug 1414366 - Don't assume aStart is aligned on block boundary in AudioParamTimeline::GetValuesAtTime.

https://reviewboard.mozilla.org/r/258164/#review266252

> Please place this inside the (mStream) block, if keeping this line.
> Don't truncate when storing in blockOffset if this will be converted to size_t below.
> 
> I wonder whether a aStartOffset parameter may be simpler, given the caller already knows this, and the function is inline.
> 
> Switching from aStartOffset, aSize to aStartOffset, aEndOffset may also help.

I hadn't quite twigged that the remainder operator was just a bitmask, and so I guess an aStartOffset parameter might be somewhat pointless.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1414366 - Fix a few `should` constructs in `constant-source-output.html`.

The test name was passed as a third argument to `ArrayBuffer.slice`, silently
ignored.

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

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1414366 - Test ConstantSourceNode where stop has been called with the parameter lower or equal than the parameter to start.

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

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8993414 [details]
Bug 1414366 - Don't assume aStart is aligned on block boundary in AudioParamTimeline::GetValuesAtTime.

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

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8993415 [details]
Bug 1414366 - Take into account start and end time when processing ConstantSource output.

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

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8993416 [details]
Bug 1414366 - Update WPT expectations.

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

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8993415 [details]
Bug 1414366 - Take into account start and end time when processing ConstantSource output.

https://reviewboard.mozilla.org/r/258166/#review266408

::: dom/media/webaudio/ConstantSourceNode.cpp:109
(Diff revision 1)
> -          output[i] = mOffset.GetValueAtTime(aFrom, 0);
> +        std::fill(output, output + count, 0.0);
> +        writeOffset += count;
> -        }
> +      }
> +
> +      MOZ_ASSERT(startTicks + writeOffset >= mStart);
> +      uint32_t count =  std::min(startTicks + WEBAUDIO_BLOCK_SIZE, mStop) - (startTicks + writeOffset);

I've tried rewriting this with offset but it wasn't clearer I thought, so I left it this way.

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8993415 [details]
Bug 1414366 - Take into account start and end time when processing ConstantSource output.

https://reviewboard.mozilla.org/r/258166/#review266254

> I initially wondered (incorrectly) whether startTicks was the ticks point of the start time.
> 
> Does this need to change from "ticks"?
> 
> If so, would streamPosition be suitable?

I've restored "ticks".

> What happens when mStart > mStop?
> 
> Perhaps
> 
>   int32_t count =
>     std::min<StreamTime>(mStop - ticks, WEBAUDIO_BLOCK_SIZE) - writeOffset;
> 
> but I wonder whether this might be easier to understand if working with
> startOffset and stopOffset, similar to OscillatorNode, instead of writeOffset
> and count.

Silence must be output then, I've added a case for this, we can just set the block to be null.

Comment 15

[Paul Adenot (:padenot)]

Comment on attachment 8993414 [details]
Bug 1414366 - Don't assume aStart is aligned on block boundary in AudioParamTimeline::GetValuesAtTime.

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

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8993415 [details]
Bug 1414366 - Take into account start and end time when processing ConstantSource output.

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

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8993416 [details]
Bug 1414366 - Update WPT expectations.

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

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8994839 [details]
Bug 1414366 - Fix a few `should` constructs in `constant-source-output.html`.

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

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 8994840 [details]
Bug 1414366 - Test ConstantSourceNode where stop has been called with the parameter lower or equal than the parameter to start.

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

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8993415 [details]
Bug 1414366 - Take into account start and end time when processing ConstantSource output.

https://reviewboard.mozilla.org/r/258166/#review266534

::: dom/media/webaudio/ConstantSourceNode.cpp:97
(Diff revision 3)
>        return;
>      }
>  
> -    if (ticks + WEBAUDIO_BLOCK_SIZE <= mStart || ticks >= mStop) {
> +    if (ticks + WEBAUDIO_BLOCK_SIZE <= mStart ||
> +        ticks >= mStop ||
> +        (mStop != STREAM_TIME_MAX && mStop <= mStart)) {

WebAudioUtils::IsTimeValid() provides that
mStart < STREAM_TIME_MAX = MEDIA_TIME_MAX
and so "mStop != STREAM_TIME_MAX" is not required.

::: dom/media/webaudio/ConstantSourceNode.cpp:105
(Diff revision 3)
> -        for (uint32_t i = 0; i < WEBAUDIO_BLOCK_SIZE; ++i) {
> -          output[i] = mOffset.GetValueAtTime(aFrom, 0);
> +        uint32_t count = mStart - ticks;
> +        std::fill_n(output, count, 0.0f);

Please MOZ_ASSERT(mStart - ticks <= WEBAUDIO_BLOCK_SIZE).

::: dom/media/webaudio/ConstantSourceNode.cpp:110
(Diff revision 3)
> +      MOZ_ASSERT(ticks + writeOffset >= mStart);
> +      uint32_t count = std::min<StreamTime>(ticks + WEBAUDIO_BLOCK_SIZE, mStop) - (ticks + writeOffset);

Please MOZ_ASSERT(mStop >= ticks + writeOffset)
or MOZ_ASSERT(mStop - ticks >= writeOffset).

ticks and mStop are int64_t.  WEBAUDIO_BLOCK_SIZE is uint32_t.  That means
that the parameters to min are both int64_t and so the <StreamTime> template
parameter is not required.  <StreamTime> is required if you avoid the double
use of ticks in the expression:

  std::min<StreamTime>(mStop - ticks, WEBAUDIO_BLOCK_SIZE) - writeOffset

Please wrap to stay within 80 columns.

::: dom/media/webaudio/ConstantSourceNode.cpp:121
(Diff revision 3)
> -        mOffset.GetValuesAtTime(ticks, output, WEBAUDIO_BLOCK_SIZE);
> +        mOffset.GetValuesAtTime(ticks + writeOffset, output + writeOffset, count);
> +      }
> +
> +      writeOffset += count;
> +
> +      if (ticks + writeOffset >= mStop) {

The conditional is not required because fill_n will do nothing when writeOffset == WEBAUDIO_BLOCK_SIZE.

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8994839 [details]
Bug 1414366 - Fix a few `should` constructs in `constant-source-output.html`.

https://reviewboard.mozilla.org/r/259382/#review266548

::: testing/web-platform/tests/webaudio/the-audio-api/the-constantsourcenode-interface/constant-source-output.html:67
(Diff revision 2)
>  
>                let prefix = 'start/stop: ';
> -              should(actual.slice(
> +              should(actual.slice(0, startFrame),
> -                         0, startFrame,
> -                         prefix + 'ConstantSourceNode frames [0, ' +
> +                     prefix + 'ConstantSourceNode frames [0, ' +
> -                             startFrame + ')'))
> +                     startFrame + ')')

Indent startFrame 4 spaces.

::: testing/web-platform/tests/webaudio/the-audio-api/the-constantsourcenode-interface/constant-source-output.html:72
(Diff revision 2)
> -                             startFrame + ')'))
> +                     startFrame + ')')
>                    .beConstantValueOf(0);
>  
> -              should(actual.slice(
> -                         startFrame, stopFrame,
> -                         prefix + 'ConstantSourceNode frames [' + startFrame +
> +              should(actual.slice(startFrame, stopFrame),
> +                     prefix + 'ConstantSourceNode frames [' +
> +                     startFrame + ', ' + stopFrame + ')')

Indent startFrame 4 spaces.

::: testing/web-platform/tests/webaudio/the-audio-api/the-constantsourcenode-interface/constant-source-output.html:77
(Diff revision 2)
> -                  prefix + 'ConstantSourceNode frames [' + stopFrame + ', ' +
> -                      renderFrames + ')')
> +                  prefix + 'ConstantSourceNode frames [' + stopFrame +
> +                  ', ' + renderFrames + ')')

Don't make unncessary whitespace changes.

::: testing/web-platform/tests/webaudio/the-audio-api/the-constantsourcenode-interface/constant-source-output.html:113
(Diff revision 2)
> -              should(
> +              should(actual.slice(rampEndFrame),
> -                  actual.slice(rampEndFrame),
> -                  prefix + 'ConstantSourceNode after ramp')
> +                     prefix + 'ConstantSourceNode after ramp')
> -                  .beConstantValueOf(1);
> +                .beConstantValueOf(1);

This file uses 4 spaces on line continuations.
i.e. for .beConstant

There is a small improvement is readability here and so I'm OK with such whitespace change here, if it was intentional and 4 spaces are used.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8994840 [details]
Bug 1414366 - Test ConstantSourceNode where stop has been called with the parameter lower or equal than the parameter to start.

https://reviewboard.mozilla.org/r/259384/#review266554

::: testing/web-platform/tests/webaudio/the-audio-api/the-constantsourcenode-interface/constant-source-output.html:47
(Diff revision 2)
> +        context.startRendering()
> +          .then(function(buffer) {
> +            let actual = buffer.getChannelData(0);

4 spaces indent to .then.
2 spaces to let.

::: testing/web-platform/tests/webaudio/the-audio-api/the-constantsourcenode-interface/constant-source-output.html:51
(Diff revision 2)
> +
> +        context.startRendering()
> +          .then(function(buffer) {
> +            let actual = buffer.getChannelData(0);
> +            should(actual,
> +                   "ConstantSourceNode with stop before start must output silence").beConstantValueOf(0);

Wrap .beConstant, 4 spaces in from should.

::: testing/web-platform/tests/webaudio/the-audio-api/the-constantsourcenode-interface/constant-source-output.html:56
(Diff revision 2)
> +      audit.define('stop equal to start', (task, should) => {
> +          let context = new OfflineAudioContext(1, renderFrames, sampleRate);

2 spaces to the function body.

::: testing/web-platform/tests/webaudio/the-audio-api/the-constantsourcenode-interface/constant-source-output.html:67
(Diff revision 2)
> +
> +          context.startRendering()
> +            .then(function(buffer) {
> +              let actual = buffer.getChannelData(0);
> +              should(actual,
> +                     "ConstantSourceNode with stop equal to start must output silence").beConstantValueOf(0);

Wrap.

Comment 23

[Paul Adenot (:padenot)]

Comment on attachment 8993415 [details]
Bug 1414366 - Take into account start and end time when processing ConstantSource output.

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

Comment 24

[Paul Adenot (:padenot)]

Comment on attachment 8993416 [details]
Bug 1414366 - Update WPT expectations.

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

Comment 25

[Paul Adenot (:padenot)]

Comment on attachment 8994839 [details]
Bug 1414366 - Fix a few `should` constructs in `constant-source-output.html`.

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

Comment 26

[Paul Adenot (:padenot)]

Comment on attachment 8994840 [details]
Bug 1414366 - Test ConstantSourceNode where stop has been called with the parameter lower or equal than the parameter to start.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35087f4f946b
Don't assume aStart is aligned on block boundary in AudioParamTimeline::GetValuesAtTime. r=karlt
https://hg.mozilla.org/integration/autoland/rev/087b66f9adeb
Take into account start and end time when processing ConstantSource output. r=karlt
https://hg.mozilla.org/integration/autoland/rev/10df871104a0
Update WPT expectations. r=karlt
https://hg.mozilla.org/integration/autoland/rev/1b109587de90
Fix a few `should` constructs in `constant-source-output.html`.  r=karlt
https://hg.mozilla.org/integration/autoland/rev/7bbc44c10608
Test ConstantSourceNode where stop has been called with the parameter lower or equal than the parameter to start. r=karlt

Comment 28

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/35087f4f946b
https://hg.mozilla.org/mozilla-central/rev/087b66f9adeb
https://hg.mozilla.org/mozilla-central/rev/10df871104a0
https://hg.mozilla.org/mozilla-central/rev/1b109587de90
https://hg.mozilla.org/mozilla-central/rev/7bbc44c10608

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12208 for changes under testing/web-platform/tests

Comment 30

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like you didn't run "mach wpt-manifest-update" (causing me to see a diff as a result of this patch, when I run it).