836072 - Validate the arguments to AudioParam.exponentialRampToValueAtTime() correctly

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[(no longer active)]

From the spec:

"The value parameter is the value the parameter will exponentially ramp to at the given time. An exception will be thrown if this value is less than or equal to 0, or if the value at the time of the previous event is less than or equal to 0."

I have a patch to enforce this.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The spec seems a little ambiguous to me. Wouldn't it make sense to sample the timeline at time T and use that as the starting point for the exponential ramp, and check here that that starting point is positive? That's not what your code does.

Comment 3

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> The spec seems a little ambiguous to me. Wouldn't it make sense to sample
> the timeline at time T and use that as the starting point for the
> exponential ramp, and check here that that starting point is positive?
> That's not what your code does.

No, I don't think that's what you want here.  This condition needs to be enforced in order to avoid computing NaNs, which is in fact how I found out that I had previously missed that sentence in the spec.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I do not understand.

What exactly does "the time of the previous event" mean in the spec?

Comment 5

[(no longer active)]

I have always taken it to mean the time parameter specified for the previous event.  I can file a spec issue in order to clarify this if you want me to.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think the text in comment #0 makes any sense then.

Comment 7

[(no longer active)]

OK, posted a question to the list.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Now that I read your email to the list and re-read the spec, I realize that I'm an idiot.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 707853 [details] [diff] [review]
Patch (v1)

Review of attachment 707853 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AudioEventTimeline.h
@@ +297,5 @@
> +    const Event* previous = nullptr;
> +    const Event* next = nullptr;
> +
> +    bool bailOut = false;
> +    for (unsigned i = 0; !bailOut && i < mEvents.Length(); ++i) {

Why aren't you iterating from the last event to the first event? Wouldn't that be simpler?

Comment 10

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 707853 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 707853 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/AudioEventTimeline.h
> @@ +297,5 @@
> > +    const Event* previous = nullptr;
> > +    const Event* next = nullptr;
> > +
> > +    bool bailOut = false;
> > +    for (unsigned i = 0; !bailOut && i < mEvents.Length(); ++i) {
> 
> Why aren't you iterating from the last event to the first event? Wouldn't
> that be simpler?

Not sure how that would be simpler.  We still need to maintain both the previous and next pointers, and then the logic would be a bit harder to follow, I think.  Note that this code is borrowed from GetValueAtTime (I didn't bother refactoring that piece into a helper since GetValueAtTime will go away soon), and reading from backwards there was my first intuition as well, but I then decided that iterating forwards was a bit cleaner.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 707853 [details] [diff] [review]
Patch (v1)

Review of attachment 707853 [details] [diff] [review]:
-----------------------------------------------------------------

OK!

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5dcf166f09

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/09796dfecc53 for a signed-unsigned comparison.

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfefa8d06ac0

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/cfefa8d06ac0

Comment 16

[(no longer active)]

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