Add AudioParam parameter scheduling implementation
Created attachment 95525 [details] Patch
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.
Committed r88037: <http://trac.webkit.org/changeset/88037>
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
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?
Ok, I'll try to fix this.
should be fixed now in: http://trac.webkit.org/changeset/88089