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

RESOLVED FIXED in Firefox 44

Status

()

defect
P2
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: karlt)

Tracking

Trunk
mozilla44
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(4 attachments)

Posted file 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.
Flags: needinfo?(roc)
Your testcase looks correct. I don't know why it doesn't work.
Flags: needinfo?(roc)
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.
NSPR_LOG_MODULES=MediaStreamGraph:5
(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
Priority: -- → P2
Assignee

Comment 9

4 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

4 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

4 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

4 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

4 years ago
Attachment #8665300 - Flags: review?(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
Assignee

Comment 14

4 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
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+
Assignee

Comment 19

4 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

4 years ago
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/9e2297e3f3d9
https://hg.mozilla.org/mozilla-central/rev/edabd5743aeb
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee

Updated

4 years ago
Blocks: 1209286
You need to log in before you can comment on or make changes to this bug.