Closed Bug 864171 Opened 8 years ago Closed 6 years ago
Problem in converting retrospective Audio
Param event times to track ticks on context that has had no nodes
840 bytes, text/html
1.92 KB, text/html
MozReview Request: bug 864171 move "extra" time accounting for AudioContext with no nodes to destination stream r?padenot
40 bytes, text/x-review-board-request
4.17 KB, patch
|Details | Diff | Splinter Review|
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.
Your testcase looks correct. I don't know why it doesn't work.
Does the current time of the graph on the MSG thread move forward if there are no streams being played back?
I haven't tested it, but it certainly should.
(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.
(In reply to comment #5) > NSPR_LOG_MODULES=MediaStreamGraph:5 Thanks, I'll get some logs the next time that I look at this.
Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.
Component: Video/Audio → Web Audio
Assigning to Paul for investigation.
Assignee: nobody → paul
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.
Assignee: padenot → karlt
Status: NEW → ASSIGNED
Summary: Problem in converting AudioParam times to track ticks → Problem in converting retrospective AudioParam event times to track ticks on context that has had no nodes
bug 864171 move "extra" time accounting for AudioContext with no nodes to destination stream r?padenot
Attachment #8664781 - Flags: review?(padenot)
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.
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
(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
Depends on: 1053011
I think I did not have the right patch set applied indeed, sorry about that.
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
Attachment #8664781 - Flags: review?(padenot) → review+
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.
Attachment #8665300 - Flags: review?(padenot) → review+
(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.
You need to log in before you can comment on or make changes to this bug.