Closed
Bug 864171
Opened 12 years ago
Closed 9 years ago
Problem in converting retrospective AudioParam event times to track ticks on context that has had no nodes
Categories
(Core :: Web Audio, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: karlt)
References
Details
Attachments
(4 files)
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.
Reporter | ||
Updated•12 years ago
|
Flags: needinfo?(roc)
Your testcase looks correct. I don't know why it doesn't work.
Flags: needinfo?(roc)
Reporter | ||
Comment 2•12 years ago
|
||
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.
Reporter | ||
Comment 4•12 years ago
|
||
(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.
NSPR_LOG_MODULES=MediaStreamGraph:5
Reporter | ||
Comment 6•12 years ago
|
||
(In reply to comment #5)
> NSPR_LOG_MODULES=MediaStreamGraph:5
Thanks, I'll get some logs the next time that I look at this.
Reporter | ||
Comment 7•11 years ago
|
||
Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.
Component: Video/Audio → Web Audio
Updated•11 years ago
|
Priority: -- → P2
Assignee | ||
Comment 9•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Comment 10•9 years ago
|
||
bug 864171 move "extra" time accounting for AudioContext with no nodes to destination stream r?padenot
Attachment #8664781 -
Flags: review?(padenot)
Assignee | ||
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8665300 -
Flags: review?(padenot)
Comment 13•9 years ago
|
||
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
Assignee | ||
Comment 14•9 years ago
|
||
(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
Comment 15•9 years ago
|
||
I think I did not have the right patch set applied indeed, sorry about that.
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Comment 18•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite+
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9e2297e3f3d9
https://hg.mozilla.org/mozilla-central/rev/edabd5743aeb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•