[webkit-reviews] review granted: [Bug 61830] Add AudioParam parameter scheduling implementation : [Attachment 95525] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Chris Rogers
<crogers at google.com>'s request for review:
Bug 61830: Add AudioParam parameter scheduling implementation
https://bugs.webkit.org/show_bug.cgi?id=61830

Attachment 95525: Patch
https://bugs.webkit.org/attachment.cgi?id=95525&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=95525&action=review

This generally looks good though I do have a few questions, one high-level.
Still, I'll mark this r+ to avoid blocking you.

> Source/WebCore/webaudio/AudioParamTimeline.cpp:145
> +    if (!m_eventsLock.tryLock()) {

I know that similar trylocks are used elsewhere on the audio thread, but won't
this lock have a pretty high chance of contention if the application is
requesting animation of a lot of parameters? It seems to me that there will be
a high risk of dropouts specifically due to this new mechanism.

> Source/WebCore/webaudio/AudioParamTimeline.cpp:174
> +    ASSERT(values);

Can you add some assert about m_eventsLock being held here?

> Source/WebCore/webaudio/AudioParamTimeline.cpp:221
> +	   float time2 = nextEvent ? nextEvent->time() : endTime + 1.0;

".0" suffix is probably unneeded here and in several places below and should be
removed per style guide.