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

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: posidron, Assigned: roc)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla25
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

6 years ago
Posted file testcase
No description provided.

Comment 1

6 years ago
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!
Flags: needinfo?(roc)
I don't have time to look into this now, and it doesn't seem all that high priority.
Flags: needinfo?(roc)

Comment 3

6 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
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 6

6 years ago
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.
Attachment #777593 - Flags: review?(ehsan) → review?(roc)
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.
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?
(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.
You're right. I think we shouldn't have that + 1 in RoundUpToAudioBlock.
Is the patch good to go?
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.
Attachment #777593 - Flags: review?(roc) → review-
Posted patch fix (obsolete) — Splinter Review
jwwang, please see if this fixes the bug :-)
Assignee: nobody → roc
Attachment #783609 - Flags: review?(paul)
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
Attachment #783609 - Flags: review?(paul) → review+
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.
Hmm, my calculations show that RoundUpToNextAudioBlock(709064,1) should be ceil(709064*128/(1024*1024)) = 87. What am I doing wrong?
I think it should be 128(tick/block) * 2^20(mediatime/sec) / 709064(tick/sec) = 189.288594542 mediatime/block
Posted patch fix v2Splinter Review
Thanks for pointing that out, jwwang, although the fix is a little more complicated.
Attachment #777593 - Attachment is obsolete: true
Attachment #783609 - Attachment is obsolete: true
Attachment #784361 - Flags: review?(paul)
Attachment #784361 - Flags: review?(paul) → review+
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.
It does, according to TimeToTicksRoundUp(), which is what matters here.
https://hg.mozilla.org/mozilla-central/rev/e1ee60e77157
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.