Validate the arguments to AudioParam.exponentialRampToValueAtTime() correctly

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla22
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

7 years ago
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.
Assignee

Comment 1

7 years ago
Posted patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #707853 - Flags: review?(roc)
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.
Assignee

Comment 3

7 years ago
(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.
I do not understand.

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

Comment 5

7 years ago
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.
I don't think the text in comment #0 makes any sense then.
Assignee

Comment 7

7 years ago
OK, posted a question to the list.
Now that I read your email to the list and re-read the spec, I realize that I'm an idiot.
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?
Assignee

Comment 10

7 years ago
(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 on attachment 707853 [details] [diff] [review]
Patch (v1)

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

OK!
Attachment #707853 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/cfefa8d06ac0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee

Comment 16

6 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in before you can comment on or make changes to this bug.