ChromeOS: enable pinning stream output routing for webapp request [extracted from 2516813002]

ChromeOS: enable pinning stream output routing for webapp request

Changes:
This CL is extracted from https://codereview.chromium.org/2516813002/, dealing with 666202 only. Support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE.

The mirror bug of 658048 exists, that is selecting 'Headphone', it will still route to internal speaker, unless headphone is activated by system selection.

BUG=666202
TEST=manual test and expect the results: "The drop down for audio input and output from a web page should show a list of available devices plus "default".  Choosing one from a web page will route only that page's audio to/from the selected device."

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2651073004
Cr-Commit-Position: refs/heads/master@{#446539}
Committed: https://chromium.googlesource.com/chromium/src/+/03d77dd3e66409c87e9654f3db112fe7b298f925

[Qiang(Joe) Xu, 2017-01-25 19:38:18]

Description was changed from

==========
ChromeOS: enable pinning stream output routing for webapp request

BUG=666202
==========

to

==========
ChromeOS: enable pinning stream output routing for webapp request

BUG=666202
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Qiang(Joe) Xu, 2017-01-25 19:41:59]

Description was changed from

==========
ChromeOS: enable pinning stream output routing for webapp request

BUG=666202
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
ChromeOS: enable pinning stream output routing for webapp request

Changes:
This CL is extracted from https://codereview.chromium.org/2516813002/, dealing
with 666202 only. Support pinning stream for output, the behavior reverts to
default if pin_device_ is NO_DEVICE.

The mirror bug of 658048 exists, that is selecting 'Headphone', it will still
route to internal speaker, unless headphone is activated by system selection.

BUG=666202
TEST=manual test and expect the results: "The drop down for audio input and
output from a web page should show a list of available devices plus "default". 
Choosing one from a web page will route only that page's audio to/from the
selected device."

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Qiang(Joe) Xu, 2017-01-25 19:41:59]

warx@chromium.org changed reviewers:
+ dgreid@chromium.org, tommi@chromium.org

[Qiang(Joe) Xu, 2017-01-25 19:42:08]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 19:42:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2017-01-25 19:43:41]

Hi dgreid@, tommi@, ptal:

It is extracted from https://codereview.chromium.org/2516813002/ without dealing
with crbug.com/658048.

The mirror bug of 658048 exists, that is selecting 'Headphone', it will still
route to internal speaker, unless headphone is activated by system selection.

[Qiang(Joe) Xu, 2017-01-25 19:45:22]

In this CL, there is no frustrated 'audio label issue'. So it can be landed and
be tested.

[tommi (sloooow) - chröme, 2017-01-25 22:05:28]

I've got some minor suggestions below but overall lgtm.

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
File media/audio/cras/audio_manager_cras.cc (right):

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
media/audio/cras/audio_manager_cras.cc:239: // (warx): pinning stream is not
supported for MakeLinearOutputStream.
nit: as is this reads like it was meant to be a TODO, so I suggest to just
remove the "(warx):" part.

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
media/audio/cras/audio_manager_cras.cc:322: AudioDeviceName device_name =
device_names.front();
nit: no need to copy to a new AudioDeviceName instance. Instead do:

const AudioDeviceName& device_name = device_names.front();

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
File media/audio/cras/audio_manager_cras.h (right):

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
media/audio/cras/audio_manager_cras.h:55: // Checks if |device_id| corresponds
to 'Default' |is_input| choice.
nit: 

// Checks if |device_id| corresponds to the default device.
// Set |is_input| to true for capture devices, false for output.

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
media/audio/cras/cras_unified.cc:71: output_bus_ = AudioBus::Create(params);
initialize in the initializer list?

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
media/audio/cras/cras_unified.cc:75: pin_device_ = dev_index_of(cras_node_id);
in order to make pin_device_ const, you could extract this part out to a little
helper function:

namespace media {
namespace {
int GetDevicePin(AudioManagerCras* manager, const std::string& device_id) {
  if (manager->IsDefault(device_id, false)) {
    uint64_t cras_node_id;
    base::StringToUint64(device_id, &cras_node_id);
    return dev_index_of(cras_node_id);
  }
  return NO_DEVICE;
}
}  // namespace

CrasUnifiedStream::CrasUnifiedStream(...)
    : client_(NULL),
      ...
      pin_device_(GetDevicePin(manager, device_id)) {
...

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
File media/audio/cras/cras_unified.h (right):

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
media/audio/cras/cras_unified.h:117: int pin_device_;
could this variable be made const?

[commit-bot: I haz the power, 2017-01-25 22:39:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 22:39:11]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2017-01-25 22:50:18]

New patch uploaded based on comments, thanks!

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
File media/audio/cras/audio_manager_cras.cc (right):

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
media/audio/cras/audio_manager_cras.cc:239: // (warx): pinning stream is not
supported for MakeLinearOutputStream.
On 2017/01/25 22:05:28, tommi (krómíum) wrote:
> nit: as is this reads like it was meant to be a TODO, so I suggest to just
> remove the "(warx):" part.

Done.

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
media/audio/cras/audio_manager_cras.cc:322: AudioDeviceName device_name =
device_names.front();
On 2017/01/25 22:05:28, tommi (krómíum) wrote:
> nit: no need to copy to a new AudioDeviceName instance. Instead do:
> 
> const AudioDeviceName& device_name = device_names.front();

Done.

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
File media/audio/cras/audio_manager_cras.h (right):

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/audio_mana...
media/audio/cras/audio_manager_cras.h:55: // Checks if |device_id| corresponds
to 'Default' |is_input| choice.
On 2017/01/25 22:05:28, tommi (krómíum) wrote:
> nit: 
> 
> // Checks if |device_id| corresponds to the default device.
> // Set |is_input| to true for capture devices, false for output.

Done.

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
media/audio/cras/cras_unified.cc:71: output_bus_ = AudioBus::Create(params);
On 2017/01/25 22:05:28, tommi (krómíum) wrote:
> initialize in the initializer list?

Done.

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
media/audio/cras/cras_unified.cc:75: pin_device_ = dev_index_of(cras_node_id);
On 2017/01/25 22:05:28, tommi (krómíum) wrote:
> in order to make pin_device_ const, you could extract this part out to a
little
> helper function:
> 
> namespace media {
> namespace {
> int GetDevicePin(AudioManagerCras* manager, const std::string& device_id) {
>   if (manager->IsDefault(device_id, false)) {
>     uint64_t cras_node_id;
>     base::StringToUint64(device_id, &cras_node_id);
>     return dev_index_of(cras_node_id);
>   }
>   return NO_DEVICE;
> }
> }  // namespace
> 
> CrasUnifiedStream::CrasUnifiedStream(...)
>     : client_(NULL),
>       ...
>       pin_device_(GetDevicePin(manager, device_id)) {
> ...
> 

Done.

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
File media/audio/cras/cras_unified.h (right):

https://codereview.chromium.org/2651073004/diff/1/media/audio/cras/cras_unifi...
media/audio/cras/cras_unified.h:117: int pin_device_;
On 2017/01/25 22:05:28, tommi (krómíum) wrote:
> could this variable be made const?

Done.

[Qiang(Joe) Xu, 2017-01-27 00:49:44]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-01-27 00:49:44]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2651073004/#ps20001
(title: "comments from ps1")

[commit-bot: I haz the power, 2017-01-27 00:50:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 02:19:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485478184304400,
"parent_rev": "827b0c2d5be45551cf28747392ea269553820900", "commit_rev":
"03d77dd3e66409c87e9654f3db112fe7b298f925"}

[commit-bot: I haz the power, 2017-01-27 02:20:30]

Description was changed from

==========
ChromeOS: enable pinning stream output routing for webapp request

Changes:
This CL is extracted from https://codereview.chromium.org/2516813002/, dealing
with 666202 only. Support pinning stream for output, the behavior reverts to
default if pin_device_ is NO_DEVICE.

The mirror bug of 658048 exists, that is selecting 'Headphone', it will still
route to internal speaker, unless headphone is activated by system selection.

BUG=666202
TEST=manual test and expect the results: "The drop down for audio input and
output from a web page should show a list of available devices plus "default". 
Choosing one from a web page will route only that page's audio to/from the
selected device."

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
ChromeOS: enable pinning stream output routing for webapp request

Changes:
This CL is extracted from https://codereview.chromium.org/2516813002/, dealing
with 666202 only. Support pinning stream for output, the behavior reverts to
default if pin_device_ is NO_DEVICE.

The mirror bug of 658048 exists, that is selecting 'Headphone', it will still
route to internal speaker, unless headphone is activated by system selection.

BUG=666202
TEST=manual test and expect the results: "The drop down for audio input and
output from a web page should show a list of available devices plus "default". 
Choosing one from a web page will route only that page's audio to/from the
selected device."

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2651073004
Cr-Commit-Position: refs/heads/master@{#446539}
Committed:
https://chromium.googlesource.com/chromium/src/+/03d77dd3e66409c87e9654f3db11...
==========

[commit-bot: I haz the power, 2017-01-27 02:20:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/03d77dd3e66409c87e9654f3db11...

[tommi (sloooow) - chröme, 2017-01-27 09:07:05]

On 2017/01/27 02:20:35, commit-bot: I haz the power wrote:
> Committed patchset #2 (id:20001) as
>
https://chromium.googlesource.com/chromium/src/+/03d77dd3e66409c87e9654f3db11...

I was expecting Dylan to have a look and approve before landing.  Can you make
sure he's OK with the change?

[dgreid, 2017-01-27 17:22:55]

On Fri, Jan 27, 2017 at 01:07:05AM -0800, tommi@chromium.org wrote:
> On 2017/01/27 02:20:35, commit-bot: I haz the power wrote:
> > Committed patchset #2 (id:20001) as
> 
>
https://chromium.googlesource.com/chromium/src/+/03d77dd3e66409c87e9654f3db11...
> 
> I was expecting Dylan to have a look and approve before landing.  Can you
> make
> sure he's OK with the change?

Thanks Tommi, warx and I talked offline.  I'm OK with the change and
wanted to get it in as early as possible because 58 is a short cycle for
Chome OS.

> 
> https://codereview.chromium.org/2651073004/
> 

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommi (sloooow) - chröme, 2017-01-27 18:27:31]

Great. Thanks for the update.

On Fri, Jan 27, 2017, 18:22 Dylan Reid <dgreid@chromium.org> wrote:

> On Fri, Jan 27, 2017 at 01:07:05AM -0800, tommi@chromium.org wrote:
> > On 2017/01/27 02:20:35, commit-bot: I haz the power wrote:
> > > Committed patchset #2 (id:20001) as
> >
> >
>
https://chromium.googlesource.com/chromium/src/+/03d77dd3e66409c87e9654f3db11...
> >
> > I was expecting Dylan to have a look and approve before landing.  Can you
> > make
> > sure he's OK with the change?
>
> Thanks Tommi, warx and I talked offline.  I'm OK with the change and
> wanted to get it in as early as possible because 58 is a short cycle for
> Chome OS.
>
> >
> > https://codereview.chromium.org/2651073004/
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.