Closed Bug 1113634 Opened 10 years ago Closed 8 years ago

[Web Audio API] automation is ignored with same timing but different type event

Categories

(Core :: Web Audio, defect, P1)

Product:
Core
Component:
Web Audio
Version:
37 Branch
Platform:
x86_64
Windows 8.1
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: gaito, Assigned: dminor)

References

Details

Attachments

(2 files, 2 obsolete files)

automation.patch.v2
(deleted), patch
Details | Diff | Splinter Review
Bug 1113634 - Update mLastComputedValue in AudioEventTimeline when skipping events of same time;
(deleted), text/x-review-board-request
karlt
: review+
Details
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0 Build ID: 20141218030202 Steps to reproduce: <button onclick="start()">start</button> <script> actx=new AudioContext(); osc=actx.createOscillator(); osc.connect(actx.destination); function start(){ var t=actx.currentTime; osc.start(0); osc.frequency.setValueAtTime(100,t) osc.frequency.setValueAtTime(1000,t+1); osc.frequency.setTargetAtTime(100,t+1,1); } </script> ---- this sample is available at: http://www.g200kg.com/demo/test/test-automation-1.html press [start] Actual results: osc sound is fixed to 100Hz. setValueAtTime(1000,t+1) is ignored. Expected results: osc should sweep from 1000Hz to 100Hz at time t+1 from spec. : If one of these events is added at a time where there is already one or more events of a different type, then it will be placed in the list after them, but before events whose times are after the event.
Confirmed 37.0a1 (2015-01-12) Win 7
Status: UNCONFIRMED → NEW
Ever confirmed: true
There might be a dupe of this, but I'm not sure. In any case, this is very important, because this pattern: setValueAtTime(1, t); setTargetAtTime(0, t, 0.03); is an exponential decay, and is what everybody does for envelopes. It might very well be the reason why any kind of Web Audio API synthesis sounds better on Blink/Safari (now that our oscillators are fixed).
Priority: -- → P1
Attached patch automation.patch.v1 (obsolete) (deleted) — — Splinter Review
Attachment #8612774 - Flags: review?(padenot)
Assignee: nobody → baptiste.em
Comment on attachment 8612774 [details] [diff] [review] automation.patch.v1 Review of attachment 8612774 [details] [diff] [review]: ----------------------------------------------------------------- This really needs testing, it's difficult code, we need to be sure we're not regressing anything. I think https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/compiledtest/TestAudioEventTimeline.cpp is pretty straightforward to understand, can you add some test cases? For example, the classic linearRampToValueAtTime(1.0, t); setTargetAtTime(0.0, t, 0.03);, or setValueAtTime(1.0, t); setTargetAtTime(0.0, t, 0.03);, etc. This test is a compiled test, you can run it like so: ./mach cppunittest obj-x86_64-unknown-linux-gnu/dist/bin/TestAudioEventTimeline
Attachment #8612774 - Flags: review?(padenot)
Attached patch automation.patch.v1 (obsolete) (deleted) — — Splinter Review
Attachment #8612774 - Attachment is obsolete: true
Attachment #8613500 - Flags: feedback?(padenot)
Attached patch automation.patch.v2 (deleted) — — Splinter Review
Fix error from the previous patch. There is an error with the mochitest but compiled test works. The compiled test and the mochitest are almost the same, so I think the problem is from the mochitest.
Attachment #8613500 - Attachment is obsolete: true
Attachment #8613500 - Flags: feedback?(padenot)
Attachment #8616571 - Flags: feedback?(padenot)
Comment on attachment 8616571 [details] [diff] [review] automation.patch.v2 Review of attachment 8616571 [details] [diff] [review]: ----------------------------------------------------------------- (clearing the feedback until we know what's up with the mochitest)
Attachment #8616571 - Flags: feedback?(padenot)
Paul - where does this stand? What's the right priority/rank? (Setting rank 15 for now, given P1). Thanks
Rank: 15
Flags: needinfo?(padenot)
I'm fixing this in bug 1184057, but I'm leaving this open as a reminder. I'll close this when it's done.
Flags: needinfo?(padenot)
Blocks: 803392
Assignee: baptiste.em → dminor
Status: NEW → ASSIGNED
Comment on attachment 8779305 [details] Bug 1113634 - Update mLastComputedValue in AudioEventTimeline when skipping events of same time; https://reviewboard.mozilla.org/r/70314/#review68250 Thanks! ::: dom/media/webaudio/AudioEventTimeline.h:353 (Diff revision 1) > private: > template<class TimeType> > void GetValuesAtTimeHelper(TimeType aTime, float* aBuffer, const size_t aSize); > > template<class TimeType> > float GetValuesAtTimeHelperInternal(TimeType aTime, This is getting a bit confusing with all these similarly named methods. I suggest GetValueAtTimeOfEvent() for the new method. ::: dom/media/webaudio/AudioEventTimeline.h:353 (Diff revision 1) > float GetValuesAtTimeHelperInternal(TimeType aTime, > + const AudioTimelineEvent* aNext); The method assumes that aTime is the time of the event, so best to make that clear at the interface by passing only an AudioTimelineEvent. ::: dom/media/webaudio/AudioEventTimeline.cpp:243 (Diff revision 1) > > - aBuffer[bufferIndex] = mComputedValue; > - } > + MOZ_ASSERT(false, "unreached"); > + return 0.0f; These lines shouldn't be necessary. The compiler knows that this can't be reached, and I expect compilation would fail if they could and there was no return statement. ::: dom/media/webaudio/test/test_bug1113634.html:4 (Diff revision 1) > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Test AudioParam.setTargetAtTime</title> Please be more specific about what this is testing, so people don't need to read the test to work it out. The key issue is that the time of the setTarget event is the same as the time of the previous event. ::: dom/media/webaudio/test/test_bug1113634.html:16 (Diff revision 1) > +<script class="testbody" type="text/javascript"> > + > +var V0 = 0.9; > +var V1 = 0.1; > +var T0 = 0; > +var TimeConstant = 10; This is a long time constant given the test only tests the first 0.04s. Please use a shorter constant if the test still passes. If not, please file a bug and reference that here. ::: dom/media/webaudio/test/test_bug1113634.html:40 (Diff revision 1) > + > + source.start(0); > + return gain; > + }, > + createExpectedBuffers: function(context) { > + var T1 = 2048 / context.sampleRate; Not used.
Attachment #8779305 - Flags: review?(karlt) → review+
Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/74b1d84ac6aa Update mLastComputedValue in AudioEventTimeline when skipping events of same time; r=karlt
Backed out for failing Cpp unit test TestAudioEventTimeline: https://hg.mozilla.org/integration/autoland/rev/26e7c3afcbada78633a8c6d6e1e11c7b4cebe23f Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=74b1d84ac6aa4c279cdb84fd213dd1f6e6ec6c70 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=1759987&repo=autoland 15:09:43 INFO - Running TestAudioEventTimeline tests... 15:09:43 INFO - TEST-PASS | Correct default value returned, Got: 10, expected: 10 15:09:43 INFO - TEST-PASS | SetValueAtTime succeeded, Got: 0, expected: 0 15:09:43 INFO - TEST-PASS | SetValueAtTime succeeded, Got: 0, expected: 0 15:09:43 INFO - TEST-PASS | SetValueAtTime succeeded, Got: 0, expected: 0 15:09:43 INFO - TEST-PASS | LinearRampToValueAtTime succeeded, Got: 0, expected: 0 15:09:43 INFO - TEST-PASS | LinearRampToValueAtTime succeeded, Got: 0, expected: 0 15:09:43 INFO - TEST-PASS | ExponentialRampToValueAtTime succeeded, Got: 0, expected: 0 15:09:43 INFO - TEST-PASS | ExponentialRampToValueAtTime succeeded, Got: 0, expected: 0 15:09:43 INFO - TEST-PASS | SetValueCurveAtTime succeeded, Got: 0, expected: 0 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.2, expected: 0.2 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.2, expected: 0.2 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.3, expected: 0.3 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.3, expected: 0.3 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.4, expected: 0.4 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.7, expected: 0.7 15:09:43 INFO - TEST-PASS | Correct value, Got: 1, expected: 1 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.575, expected: 0.575 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.15, expected: 0.15 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.224302, expected: 0.224302 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.33541, expected: 0.33541 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.501555, expected: 0.501555 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.75, expected: 0.75 15:09:43 INFO - TEST-PASS | Correct value, Got: 0.193649, expected: 0.193649 15:09:43 WARNING - TEST-UNEXPECTED-FAIL | Correct value, Got: 0.05, expected: -1
Flags: needinfo?(dminor)
Karl, sadly I missed running the associated unittest the first time around :/. I've updated it to match the new behaviour. It appears we had an incorrect value in TestSpecExample() compared to what is in the current spec. Leaving a needinfo since there doesn't seem to be a clean way to re-request review in MozReview. If there are problems, you should be able to open new issues there. cppunit try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ef3f29bad0
Flags: needinfo?(dminor) → needinfo?(karlt)
Comment on attachment 8779305 [details] Bug 1113634 - Update mLastComputedValue in AudioEventTimeline when skipping events of same time; https://reviewboard.mozilla.org/r/70314/#review68550 ::: dom/media/webaudio/AudioEventTimeline.cpp:201 (Diff revision 3) > } > > if (timeMatchesEventIndex) { > // The time matches one of the events exactly. > MOZ_ASSERT(TimesEqual(aTime, TimeOf(mEvents[eventIndex]))); > + mComputedValue = GetValueAtTimeOfEvent<TimeType>(next); Need to keep passing |&mEvents[eventIndex]| here for the event, as |next| is not updated when there are several events at the same time. The values in TestAudioEventTimeline look correct to me. I don't expect any changes will be necessary with that fix. Please retest with this fixed and with TestAudioEventTimeline unchanged. If that passes we're all good. If not, we can have another look.
Flags: needinfo?(karlt)
Embarrassingly enough, it looks like the compiled test was ok in the first place. As far as I can tell from the spec, a ramp to at the same time as the previous event in effect replaces it. The value that appeared wrong in the spec test was due to the y axis in the spec stopping at zero.
Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e8234e96da51 Update mLastComputedValue in AudioEventTimeline when skipping events of same time; r=karlt
https://hg.mozilla.org/mozilla-central/rev/e8234e96da51
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: