Flip the left/right speaker when the device is in yoga mode.

Flip the left/right speaker when the device is in yoga mode.

BUG=418656
Committed: https://crrev.com/cdeffd2c7a44895aafbf1f615cc3937ea2fd280a
Cr-Commit-Position: refs/heads/master@{#297766}

[jennyz, 2014-09-30 21:01:19]

jennyz@chromium.org changed reviewers:
+ cychiang@chromium.org, flackr@chromium.org

[jennyz, 2014-09-30 21:02:11]

jennyz@chromium.org changed reviewers:
+ oshima@chromium.org

[oshima, 2014-09-30 22:29:53]

https://codereview.chromium.org/620673002/diff/1/ash/display/display_manager.cc
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/620673002/diff/1/ash/display/display_manager....
ash/display/display_manager.cc:443: // Swap the left/right channel of the
speaker only if it is yoga mode.
1) Can you use gfx::DisplayObserver::OnDisplayMetricsChanged?

2) You need to check if the display is for internal display as you can rotate
external display.

3) You can rotate display on any display, so this happens when a user rotate
screen 180 on any chromebook. Is this ok?

https://codereview.chromium.org/620673002/diff/1/ash/system/audio/tray_audio_...
File ash/system/audio/tray_audio_delegate.h (right):

https://codereview.chromium.org/620673002/diff/1/ash/system/audio/tray_audio_...
ash/system/audio/tray_audio_delegate.h:47: virtual void SwapLeftRight(bool swap)
= 0;
It'd be better to have something like: (assuming this is how it works)

enum AudioChannelMode {  // Or SwapMode, LeftRightMode?
  NORMAL,
  LEFT_RIGHT_SWAPPED,
}

SetAutioChannelMode(AudioChannelMode);

[jennyz, 2014-10-01 00:22:59]

https://codereview.chromium.org/620673002/diff/1/ash/display/display_manager.cc
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/620673002/diff/1/ash/display/display_manager....
ash/display/display_manager.cc:443: // Swap the left/right channel of the
speaker only if it is yoga mode.
On 2014/09/30 22:29:53, oshima wrote:
> 1) Can you use gfx::DisplayObserver::OnDisplayMetricsChanged?
> 
> 2) You need to check if the display is for internal display as you can rotate
> external display.
> 
> 3) You can rotate display on any display, so this happens when a user rotate
> screen 180 on any chromebook. Is this ok?
Thanks for pointing me to gfx::DisplayObserver::OnDisplayMetricsChanged, this
makes the code much easier and clean.
I think it is ok to swap the audio channel when the display is rotated, which is
a rare case for user anyway.

https://codereview.chromium.org/620673002/diff/1/ash/system/audio/tray_audio_...
File ash/system/audio/tray_audio_delegate.h (right):

https://codereview.chromium.org/620673002/diff/1/ash/system/audio/tray_audio_...
ash/system/audio/tray_audio_delegate.h:47: virtual void SwapLeftRight(bool swap)
= 0;
On 2014/09/30 22:29:53, oshima wrote:
> It'd be better to have something like: (assuming this is how it works)
> 
> enum AudioChannelMode {  // Or SwapMode, LeftRightMode?
>   NORMAL,
>   LEFT_RIGHT_SWAPPED,
> }
> 
> SetAutioChannelMode(AudioChannelMode);
> 

Done.

[oshima, 2014-10-01 00:45:52]

one more.

https://codereview.chromium.org/620673002/diff/40001/ash/system/audio/tray_au...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/620673002/diff/40001/ash/system/audio/tray_au...
ash/system/audio/tray_audio.cc:160: gfx::Display::InternalDisplayId());
you should check if there is internal display (== kInvalidDisplayID)
and set to normal if there is none. (internal display can disappear when docked
mode)

[sadrul, 2014-10-01 00:54:09]

Drive-by: can I suggest having something (e.g. SpeakerFlipController) in
//ui/chromeos/ instead that can also be trivially instantiated and used in
athena?

[cychiang, 2014-10-01 02:36:29]

https://codereview.chromium.org/620673002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/620673002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.h:187: // Swaps the left and right channel of
the the primary active output device.
extra the

https://codereview.chromium.org/620673002/diff/40001/chromeos/dbus/cras_audio...
File chromeos/dbus/cras_audio_client.h (right):

https://codereview.chromium.org/620673002/diff/40001/chromeos/dbus/cras_audio...
chromeos/dbus/cras_audio_client.h:118: // Swaps the left and right channel of
the the primary active output device.
extra the

https://codereview.chromium.org/620673002/diff/40001/chromeos/dbus/cras_audio...
chromeos/dbus/cras_audio_client.h:119: // Swap the left and right channel if
|swap| is true; otherwise, swap the left
dbus message will be dropped if this feature is not supported on the node.
Should this be documented as well ?

[flackr, 2014-10-01 17:25:28]

https://codereview.chromium.org/620673002/diff/40001/ash/system/audio/tray_au...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/620673002/diff/40001/ash/system/audio/tray_au...
ash/system/audio/tray_audio.cc:160: gfx::Display::InternalDisplayId());
On 2014/10/01 00:45:52, oshima wrote:
> you should check if there is internal display (== kInvalidDisplayID)
> and set to normal if there is none. (internal display can disappear when
docked
> mode)

In which case we should call this from OnDisplayRemoved too right?

https://codereview.chromium.org/620673002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/620673002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:263: active_output_node_id_, swap);
I'm not too familiar with cras, but we should only swap the internal speaker
output (i.e. not a headset or attached speakers).

[jennyz, 2014-10-01 17:43:49]

On 2014/10/01 00:54:09, sadrul wrote:
> Drive-by: can I suggest having something (e.g. SpeakerFlipController) in
> //ui/chromeos/ instead that can also be trivially instantiated and used in
> athena?

Good point. It seems we are not sure about if any athena device will use this
feature, so I would like to keep it as such for now, it wouldn't be too much
work to refactor it to SpeakerFlipControoler /ui/chormos/ as you suggested later
when we are sure we will need this for athena.

[jennyz, 2014-10-01 17:43:54]

https://codereview.chromium.org/620673002/diff/40001/ash/system/audio/tray_au...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/620673002/diff/40001/ash/system/audio/tray_au...
ash/system/audio/tray_audio.cc:160: gfx::Display::InternalDisplayId());
On 2014/10/01 00:45:52, oshima wrote:
> you should check if there is internal display (== kInvalidDisplayID)
> and set to normal if there is none. (internal display can disappear when
docked
> mode)

Done.

https://codereview.chromium.org/620673002/diff/40001/ash/system/audio/tray_au...
ash/system/audio/tray_audio.cc:160: gfx::Display::InternalDisplayId());
On 2014/10/01 17:25:28, flackr wrote:
> On 2014/10/01 00:45:52, oshima wrote:
> > you should check if there is internal display (== kInvalidDisplayID)
> > and set to normal if there is none. (internal display can disappear when
> docked
> > mode)
> 
> In which case we should call this from OnDisplayRemoved too right?

Done.

https://codereview.chromium.org/620673002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/620673002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.h:187: // Swaps the left and right channel of
the the primary active output device.
On 2014/10/01 02:36:29, cychiang wrote:
> extra the

Done.

https://codereview.chromium.org/620673002/diff/40001/chromeos/dbus/cras_audio...
File chromeos/dbus/cras_audio_client.h (right):

https://codereview.chromium.org/620673002/diff/40001/chromeos/dbus/cras_audio...
chromeos/dbus/cras_audio_client.h:118: // Swaps the left and right channel of
the the primary active output device.
On 2014/10/01 02:36:29, cychiang wrote:
> extra the

Done.

https://codereview.chromium.org/620673002/diff/40001/chromeos/dbus/cras_audio...
chromeos/dbus/cras_audio_client.h:119: // Swap the left and right channel if
|swap| is true; otherwise, swap the left
On 2014/10/01 02:36:29, cychiang wrote:
> dbus message will be dropped if this feature is not supported on the node.
> Should this be documented as well ?

Done.

[flackr, 2014-10-01 20:32:33]

https://codereview.chromium.org/620673002/diff/80001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/620673002/diff/80001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:263: active_output_node_id_, swap);
We shouldn't swap the channels on any device except for the internal speakers:
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/audio/aud...

[jennyz, 2014-10-01 20:41:20]

https://codereview.chromium.org/620673002/diff/80001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/620673002/diff/80001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:263: active_output_node_id_, swap);
On 2014/10/01 20:32:33, flackr wrote:
> We shouldn't swap the channels on any device except for the internal speakers:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/audio/aud...

Cras decides if the channel swapping is supported, for the current code, as I
can see, only the internal speaker supports this feature. cychiang@, please
correct me if I am wrong.

[jennyz, 2014-10-01 20:52:11]

https://codereview.chromium.org/620673002/diff/80001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/620673002/diff/80001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:263: active_output_node_id_, swap);
On 2014/10/01 20:32:33, flackr wrote:
> We shouldn't swap the channels on any device except for the internal speakers:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/audio/aud...
Think again, you are right, we'd only guard to make this only applies to
internal speaker, will upload another patch soon, Thank!

[jennyz, 2014-10-01 21:30:34]

I have swapped the internal speaker's left/right channel based on the display
rotation regardless of the active node.

https://codereview.chromium.org/620673002/diff/80001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/620673002/diff/80001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:263: active_output_node_id_, swap);
On 2014/10/01 20:32:33, flackr wrote:
> We shouldn't swap the channels on any device except for the internal speakers:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/audio/aud...

Done.

[jennyz, 2014-10-01 21:30:46]

PTAL.

[oshima, 2014-10-01 21:53:56]

https://codereview.chromium.org/620673002/diff/100001/ash/system/audio/tray_a...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/620673002/diff/100001/ash/system/audio/tray_a...
ash/system/audio/tray_audio.cc:140: void TrayAudio::OnDisplayAdded(const
gfx::Display& new_display) {
you should do the same here, in case the device came back from docked mode.

[jennyz, 2014-10-01 22:03:51]

https://codereview.chromium.org/620673002/diff/100001/ash/system/audio/tray_a...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/620673002/diff/100001/ash/system/audio/tray_a...
ash/system/audio/tray_audio.cc:140: void TrayAudio::OnDisplayAdded(const
gfx::Display& new_display) {
On 2014/10/01 21:53:55, oshima wrote:
> you should do the same here, in case the device came back from docked mode.

Done.

[oshima, 2014-10-01 23:18:29]

my bits lgtm

Please address other reviewers comments before landing.

[flackr, 2014-10-02 00:39:19]

lgtm

https://codereview.chromium.org/620673002/diff/120001/ash/system/audio/tray_a...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/620673002/diff/120001/ash/system/audio/tray_a...
ash/system/audio/tray_audio.cc:173:
audio_delegate_->SetInternalSpeakerChannelMode(channel_mode);
This feels a little misplaced since it doesn't have anything to do with the tray
control and I think it will fire twice if you have two shelves (i.e. two
displays). Ideally I think we would have a separate controller which just
listens to display added/removed/changed and only flips channels.

[jennyz, 2014-10-02 00:43:33]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-10-02 00:44:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/620673002/120001

[commit-bot: I haz the power, 2014-10-02 02:06:55]

Committed patchset #7 (id:120001) as 075e48b45eca14a44256f6ec9d91639f10213608

[commit-bot: I haz the power, 2014-10-02 02:08:07]

Patchset 7 (id:??) landed as
https://crrev.com/cdeffd2c7a44895aafbf1f615cc3937ea2fd280a
Cr-Commit-Position: refs/heads/master@{#297766}