Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929283002:
Avoid AudioBufferSourceHandler data race. (Closed)
DescriptionAvoid AudioBufferSourceHandler data race.
Following r478084, the main thread may contend with the audio thread
on accessing AudioBufferSourceNodeHandler's mutable state. Coordinate
such access by introducing a Mutex over |min_playback_rate_|.
Using atomic ops would be the natural choice for handling this, but
steering clear of those over doubles (cf. https://crrev.com/1256053006)
until std::atomic<> is allowed.
R=haraken,hongchan
BUG=731568
Review-Url: https://codereview.chromium.org/2929283002
Cr-Commit-Position: refs/heads/master@{#478679}
Committed: https://chromium.googlesource.com/chromium/src/+/7ad07b8cd01390ea3b8ec8e43c9827fb5f87891a
Patch Set 1 #Patch Set 2 : minimize lock taking #
Total comments: 5
Patch Set 3 : rename accessor #
Messages
Total messages: 24 (18 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sigbjornf@opera.com changed reviewers: + haraken@chromium.org, hongchan@chromium.org, rtoy@chromium.org
please take a look. (If needed, see review mentioned in the commit message for context on why atomic ops aren't used.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
Again, thanks for working on this! lgtm with nits https://codereview.chromium.org/2929283002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp (right): https://codereview.chromium.org/2929283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:602: double AudioBufferSourceHandler::MinPlaybackRate() { I think we prefer Get...() as a getter name. https://codereview.chromium.org/2929283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:603: MutexLocker locker(min_playback_rate_mutex_); Can we have DCHECK(isMainThread()) here? https://codereview.chromium.org/2929283002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.h (right): https://codereview.chromium.org/2929283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.h:171: double MinPlaybackRate(); GetMinPlaybackRate() might be better.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review; I don't see any other parts of the object state that's cross-thread mutated and needs a similar treatment. https://codereview.chromium.org/2929283002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp (right): https://codereview.chromium.org/2929283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:602: double AudioBufferSourceHandler::MinPlaybackRate() { On 2017/06/12 14:09:17, hongchan wrote: > I think we prefer Get...() as a getter name. Done (a naming outlier for this class.) https://codereview.chromium.org/2929283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:603: MutexLocker locker(min_playback_rate_mutex_); On 2017/06/12 14:09:17, hongchan wrote: > Can we have DCHECK(isMainThread()) here? Done.
Description was changed from ========== Avoid AudioBufferSourceHandler data race. Following r478084, the main thread may contend with the audio thread on accessing AudioBufferSourceNodeHandler's mutable state. Coordinate such access by introducing a Mutex over |min_playback_rate_|. Using atomic ops would be the natural choice for handling this, but steering clear of those over doubles (cf. https://crrev.com/1256053006) until std::atomic<> is allowed. R= BUG=731568 ========== to ========== Avoid AudioBufferSourceHandler data race. Following r478084, the main thread may contend with the audio thread on accessing AudioBufferSourceNodeHandler's mutable state. Coordinate such access by introducing a Mutex over |min_playback_rate_|. Using atomic ops would be the natural choice for handling this, but steering clear of those over doubles (cf. https://crrev.com/1256053006) until std::atomic<> is allowed. R=haraken,hongchan BUG=731568 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, hongchan@chromium.org Link to the patchset: https://codereview.chromium.org/2929283002/#ps40001 (title: "rename accessor")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497290210731660,
"parent_rev": "775f2fb6ca172043fa05479a909e5f78e1c881a1", "commit_rev":
"7ad07b8cd01390ea3b8ec8e43c9827fb5f87891a"}
Message was sent while issue was closed.
Description was changed from ========== Avoid AudioBufferSourceHandler data race. Following r478084, the main thread may contend with the audio thread on accessing AudioBufferSourceNodeHandler's mutable state. Coordinate such access by introducing a Mutex over |min_playback_rate_|. Using atomic ops would be the natural choice for handling this, but steering clear of those over doubles (cf. https://crrev.com/1256053006) until std::atomic<> is allowed. R=haraken,hongchan BUG=731568 ========== to ========== Avoid AudioBufferSourceHandler data race. Following r478084, the main thread may contend with the audio thread on accessing AudioBufferSourceNodeHandler's mutable state. Coordinate such access by introducing a Mutex over |min_playback_rate_|. Using atomic ops would be the natural choice for handling this, but steering clear of those over doubles (cf. https://crrev.com/1256053006) until std::atomic<> is allowed. R=haraken,hongchan BUG=731568 Review-Url: https://codereview.chromium.org/2929283002 Cr-Commit-Position: refs/heads/master@{#478679} Committed: https://chromium.googlesource.com/chromium/src/+/7ad07b8cd01390ea3b8ec8e43c98... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7ad07b8cd01390ea3b8ec8e43c98... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
