Closed Bug 864171 Opened 8 years ago Closed 6 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)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ehsan, Assigned: karlt)

References

Details

Attachments

(4 files)

Attached 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
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.
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
(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.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/9e2297e3f3d9
https://hg.mozilla.org/mozilla-central/rev/edabd5743aeb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1209286
You need to log in before you can comment on or make changes to this bug.