864171 - Problem in converting retrospective AudioParam event times to track ticks on context that has had no nodes

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[(no longer active)]

Attachment: testcase

So, right now, if we create a context (which creates the destination stream immediately), wait for 5 seconds, and then set up scheduled events on an AudioParam on something in the graph, the passed time doesn't get into account.  I think this happens because the graph's current time doesn't progress if there's nothing being played back, but I'm not quite sure.

See the test case.  I would expect the computedGain to be around 0.5 and 1.0, but it's 0 both of the times.

Comment 1

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

Your testcase looks correct. I don't know why it doesn't work.

Comment 2

[(no longer active)]

Does the current time of the graph on the MSG thread move forward if there are no streams being played back?

Comment 3

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

I haven't tested it, but it certainly should.

Comment 4

[(no longer active)]

(In reply to comment #3)
> I haven't tested it, but it certainly should.

Can you please give me some pointers on how to test this?  I'm sort of stuck here and I'm not sure how these times blocking times etc are supposed to work.

Comment 5

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

NSPR_LOG_MODULES=MediaStreamGraph:5

Comment 6

[(no longer active)]

(In reply to comment #5)
> NSPR_LOG_MODULES=MediaStreamGraph:5

Thanks, I'll get some logs the next time that I look at this.

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

Assigning to Paul for investigation.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: retrospective timeline event testcase 2

Another testcase that I wrote before finding this bug.

AudioNodeStream::FractionalTicksFromDestinationTime() was not designed to
handle negative times that result from subtracting extra time in
DOMTimeToStreamTime().  That could probably be changed to cope with negative
times, but having DOMTimeToStreamTime and StreamTimeToDOMTime sprinkled
everywhere adds to confusion.  Better to deal with this on the destination
node stream so that everything is in one place.

DOMTimeToStreamTime and StreamTimeToDOMTime weren't introduced until bug 952893, which is after this bug was filed, so its interesting to find that this bug already existed.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 864171 move "extra" time accounting for AudioContext with no nodes to destination stream r?padenot

bug 864171 move "extra" time accounting for AudioContext with no nodes to destination stream r?padenot

Comment 11

[Karl Tomlinson (:karlt)]

I can make a patch to remove the DOMTimeToStreamTime and StreamTimeToDOMTime that are identity functions are the first patch, but I can do that on top of the patch in bug 1200579, and the patch there currently needs this first patch.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: test for and bug 1053011

Comment 13

[Paul Adenot (:padenot)]

Testing this patch, I see:

6 INFO TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_scriptProcessorNode_playbackTime1.html | playbackTime 0.005804988662131519 beyond expected minimum 0.10580498866213149

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #13)
> Testing this patch, I see:
> 
> 6 INFO TEST-UNEXPECTED-FAIL |
> dom/media/webaudio/test/test_scriptProcessorNode_playbackTime1.html |
> playbackTime 0.005804988662131519 beyond expected minimum 0.10580498866213149

I'm not seeing that result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4da2fc3470f9&filter-tier=1

Looking at that test made me aware of bug 1208318, but I don't think that is
the cause of your result.

Did the patch apply cleanly, or did you pull?
hg pull -ur 6a8cd2be1e23f7ae4fac7da04e4fed19e59e5ab6 https://reviewboard-hg.mozilla.org/gecko

Note that this applies on top of
https://reviewboard-hg.mozilla.org/gecko/rev/03f4dbe6c85c

Comment 15

[Paul Adenot (:padenot)]

I think I did not have the right patch set applied indeed, sorry about that.

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8664781 [details]
MozReview Request: bug 864171 move "extra" time accounting for AudioContext with no nodes to destination stream r?padenot

https://reviewboard.mozilla.org/r/20031/#review18197

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8665300 [details] [diff] [review]
test for  and bug 1053011

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

I like tests that use OfflineAudioContext (because they run faster and directly produce a buffer that can be inspected), but I don't know what style is preferred in the web platform tests. 

In any case, the test look good.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2297e3f3d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/edabd5743aeb

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #17)
> I like tests that use OfflineAudioContext (because they run faster and
> directly produce a buffer that can be inspected)

Yes, I agree.  In this case though, a realtime context was necessary to modify the parameter after the graph had started, and the bug was in code only effective for realtime contexts.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e2297e3f3d9
https://hg.mozilla.org/mozilla-central/rev/edabd5743aeb