Bug 61830 – Add AudioParam parameter scheduling implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add AudioParam parameter scheduling implementation

[Chris Rogers]

Created attachment 95525 [details]
Patch

[Kenneth Russell]

Comment on attachment 95525 [details]
Patch

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.

[Chris Rogers]

Committed r88037: <http://trac.webkit.org/changeset/88037>

[Chris Rogers]

Comment on attachment 95525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95525&action=review

>> 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.

Each parameter has its own lock, so having many parameters won't increase the risk.  Also, the result of failing the tryLock will not be a "dropout" but simply a smooth continuation of using the previous parameter value.  Any param change will happen the next render quantum delayed by only a few milliseconds.  But even this will be a somewhat rare occurrence.

>> Source/WebCore/webaudio/AudioParamTimeline.cpp:174
>> +    ASSERT(values);
> 
> Can you add some assert about m_eventsLock being held here?

Not sure how to assert a Mutex being held.  I'd rather not call tryLock() just to make sure it fails...

>> 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.

FIXED

[Nico Weber]

Hi this breaks the clang build thusly:

clang:warning: argument unused during compilation: '-fobjc-exceptions'
/b/build/slave/Mac_Clang__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../webaudio/AudioParamTimeline.cpp:231:39:error: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Werror,-Wsign-compare]
        int nextEventType = nextEvent ? nextEvent->type() : -1 /* unknown */;

Can you fix?

[Chris Rogers]

Ok, I'll try to fix this.

[Chris Rogers]

should be fixed now in:
http://trac.webkit.org/changeset/88089