878015 - WebAudio ASSERTION: Something went wrong with rounding to block boundaries: 't == aTo'

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 1

[(no longer active)]

So when the assertion happens, t is 31990 and aTo is 31801.

Here's the log:

2135921024[100564070]: MediaStreamGraph 11ddff880 ForceShutdown
505430016[112f9b380]: Media graph global underrun detected
505430016[112f9b380]: Updating current time to 0.000001 (real 286.443176, mStateComputedTime 0.000001)
505430016[112f9b380]: Time did not advance
505430016[112f9b380]: Adding media stream 11bc4d420 to the graph
505430016[112f9b380]: Media graph 11ddff880 computing blocking for time 0.000001
505430016[112f9b380]: MediaStream 11bc4d420 is blocked due to explicit blocker
505430016[112f9b380]: Media graph 11ddff880 computed blocking for interval 0.000001 to 0.030328
505430016[112f9b380]: MediaStream 11bc4d420 will finish
###!!! ASSERTION: Something went wrong with rounding to block boundaries: 't == aTo', file ../../../content/media/MediaStreamGraph.cpp, line 972
505430016[112f9b380]: MediaStreamGraph 11ddff880 waiting for main thread cleanup
2135921024[100564070]: Stopping threads for MediaStreamGraph 11ddff880
2135921024[100564070]: Removing media stream 11bc4d420 from the graph
2135921024[100564070]: MediaStreamGraph 11ddff880 destroyed

I don't know how this timing stuff works, and with a quick look, I'm not sure where the thing that we assert there is enforced.  roc, can you please take a look?  Thanks!

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't have time to look into this now, and it doesn't seem all that high priority.

Comment 3

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

I output the loop values of t and next in MediaStreamGraphImpl::ProduceDataForStreamsBlockByBlock():

t=  29530, next=  29908, (next-t)=    378
t=  29908, next=  30097, (next-t)=    189
t=  30097, next=  30287, (next-t)=    190
t=  30287, next=  30665, (next-t)=    378
t=  30665, next=  30855, (next-t)=    190
t=  30855, next=  31233, (next-t)=    378
t=  31233, next=  31422, (next-t)=    189
t=  31422, next=  31612, (next-t)=    190
t=  31612, next=  31990, (next-t)=    378

It looks like we should pass t instead of t+1 to RoundUpToAudioBlock() so we have a constant (next-t) for ps->ProduceOutput(t, next), since t and t+1 sometimes can round up to different values.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Fix level skipping when passing t+1 to RoundUpToAudioBlock.

https://tbpl.mozilla.org/?tree=Try&rev=a9602a460a56
It looks like no regression.

Comment 6

[(no longer active)]

Comment on attachment 777593 [details] [diff] [review]
Fix level skipping when passing t+1 to RoundUpToAudioBlock.

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

I'd rather have roc review this, as I'm not sure if I understand all of the implications of this correctly.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 777593 [details] [diff] [review]
Fix level skipping when passing t+1 to RoundUpToAudioBlock.

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

::: content/media/MediaStreamGraph.cpp
@@ +971,5 @@
>                                                          GraphTime aTo)
>  {
>    GraphTime t = aFrom;
>    while (t < aTo) {
> +    GraphTime next = RoundUpToAudioBlock(aSampleRate, t);

The idea of using t+1 was to ensure that we make some progress. It seems to me this patch creates the possibility of next == t, in which case we're stuck in an infinite loop.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

I am kinda curious about the return value of RoundUpToAudioBlock().

It can be rephrased as A+B+C where (ignore the precision error)
A = (((blocksAtIdealaSampleRate + 1)*WEBAUDIO_BLOCK_SIZE) << MEDIA_TIME_FRAC_BITS) /aSampleRate
B = aSampleRate/aSampleRate
C = 1/aSampleRate

A stands for the length of blocks (1 block = 128 samples) in GraphTime (1 second = 2^20 GraphTime). What do B and c mean?

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 777593 [details] [diff] [review]
> Fix level skipping when passing t+1 to RoundUpToAudioBlock.
> 
> Review of attachment 777593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaStreamGraph.cpp
> @@ +971,5 @@
> >                                                          GraphTime aTo)
> >  {
> >    GraphTime t = aFrom;
> >    while (t < aTo) {
> > +    GraphTime next = RoundUpToAudioBlock(aSampleRate, t);
> 
> The idea of using t+1 was to ensure that we make some progress. It seems to
> me this patch creates the possibility of next == t, in which case we're
> stuck in an infinite loop.

Here is the return value of RoundUpToAudioBlock().

((((blocksAtIdealaSampleRate + 1)*WEBAUDIO_BLOCK_SIZE) << MEDIA_TIME_FRAC_BITS)
     + aSampleRate - 1)/aSampleRate;

I think (blocksAtIdealaSampleRate + 1) means 'go to next block' and should ensure the return value is greater than the input aTime.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're right. I think we shouldn't have that + 1 in RoundUpToAudioBlock.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Is the patch good to go?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 777593 [details] [diff] [review]
Fix level skipping when passing t+1 to RoundUpToAudioBlock.

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

We need to fix RoundUpToAudioBlock as discussed in the comments.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

jwwang, please see if this fixes the bug :-)

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

The test case passes.

Btw, the comment should change to
Returns smallest value of t such that
TimeToTicksRoundDown(aSampleRate, t) is a multiple of WEBAUDIO_BLOCK_SIZE

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 783609 [details] [diff] [review]
fix

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

::: content/media/MediaStreamGraph.cpp
@@ +962,2 @@
>  {
> +  TrackTicks ticks = TimeToTicksRoundUp(aSampleRate, aTime);

I think it should be TimeToTicksRoundDown() here so that aTime won't round to "next next" block in some corner cases.

RoundUpToNextAudioBlock(709064,1) = 190
TimeToTicksRoundUp(709064,1) = 1
TimeToTicksRoundUp(709064, 188) = 128

The smallest t when aTime=1 such that
floor(TimeToTicksRoundUp(aSampleRate, t)/WEBAUDIO_BLOCK_SIZE) >
floor(TimeToTicksRoundUp(aSampleRate, aTime)/WEBAUDIO_BLOCK_SIZE)
is 188 instead of 190.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, my calculations show that RoundUpToNextAudioBlock(709064,1) should be ceil(709064*128/(1024*1024)) = 87. What am I doing wrong?

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

I think it should be 128(tick/block) * 2^20(mediatime/sec) / 709064(tick/sec) = 189.288594542 mediatime/block

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Thanks for pointing that out, jwwang, although the fix is a little more complicated.

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

Now we have
RoundUpToNextAudioBlock(709064,   1) = 188
RoundUpToNextAudioBlock(709064, 187) = 188
RoundUpToNextAudioBlock(709064, 188) = 378

Since a block takes 189.288594542 mediatime, for each t where 0 <= t <= 189 (time within block 0), RoundUpToNextAudioBlock(t) should be 190 (the first integral mediatime within block 1).

Going from 187 to 188 mediatime doesn't really go to next block.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It does, according to TimeToTicksRoundUp(), which is what matters here.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ee60e77157

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e1ee60e77157