AudioParam is one of the most complicated things in the web audio spec. I'm working on a patch to implement this object, which would allow us to implement a whole new set of the web audio objects. The tricky things here is that at least until the offline context stuff gets spec'ed and we implement them, there's no way for us to test AudioParam, which sucks, especially since it's a very complicated class. I have thought about ways of testing this. One obvious way is to add a ChromeOnly method on an interface to use it as a testing hook, but I would like to avoid that if possible. Another possibility is to test AudioParam through a compiled test, which is something that I like since most of AudioParam is pretty self-contained. So what I've done so far is to put the meat of AudioParam into a base class (called AudioEventTimeline), templatize it based on one type which is a thin wrapper around Float32Array in our real implementation and will be set to a type which will wrap a plain float array for our testing, and then just use that base class for almost all of the AudioParam implementation. Given that, I can stand up a compiled test which #includes AudioEventTimeline.h and does everything that it needs to. I have a patch which I was iterating on to get the basics right. Submitting it for feedback on the architecture that I described above. I don't need a more detailed review at this time.

Comment on attachment 673066 [details] [diff] [review] WIP So this seems fine as a plan, but you'll need to be careful to expose methods on your wrapper thing for tracing the typed array, etc...

This does not yet trace the JS objects etc, since setValueCurveAtTime is not implemented yet.

Comment on attachment 675299 [details] [diff] [review] Patch (v1) I'm not sure I follow the "creates an uninitialized object" bit. Seems like it will call the TypedArray<float, JS_GetFloat32ArrayData, JS_GetObjectAsFloat32Array, JS_NewFloat32Array> constructor with null and null, which will call the TypedArray_base<float,JS_GetObjectAsFloat32Array> constructor, which will do, if you expand it all out: mObj = JS_GetObjectAsFloat32Array(nullptr, nullptr, &mLength, &mData); which I fully expect to crash.

(In reply to Boris Zbarsky (:bz) from comment #4) > Comment on attachment 675299 [details] [diff] [review] > Patch (v1) > > I'm not sure I follow the "creates an uninitialized object" bit. Seems like > it will call the TypedArray<float, JS_GetFloat32ArrayData, > JS_GetObjectAsFloat32Array, JS_NewFloat32Array> constructor with null and > null, which will call the TypedArray_base<float,JS_GetObjectAsFloat32Array> > constructor, which will do, if you expand it all out: > > mObj = JS_GetObjectAsFloat32Array(nullptr, nullptr, &mLength, &mData); > > which I fully expect to crash. Oh, I did not realize that (and I have not been able to run this code in a DOM object yet, as I don't yet have any way to create those objects... Is there any good way to create a Float32Array object with an invalid state that can be differentiated later than a regular object?

Not really, no. But you could perhaps use a Maybe<Float32Array>?

Good idea, I'll do that.

In addition to fixing that, this patch is actually written in C++. ;-)

Try run for 63775d21606a is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=63775d21606a Results (out of 16 total builds): success: 16 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-63775d21606a

Comment on attachment 675653 [details] [diff] [review] Patch (v2) >+++ b/content/media/webaudio/AudioEventTimeline.h >+ static bool IsValid(float value) >+ { >+ return !MOZ_DOUBLE_IS_NaN(value) && >+ !MOZ_DOUBLE_IS_INFINITE(value); >+ } How about just: return MOZ_DOUBLE_IS_FINITE(value); Note that I haven't carefully checked correctness of GetValueAtTime against the spec. Please let me know if I should. That said, the whole strict equality testing on floats thing worries me... >+ static float LinearInterpolate(float t0, float v0, float t1, float v1, float t) >+ { >+ return v0 + (v1 - v0) * ((t - t0) / (t1 - t0)); >+ } I guess you already ensured that t1-t0 is nonzero (or more precisely that t1 != t0) in GetValueAtTime? Because divide-by-0 is not good. :( Maybe we'll get NaN, maybe we'll get a signal. >+ static float ExponentialInterpolate(float t0, float v0, float t1, float v1, float t) >+ { >+ return v0 * powf(v1 / v0, (t - t0) / (t1 - t0)); >+ } So the goal here is a function that's v0 at t0, v1 at t1 and has the form A*exp(B(t-t0)) ? >+ // Make sure that non-curve events don't fall within the duration of a >+ // curve event. >+ for (unsigned i = 0; i < mEvents.Length(); ++i) { >+ if (mEvents[i].mType == Event::Type::SetValueCurve && >+ mEvents[i].mTime >= aEvent.mTime && >+ (mEvents[i].mTime + mEvents[i].mDuration) <= aEvent.mTime) { So what this is checking is that mEvents[i] starts after aEvent.mTime and ends before aEvent.mTime? I'm not sure that makes sense. >+ // Make sure that curve events don't fall in a range which includes other >+ // events. >+ if (aEvent.mType == Event::Type::SetValueCurve) { >+ for (unsigned i = 0; i < mEvents.Length(); ++i) { >+ if (mEvents[i].mTime >= aEvent.mTime || >+ mEvents[i].mTime <= (aEvent.mTime + aEvent.mDuration)) { This will throw on any event that comes completely before aEvent.mTime. Again, something is fishy here. >+ if (aEvent.mTime == mEvents[i].mTime) { >+ if (aEvent.mType == mEvents[i].mType) { >+ // If times and types are equal, replace the event Again, strict equality testing of floats worries me. Not sure I have a better proposal, though. :( >+ // Otherwise, place the element after the last event of another type So you can't have multiple SetValue events at a single time, but having a bunch of alternating SetValue and LinearRamp, all for the same time, is ok? >+++ b/content/media/webaudio/AudioParam.h >+ FloatArrayWrapper(const FloatArrayWrapper& rhs) >+ { >+ mArray.construct(rhs.mArray.ref()); Don't you need to check that !rhs.mArray.empty() before doing that? >+ FloatArrayWrapper& operator=(const FloatArrayWrapper& rhs) >+ { >+ if (!mArray.empty()) { >+ mArray.destroy(); mArray.destroyIfConstructed(); >+ mArray.construct(rhs.mArray.ref()); Again, do you need to deal with a non-constructed rhs? >+ AudioParamTimeline::SetValueCurveAtTime(detail::FloatArrayWrapper(cx, aValues.Obj()), >+ aStartTime, aDuration, aRv); Fix indent, please. >+++ b/dom/bindings/Bindings.conf >+ 'implicitJSContext': [ 'setValueCurveAtTime' ], Why not give FloatArrayWrapper a constructor taking |const Float32Array&|? Then I don't think you'd need this (and may not need the constructor taking JSContext* and JSObject* at all). No r+ yet, because I'd like to understand what's going on with those "Make sure" bits that are doing time comparisons...

(In reply to Boris Zbarsky (:bz) from comment #11) > Comment on attachment 675653 [details] [diff] [review] > Patch (v2) > > >+++ b/content/media/webaudio/AudioEventTimeline.h > >+ static bool IsValid(float value) > >+ { > >+ return !MOZ_DOUBLE_IS_NaN(value) && > >+ !MOZ_DOUBLE_IS_INFINITE(value); > >+ } > > How about just: > > return MOZ_DOUBLE_IS_FINITE(value); Done. > Note that I haven't carefully checked correctness of GetValueAtTime against > the spec. Please let me know if I should. I don't think that you need to. I tried to test all of the interesting cases, and we can just fix the bugs later as this code actually gets used. Mapping the spec to the implementation is entirely non-trivial, so it doesn't make a lot of sense to ask you to do that in the review, really. > That said, the whole strict > equality testing on floats thing worries me... So I gave this some thought. In the future we might decide not to do this, but the entire business of how the sampling stuff works is not entirely clear to me yet... I can definitely adjust the logic here once it's evident that it will be necessary. > >+ static float LinearInterpolate(float t0, float v0, float t1, float v1, float t) > >+ { > >+ return v0 + (v1 - v0) * ((t - t0) / (t1 - t0)); > >+ } > > I guess you already ensured that t1-t0 is nonzero (or more precisely that t1 > != t0) in GetValueAtTime? Because divide-by-0 is not good. :( Maybe we'll > get NaN, maybe we'll get a signal. Good point. I am in fact handling those cases, but I added a bunch of explicit tests for those. > >+ static float ExponentialInterpolate(float t0, float v0, float t1, float v1, float t) > >+ { > >+ return v0 * powf(v1 / v0, (t - t0) / (t1 - t0)); > >+ } > > So the goal here is a function that's v0 at t0, v1 at t1 and has the form > A*exp(B(t-t0)) ? I guess so. Here's the function defined in the spec: v(t) = V0 * (V1 / V0) ^ ((t - T0) / (T1 - T0)) > >+ // Make sure that non-curve events don't fall within the duration of a > >+ // curve event. > >+ for (unsigned i = 0; i < mEvents.Length(); ++i) { > >+ if (mEvents[i].mType == Event::Type::SetValueCurve && > >+ mEvents[i].mTime >= aEvent.mTime && > >+ (mEvents[i].mTime + mEvents[i].mDuration) <= aEvent.mTime) { > > So what this is checking is that mEvents[i] starts after aEvent.mTime and > ends before aEvent.mTime? I'm not sure that makes sense. Right, I got the condition wrong here. Will fix in the next iteration. Note that this code is not being used yet which is why I did not catch this in the tests. > >+ // Make sure that curve events don't fall in a range which includes other > >+ // events. > >+ if (aEvent.mType == Event::Type::SetValueCurve) { > >+ for (unsigned i = 0; i < mEvents.Length(); ++i) { > >+ if (mEvents[i].mTime >= aEvent.mTime || > >+ mEvents[i].mTime <= (aEvent.mTime + aEvent.mDuration)) { > > This will throw on any event that comes completely before aEvent.mTime. > Again, something is fishy here. Yeah sorry, fixed this too. > >+ if (aEvent.mTime == mEvents[i].mTime) { > >+ if (aEvent.mType == mEvents[i].mType) { > >+ // If times and types are equal, replace the event > > Again, strict equality testing of floats worries me. Not sure I have a > better proposal, though. :( So this is what the spec says, and also what WebKit does. Like I said, this worries me a bit too, but at this time I'm convinced that it won't be a problem in practice. (But I may end up changing this later.) > >+ // Otherwise, place the element after the last event of another type > > So you can't have multiple SetValue events at a single time, but having a > bunch of alternating SetValue and LinearRamp, all for the same time, is ok? Seems like it! :-) > >+++ b/content/media/webaudio/AudioParam.h > >+ FloatArrayWrapper(const FloatArrayWrapper& rhs) > >+ { > >+ mArray.construct(rhs.mArray.ref()); > > Don't you need to check that !rhs.mArray.empty() before doing that? Yeah, should MOZ_ASSERT. > >+++ b/dom/bindings/Bindings.conf > >+ 'implicitJSContext': [ 'setValueCurveAtTime' ], > > Why not give FloatArrayWrapper a constructor taking |const Float32Array&|? > Then I don't think you'd need this (and may not need the constructor taking > JSContext* and JSObject* at all). Hmm, right, yeah, I can do that. I don't really remember why I needed the JSContext in the first place.

Comment on attachment 676304 [details] [diff] [review] Patch (v3) Ok. The curve event bits look much saner now. One more question there: we only need to check that mTime doesn't fall on our curve, not that nothing in [mTime, mTime+mDuration] falls on our curve? That seems a bit odd to me without having read the spec. r=me modulo that question

mDuration is only used for the curve events. I should probably add such checks when I implement adding curve events for real... It won't matter for now.

Is this already usable by end-users and (already) need documentation?

(In reply to comment #18) > Is this already usable by end-users and (already) need documentation? This is hidden behind a pref right now (media.webaudio.enabled). We definitely need documentation for this, but this is only one part of the Web Audio API which needs documentation. :-)

Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.

This has been documented: https://developer.mozilla.org/en-US/docs/Web/API/AudioParam