Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2527813002:
Remove unnecessary lock on setClient(). (Closed)
|
Created:
4 years ago by DaleCurtis Modified:
4 years ago Reviewers:
o1ka CC:
chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unnecessary lock on setClient().
The lock is unnecessary because the operation is a no-op
when client is unchanged (the vast majority of cases).
BUG=615589, 647498
TEST=none
Committed: https://crrev.com/8be2e3fd36fb909ba695c0bd84ef9f97d3855ac3
Cr-Commit-Position: refs/heads/master@{#434380}
Patch Set 1 #Patch Set 2 : Fix comment. #
Total comments: 2
Messages
Total messages: 22 (14 generated)
Description was changed from ========== Remove unnecessary lock on GetOutputDeviceInfo(). The lock is unnecessary because concurrent access to GetOutptuDeviceInfo() works just fine. This reduces the chance of render hangs on long authorizations. BUG=615589 TEST=none ========== to ========== Remove unnecessary lock on GetOutputDeviceInfo(). The lock is unnecessary because concurrent access to GetOutptuDeviceInfo() works just fine. This reduces the chance of render hangs on long authorizations. BUG=615589,647498 TEST=none ==========
dalecurtis@chromium.org changed reviewers: + olka@chromium.org
Probably after this lands we can up the authorization timeouts too since the block will happen only on the media thread instead of the render thread. If we do so, we'll need to revert the NullAudioSink change since it calls into this method on the main thread.
The CQ bit was checked by dalecurtis@chromium.org 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...
Actually, after looking at AudioRendererMixerInput more closely we can't remove the lock here since setClient() may destroy the |mixer_| used during ARMI::GetOutputDeviceInfo(). If instead we always call ARMM::GetOutputDeviceInfo(owner_id_, 0, device_id_, security_origin_) we suffer a similar problem from SwitchOutputDevice(). I think the best we can do is to reduce the frequency of this lock attempt, which is much easier to do. The latest patch set avoids taking the lock if its a no-op.
Description was changed from ========== Remove unnecessary lock on GetOutputDeviceInfo(). The lock is unnecessary because concurrent access to GetOutptuDeviceInfo() works just fine. This reduces the chance of render hangs on long authorizations. BUG=615589,647498 TEST=none ========== to ========== Remove unnecessary lock on setClient(). The lock is unnecessary because the operation is a no-op when client is unchanged (the vast majority of cases). BUG=615589,647498 TEST=none ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by dalecurtis@chromium.org 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: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
lgtm if we state that setClient() is not thread-safe. https://codereview.chromium.org/2527813002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2527813002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:110: if (client_ == client) It looks like a data race if setClient() is called concurrently from two different threads: |client_| may be accessed here and modified at l.119 at the same time. As far as I see it is never called concurrently thought: HTMLMediaElement always calls setClient() under |provideInputLock|, so it should hopefully be fine if we have a comment for setClient() that it cannot be called concurrently.
https://codereview.chromium.org/2527813002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2527813002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:110: if (client_ == client) On 2016/11/24 at 13:21:16, o1ka wrote: > It looks like a data race if setClient() is called concurrently from two different threads: > |client_| may be accessed here and modified at l.119 at the same time. > > As far as I see it is never called concurrently thought: HTMLMediaElement always calls setClient() under |provideInputLock|, so it should hopefully be fine if we have a comment for setClient() that it cannot be called concurrently. The lock in HTMLME is for the same reason the lock exists internally here; to prevent the client from being null'd during provideInput / Render calls, but otherwise all methods on this interface are always called on the render thread. I think instead of adding a comment to this interface, which no caller would really ever see, we should add one to https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebAu... WDYT? I can do that in a followup patch set if so. I don't have access to make changes today (Turkey time!)
On 2016/11/24 18:06:53, DaleCurtis wrote: > https://codereview.chromium.org/2527813002/diff/60001/media/blink/webaudiosou... > File media/blink/webaudiosourceprovider_impl.cc (right): > > https://codereview.chromium.org/2527813002/diff/60001/media/blink/webaudiosou... > media/blink/webaudiosourceprovider_impl.cc:110: if (client_ == client) > On 2016/11/24 at 13:21:16, o1ka wrote: > > It looks like a data race if setClient() is called concurrently from two > different threads: > > |client_| may be accessed here and modified at l.119 at the same time. > > > > As far as I see it is never called concurrently thought: HTMLMediaElement > always calls setClient() under |provideInputLock|, so it should hopefully be > fine if we have a comment for setClient() that it cannot be called concurrently. > > The lock in HTMLME is for the same reason the lock exists internally here; to > prevent the client from being null'd during provideInput / Render calls, but > otherwise all methods on this interface are always called on the render thread. > > I think instead of adding a comment to this interface, which no caller would > really ever see, we should add one to > https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebAu... > > WDYT? I can do that in a followup patch set if so. I don't have access to make > changes today (Turkey time!) Sure, I meant the interface class. LGTM - Have a nice weekend!
The CQ bit was checked by olka@chromium.org
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": 60001, "attempt_start_ts": 1480012571220050,
"parent_rev": "c46bd56856484e3f5f40af2307539ad33f1b6408", "commit_rev":
"73b3478f2ee436c605ba26905cd5f810a236c1c5"}
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary lock on setClient(). The lock is unnecessary because the operation is a no-op when client is unchanged (the vast majority of cases). BUG=615589,647498 TEST=none ========== to ========== Remove unnecessary lock on setClient(). The lock is unnecessary because the operation is a no-op when client is unchanged (the vast majority of cases). BUG=615589,647498 TEST=none Committed: https://crrev.com/8be2e3fd36fb909ba695c0bd84ef9f97d3855ac3 Cr-Commit-Position: refs/heads/master@{#434380} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8be2e3fd36fb909ba695c0bd84ef9f97d3855ac3 Cr-Commit-Position: refs/heads/master@{#434380} |
