Bug 76772 - Clean up m_processLock logic in AudioBasicProcessorNode and AudioGainNode
Summary: Clean up m_processLock logic in AudioBasicProcessorNode and AudioGainNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-20 23:07 PST by Raymond
Modified: 2012-01-31 19:05 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.73 KB, patch)
2012-01-20 23:13 PST, Raymond
no flags Details | Formatted Diff | Diff
Patch (7.80 KB, patch)
2012-01-29 18:22 PST, Raymond
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond 2012-01-20 23:07:25 PST
Clean up m_processLock logic in AudioBasicProcessorNode and AudioGainNode
Comment 1 Raymond 2012-01-20 23:13:52 PST
Created attachment 123435 [details]
Patch
Comment 2 Raymond 2012-01-20 23:24:22 PST
Comment on attachment 123435 [details]
Patch

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

> Source/WebCore/webaudio/AudioBasicProcessorNode.cpp:-128
> -        // We need to be careful since we may be actively processing right now, so synchronize with process().
> -        MutexLocker locker(m_processLock);

In AudioGainNode, all m_processLock related code is removed since no one use it anymore. While for AudioBasicProcessorNode, only remove the lock here, while reserve the lock mechanism in process(), since subclass might do some thing in main thread. ( Though, actually it seems to me they do not at present.)
Comment 3 Chris Rogers 2012-01-25 14:58:35 PST
(In reply to comment #2)
> (From update of attachment 123435 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123435&action=review
> 
> > Source/WebCore/webaudio/AudioBasicProcessorNode.cpp:-128
> > -        // We need to be careful since we may be actively processing right now, so synchronize with process().
> > -        MutexLocker locker(m_processLock);
> 
> In AudioGainNode, all m_processLock related code is removed since no one use it anymore. While for AudioBasicProcessorNode, only remove the lock here, while reserve the lock mechanism in process(), since subclass might do some thing in main thread. ( Though, actually it seems to me they do not at present.)

Raymond, thanks for the patch.  I think we should remove the lock in AudioBasicProcessorNode::process() as well.  m_processLock is private, so no sub-class can do anything with it.  Also, all changes should now happen with the AudioContext lock in the pre or post-render tasks anyway so we shouldn't need it.  Does that make sense?
Comment 4 Raymond 2012-01-29 18:22:49 PST
Created attachment 124481 [details]
Patch
Comment 5 Chris Rogers 2012-01-31 16:45:06 PST
Thanks Raymond, looks good.
Comment 6 Kenneth Russell 2012-01-31 17:09:29 PST
Comment on attachment 124481 [details]
Patch

rs=me
Comment 7 WebKit Review Bot 2012-01-31 19:05:04 PST
Comment on attachment 124481 [details]
Patch

Clearing flags on attachment: 124481

Committed r106420: <http://trac.webkit.org/changeset/106420>
Comment 8 WebKit Review Bot 2012-01-31 19:05:08 PST
All reviewed patches have been landed.  Closing bug.