Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813113002:
Adding ArcBridge Interface for Volume Update Requests (Closed)
|
Created:
3 years, 8 months ago by yueli Modified:
3 years, 6 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding ArcBridge Interface for Volume Update Requests
In order to enable volume adjustment from Android side, this CL provide
ArcBridge interface to ArcAudioService on Android side.
Android could send volume update request by calling
OnVolumeUpdateRequest(float volume) with volume in range [0,100].
BUG=b/36129286
Review-Url: https://codereview.chromium.org/2813113002
Cr-Commit-Position: refs/heads/master@{#477399}
Committed: https://chromium.googlesource.com/chromium/src/+/3b05369516bd12ef0c06f1646240bf60abe6c28f
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding ArcBridge Interface for Volume Update Requests #
Total comments: 8
Patch Set 3 : Adding ArcBridge Interface for Volume Update Requests #
Total comments: 2
Patch Set 4 : Adding ArcBridge Interface for Volume Update Requests #
Total comments: 3
Patch Set 5 : Adding ArcBridge Interface for Volume Update Requests #
Messages
Total messages: 24 (10 generated)
Description was changed from ========== Adding ArcBridge Interface for Volume Update Requests In order to enable volume adjustment from Android side, this CL provide ArcBridge interface to ArcAudioService on Android side. Android could send volume update request by calling OnVolumeUpdateRequest(float volume) with volume in range [0,100]. BUG=b/36129286 ========== to ========== Adding ArcBridge Interface for Volume Update Requests In order to enable volume adjustment from Android side, this CL provide ArcBridge interface to ArcAudioService on Android side. Android could send volume update request by calling OnVolumeUpdateRequest(float volume) with volume in range [0,100]. BUG=b/36129286 ==========
updowndota@chromium.org changed reviewers: + dcheng@chromium.org, lhchavez@chromium.org, muyuanli@chromium.org, xiaohuic@chromium.org
Waiting for the decision whether or not to land this. https://codereview.chromium.org/2813113002/diff/1/components/arc/audio/arc_au... File components/arc/audio/arc_audio_bridge.cc (right): https://codereview.chromium.org/2813113002/diff/1/components/arc/audio/arc_au... components/arc/audio/arc_audio_bridge.cc:45: volume <= cras_audio_handler_->GetOutputDefaultVolumeMuteThreshold()) { how about: bool is_muted = volume <= cras_audio_handler_->GetOutputDefaultVolumeMuteThreshold(); if (cras_audio_handler_->IsOutputMuted() != is_muted) cras_audio_handler_->SetOutputMute(is_muted);
https://codereview.chromium.org/2813113002/diff/1/components/arc/audio/arc_au... File components/arc/audio/arc_audio_bridge.cc (right): https://codereview.chromium.org/2813113002/diff/1/components/arc/audio/arc_au... components/arc/audio/arc_audio_bridge.cc:45: volume <= cras_audio_handler_->GetOutputDefaultVolumeMuteThreshold()) { On 2017/04/12 15:10:11, Luis Héctor Chávez wrote: > how about: > > bool is_muted = volume <= > cras_audio_handler_->GetOutputDefaultVolumeMuteThreshold(); > > if (cras_audio_handler_->IsOutputMuted() != is_muted) > cras_audio_handler_->SetOutputMute(is_muted); Done.
https://codereview.chromium.org/2813113002/diff/20001/components/arc/common/a... File components/arc/common/audio.mojom (right): https://codereview.chromium.org/2813113002/diff/20001/components/arc/common/a... components/arc/common/audio.mojom:23: [MinVersion=3] OnVolumeUpdateRequest@1(float volume); nit: OnSystemVolumeUpdateRequest Also mention that this is a privileged API and should only be used on whitelisted cases.
https://codereview.chromium.org/2813113002/diff/20001/components/arc/common/a... File components/arc/common/audio.mojom (right): https://codereview.chromium.org/2813113002/diff/20001/components/arc/common/a... components/arc/common/audio.mojom:23: [MinVersion=3] OnVolumeUpdateRequest@1(float volume); On 2017/04/13 14:21:25, Luis Héctor Chávez wrote: > nit: OnSystemVolumeUpdateRequest > > Also mention that this is a privileged API and should only be used on > whitelisted cases. Can you clarify what that means? We have no way of enforcing that from the browser process, right? So really anyone can call it.
https://codereview.chromium.org/2813113002/diff/20001/components/arc/common/a... File components/arc/common/audio.mojom (right): https://codereview.chromium.org/2813113002/diff/20001/components/arc/common/a... components/arc/common/audio.mojom:23: [MinVersion=3] OnVolumeUpdateRequest@1(float volume); On 2017/04/13 18:02:40, dcheng wrote: > On 2017/04/13 14:21:25, Luis Héctor Chávez wrote: > > nit: OnSystemVolumeUpdateRequest > > > > Also mention that this is a privileged API and should only be used on > > whitelisted cases. > > Can you clarify what that means? We have no way of enforcing that from the > browser process, right? So really anyone can call it. Yes, we're trusting that the other side will only call this in appropriate occasions. Otherwise random apps will be able to modify the system volume and we'd like to avoid that. The best we can do here is document the intended usage.
https://codereview.chromium.org/2813113002/diff/20001/components/arc/audio/ar... File components/arc/audio/arc_audio_bridge.cc (right): https://codereview.chromium.org/2813113002/diff/20001/components/arc/audio/ar... components/arc/audio/arc_audio_bridge.cc:43: cras_audio_handler_->SetOutputVolumePercent(volume); This should check that the input is in range. (Out of curiosity, why a float and not an int?)
Finally got legal approval and got android side CLs prepared. PTAL https://codereview.chromium.org/2813113002/diff/20001/components/arc/audio/ar... File components/arc/audio/arc_audio_bridge.cc (right): https://codereview.chromium.org/2813113002/diff/20001/components/arc/audio/ar... components/arc/audio/arc_audio_bridge.cc:43: cras_audio_handler_->SetOutputVolumePercent(volume); On 2017/04/14 03:36:42, dcheng wrote: > This should check that the input is in range. > > (Out of curiosity, why a float and not an int?) It seems the range is already handled in SetOutputNodeVolumePercent. As for the float value, just want to provide the more accurate value. But since we can only set volume by int, it also make sense to just send an int. https://codereview.chromium.org/2813113002/diff/20001/components/arc/common/a... File components/arc/common/audio.mojom (right): https://codereview.chromium.org/2813113002/diff/20001/components/arc/common/a... components/arc/common/audio.mojom:23: [MinVersion=3] OnVolumeUpdateRequest@1(float volume); On 2017/04/13 18:05:40, Luis Héctor Chávez wrote: > On 2017/04/13 18:02:40, dcheng wrote: > > On 2017/04/13 14:21:25, Luis Héctor Chávez wrote: > > > nit: OnSystemVolumeUpdateRequest > > > > > > Also mention that this is a privileged API and should only be used on > > > whitelisted cases. > > > > Can you clarify what that means? We have no way of enforcing that from the > > browser process, right? So really anyone can call it. > > Yes, we're trusting that the other side will only call this in appropriate > occasions. Otherwise random apps will be able to modify the system volume and > we'd like to avoid that. The best we can do here is document the intended usage. Done.
https://codereview.chromium.org/2813113002/diff/20001/components/arc/audio/ar... File components/arc/audio/arc_audio_bridge.cc (right): https://codereview.chromium.org/2813113002/diff/20001/components/arc/audio/ar... components/arc/audio/arc_audio_bridge.cc:43: cras_audio_handler_->SetOutputVolumePercent(volume); On 2017/06/02 19:10:13, yueli wrote: > On 2017/04/14 03:36:42, dcheng wrote: > > This should check that the input is in range. > > > > (Out of curiosity, why a float and not an int?) > > It seems the range is already handled in SetOutputNodeVolumePercent. This is OK, but I will note that it makes it a bit confusing when validation is handled at different layers. I wouldn't have expected SetOuputNodeVolumePercent to have to try to handle bad inputs gracefully, but I don't understand the CrOS layering well enough to comment more. > As for the float value, just want to provide the more accurate value. But since > we can only set volume by int, it also make sense to just send an int. I don't see any changes in this CL, does this comment mean that we're planning on changing this to pass an int? https://codereview.chromium.org/2813113002/diff/40001/components/arc/common/a... File components/arc/common/audio.mojom (right): https://codereview.chromium.org/2813113002/diff/40001/components/arc/common/a... components/arc/common/audio.mojom:24: [MinVersion=3] OnSystemVolumeUpdateRequest@1(float volume); Nit: I would suggest calling this parameter |percent|, which makes it a bit more self documenting.
https://codereview.chromium.org/2813113002/diff/20001/components/arc/audio/ar... File components/arc/audio/arc_audio_bridge.cc (right): https://codereview.chromium.org/2813113002/diff/20001/components/arc/audio/ar... components/arc/audio/arc_audio_bridge.cc:43: cras_audio_handler_->SetOutputVolumePercent(volume); On 2017/06/02 23:25:35, dcheng wrote: > On 2017/06/02 19:10:13, yueli wrote: > > On 2017/04/14 03:36:42, dcheng wrote: > > > This should check that the input is in range. > > > > > > (Out of curiosity, why a float and not an int?) > > > > It seems the range is already handled in SetOutputNodeVolumePercent. > > This is OK, but I will note that it makes it a bit confusing when validation is > handled at different layers. I wouldn't have expected SetOuputNodeVolumePercent > to have to try to handle bad inputs gracefully, but I don't understand the CrOS > layering well enough to comment more. > > > As for the float value, just want to provide the more accurate value. But > since > > we can only set volume by int, it also make sense to just send an int. > > I don't see any changes in this CL, does this comment mean that we're planning > on changing this to pass an int? Done. https://codereview.chromium.org/2813113002/diff/40001/components/arc/common/a... File components/arc/common/audio.mojom (right): https://codereview.chromium.org/2813113002/diff/40001/components/arc/common/a... components/arc/common/audio.mojom:24: [MinVersion=3] OnSystemVolumeUpdateRequest@1(float volume); On 2017/06/02 23:25:35, dcheng wrote: > Nit: I would suggest calling this parameter |percent|, which makes it a bit more > self documenting. Done.
lgtm with a nit https://codereview.chromium.org/2813113002/diff/60001/components/arc/audio/ar... File components/arc/audio/arc_audio_bridge.h (right): https://codereview.chromium.org/2813113002/diff/60001/components/arc/audio/ar... components/arc/audio/arc_audio_bridge.h:34: void OnSystemVolumeUpdateRequest(int32_t volume) override; nit: s/volume/percent/
mojom lgtm https://codereview.chromium.org/2813113002/diff/60001/components/arc/audio/ar... File components/arc/audio/arc_audio_bridge.cc (right): https://codereview.chromium.org/2813113002/diff/60001/components/arc/audio/ar... components/arc/audio/arc_audio_bridge.cc:47: percent <= cras_audio_handler_->GetOutputDefaultVolumeMuteThreshold(); FWIW, it seems like this is already down in CrasAudioHandler: https://cs.chromium.org/chromium/src/chromeos/audio/cras_audio_handler.cc?rcl...
The CQ bit was checked by updowndota@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2813113002/diff/60001/components/arc/audio/ar... File components/arc/audio/arc_audio_bridge.cc (right): https://codereview.chromium.org/2813113002/diff/60001/components/arc/audio/ar... components/arc/audio/arc_audio_bridge.cc:47: percent <= cras_audio_handler_->GetOutputDefaultVolumeMuteThreshold(); On 2017/06/06 18:45:38, dcheng wrote: > FWIW, it seems like this is already down in CrasAudioHandler: > https://cs.chromium.org/chromium/src/chromeos/audio/cras_audio_handler.cc?rcl... IIUC SetOutputMute will change the device mute settings, which is not equivalent to set volume to 0.
The CQ bit was checked by updowndota@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2813113002/#ps80001 (title: "Adding ArcBridge Interface for Volume Update Requests")
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": 80001, "attempt_start_ts": 1496779673545910,
"parent_rev": "ecd2115cc793eddb9e1abc518a1b6388130a61a2", "commit_rev":
"3b05369516bd12ef0c06f1646240bf60abe6c28f"}
Message was sent while issue was closed.
Description was changed from ========== Adding ArcBridge Interface for Volume Update Requests In order to enable volume adjustment from Android side, this CL provide ArcBridge interface to ArcAudioService on Android side. Android could send volume update request by calling OnVolumeUpdateRequest(float volume) with volume in range [0,100]. BUG=b/36129286 ========== to ========== Adding ArcBridge Interface for Volume Update Requests In order to enable volume adjustment from Android side, this CL provide ArcBridge interface to ArcAudioService on Android side. Android could send volume update request by calling OnVolumeUpdateRequest(float volume) with volume in range [0,100]. BUG=b/36129286 Review-Url: https://codereview.chromium.org/2813113002 Cr-Commit-Position: refs/heads/master@{#477399} Committed: https://chromium.googlesource.com/chromium/src/+/3b05369516bd12ef0c06f1646240... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3b05369516bd12ef0c06f1646240... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
