Support audio output device enumeration and selection in PPAPI

Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, tsepez@chromium.org, isherman@chromium.org

Review-Url: https://codereview.chromium.org/2755613002
Cr-Commit-Position: refs/heads/master@{#462889}
Committed: https://chromium.googlesource.com/chromium/src/+/3a2a470eed665050d284f1e5dd0e7a1c4949d190

[Xing, 2017-03-15 15:44:07]

Patchset #4 (id:60001) has been deleted

[Xing, 2017-03-15 15:48:33]

Description was changed from

==========
Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, jochen@chromium.org, dcheng@chromium.org
==========

to

==========
Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, jochen@chromium.org, dcheng@chromium.org
==========

[Xing, 2017-03-15 15:48:33]

xzhang@adobe.com changed reviewers:
+ piman@chromium.org, raymes@chromium.org
- dcheng@chromium.org, jochen@chromium.org

[Xing, 2017-03-15 15:48:57]

Description was changed from

==========
Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, jochen@chromium.org, dcheng@chromium.org
==========

to

==========
Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, raymes@chromium.org, piman@chromium.org
==========

[bbudge, 2017-03-23 18:05:46]

Overall, this looks reasonable. My major concern are the changes to
PepperPlatformAudioOutput. Even though this duplicates code, I think it would be
better to define a new class, rather than modify this class to work in two
different ways.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_audio_controller.cc (right):

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_controller.cc:37: }
It might be worth extracting two helper methods, StartPlaybackIfFirstInstance
and StopPlaybackIfLastInstance.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_audio_output_host.cc (right):

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_output_host.cc:264:
audio_output_->StartPlayback();
check for null?

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_audio_output.cc (right):

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.cc:137:
base::Bind(&PepperPlatformAudioOutput::NotifyStreamCreationFailed, this));
This looks like a change to the behavior of PPB_AudioOutput. Are you sure it's
benign?

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_audio_output.h (right):

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:38: // callbacks
properly.
Your changes to this class risk breaking the old PPB_Audio interface.

I think it would be better to create a separate class for the new behavior, e.g.
PepperPlatformAudioOutputDev.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:60: // Request
authorization to use the device.
nit: comment isn't necessary

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:111: // Used for old
Pepper audio interface.
nit: be specific, PPB_Audio

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:117: // Creates audio
stream. Used for new Pepper audio output interface.
ditto: PPB_AudioOutput_Dev

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:123: // Used only by old
Pepper audio interface.
ditto

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:126: void
RequestDeviceAuthorizationOnIOThread();
I assume these are only used to support PPB_AudioOutput_Dev. Could you segregate
the methods into two groups, for the two Pepper interfaces?

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:138: // TODO: This
PepperAudioOutputHost client will eventually replace
nit: chromium style requires TODO(owner), e.g. TODO(xing)

Do you really intend to do this work?

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:166: // Only used by
Unified IO.
What is Unified IO?

https://codereview.chromium.org/2755613002/diff/40001/ppapi/api/dev/ppb_audio...
File ppapi/api/dev/ppb_audio_output_dev.idl (right):

https://codereview.chromium.org/2755613002/diff/40001/ppapi/api/dev/ppb_audio...
ppapi/api/dev/ppb_audio_output_dev.idl:1: /* Copyright (c) 2016 The Chromium
Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2755613002/diff/40001/ppapi/c/BUILD.gn
File ppapi/c/BUILD.gn (right):

https://codereview.chromium.org/2755613002/diff/40001/ppapi/c/BUILD.gn#newcode95
ppapi/c/BUILD.gn:95: "dev/ppb_printing_dev.h",
duplicate

https://codereview.chromium.org/2755613002/diff/40001/ppapi/cpp/dev/audio_out...
File ppapi/cpp/dev/audio_output_dev.h (right):

https://codereview.chromium.org/2755613002/diff/40001/ppapi/cpp/dev/audio_out...
ppapi/cpp/dev/audio_output_dev.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
nit: 2017

[Xing, 2017-03-23 18:38:58]

Thanks bbudge, I will re-work on this change.

[Xing, 2017-03-29 20:40:53]

In patch set 4, PepperPlatformAudioOutput has been separated into 2 classes:
PepperPlatformAudioOutput and PepperPlatformAudioOutputDev.
pepper_platform_audio_output.cc{h} stays unchanged, and all the new
implementation has been moved into pepper_platform_audio_output_dev.cc{h}

[Xing, 2017-03-29 21:14:32]

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_audio_controller.cc (right):

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_controller.cc:37: }
On 2017/03/23 18:05:44, bbudge wrote:
> It might be worth extracting two helper methods, StartPlaybackIfFirstInstance
> and StopPlaybackIfLastInstance.

Done.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_audio_output_host.cc (right):

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_output_host.cc:264:
audio_output_->StartPlayback();
On 2017/03/23 18:05:44, bbudge wrote:
> check for null?

Done.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_audio_output.cc (right):

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.cc:137:
base::Bind(&PepperPlatformAudioOutput::NotifyStreamCreationFailed, this));
On 2017/03/23 18:05:44, bbudge wrote:
> This looks like a change to the behavior of PPB_AudioOutput. Are you sure it's
> benign?

New implementation has been moved to pepper_platform_audio_output_dev.cc{h},
and |OnError()| only does warning output.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_audio_output.h (right):

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:38: // callbacks
properly.
On 2017/03/23 18:05:45, bbudge wrote:
> Your changes to this class risk breaking the old PPB_Audio interface.
> 
> I think it would be better to create a separate class for the new behavior,
e.g.
> PepperPlatformAudioOutputDev.

New implementation has been moved to pepper_platform_audio_output_dev.cc{h}, new
class is called PepperPlatformAudioOutputDev.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:60: // Request
authorization to use the device.
On 2017/03/23 18:05:45, bbudge wrote:
> nit: comment isn't necessary

Done.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:111: // Used for old
Pepper audio interface.
On 2017/03/23 18:05:45, bbudge wrote:
> nit: be specific, PPB_Audio

Done.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:117: // Creates audio
stream. Used for new Pepper audio output interface.
On 2017/03/23 18:05:45, bbudge wrote:
> ditto: PPB_AudioOutput_Dev

Done.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:123: // Used only by old
Pepper audio interface.
On 2017/03/23 18:05:45, bbudge wrote:
> ditto

Done.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:126: void
RequestDeviceAuthorizationOnIOThread();
On 2017/03/23 18:05:44, bbudge wrote:
> I assume these are only used to support PPB_AudioOutput_Dev. Could you
segregate
> the methods into two groups, for the two Pepper interfaces?

Done.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:138: // TODO: This
PepperAudioOutputHost client will eventually replace
On 2017/03/23 18:05:45, bbudge wrote:
> nit: chromium style requires TODO(owner), e.g. TODO(xing)
> 
> Do you really intend to do this work?

Removed this TODO comment, since future modification to Pepper will not be
accepted.

https://codereview.chromium.org/2755613002/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_audio_output.h:166: // Only used by
Unified IO.
On 2017/03/23 18:05:44, bbudge wrote:
> What is Unified IO?

Removed.

https://codereview.chromium.org/2755613002/diff/40001/ppapi/api/dev/ppb_audio...
File ppapi/api/dev/ppb_audio_output_dev.idl (right):

https://codereview.chromium.org/2755613002/diff/40001/ppapi/api/dev/ppb_audio...
ppapi/api/dev/ppb_audio_output_dev.idl:1: /* Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2017/03/23 18:05:45, bbudge wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2755613002/diff/40001/ppapi/c/BUILD.gn
File ppapi/c/BUILD.gn (right):

https://codereview.chromium.org/2755613002/diff/40001/ppapi/c/BUILD.gn#newcode95
ppapi/c/BUILD.gn:95: "dev/ppb_printing_dev.h",
On 2017/03/23 18:05:45, bbudge wrote:
> duplicate

Done.

https://codereview.chromium.org/2755613002/diff/40001/ppapi/cpp/dev/audio_out...
File ppapi/cpp/dev/audio_output_dev.h (right):

https://codereview.chromium.org/2755613002/diff/40001/ppapi/cpp/dev/audio_out...
ppapi/cpp/dev/audio_output_dev.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
On 2017/03/23 18:05:45, bbudge wrote:
> nit: 2017

Done.

[bbudge, 2017-03-31 00:25:00]

Almost there, just a few things to fix.

https://codereview.chromium.org/2755613002/diff/80001/content/renderer/pepper...
File content/renderer/pepper/pepper_audio_controller.cc (right):

https://codereview.chromium.org/2755613002/diff/80001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_controller.cc:84:
StopPlaybackIfLastInstance();
I'm sorry if I led you astray here, the original test is !ppb_audios_.empty()
while StopPlayback tests the reverse condition. So you should call
NotifyPlaybackStopsOnEmpty if either array is non-empty.

https://codereview.chromium.org/2755613002/diff/80001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_controller.cc:101: return;
DCHECK(instance_);

Rather than if - statement, because of the tests in calling code, it's a
programming error if this is null here.

https://codereview.chromium.org/2755613002/diff/80001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_controller.cc:112: return;
DCHECK(instance_)

https://codereview.chromium.org/2755613002/diff/80001/ppapi/api/dev/ppb_audio...
File ppapi/api/dev/ppb_audio_output_dev.idl (right):

https://codereview.chromium.org/2755613002/diff/80001/ppapi/api/dev/ppb_audio...
ppapi/api/dev/ppb_audio_output_dev.idl:23: * @param[in] sample_buffer A buffer
to fill with audio data.
nit:s/in/out

https://codereview.chromium.org/2755613002/diff/80001/ppapi/thunk/ppb_audio_o...
File ppapi/thunk/ppb_audio_output_dev_thunk.cc (right):

https://codereview.chromium.org/2755613002/diff/80001/ppapi/thunk/ppb_audio_o...
ppapi/thunk/ppb_audio_output_dev_thunk.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
nit: regenerate files, date should match IDL

[Xing, 2017-03-31 18:23:30]

https://codereview.chromium.org/2755613002/diff/80001/content/renderer/pepper...
File content/renderer/pepper/pepper_audio_controller.cc (right):

https://codereview.chromium.org/2755613002/diff/80001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_controller.cc:84:
StopPlaybackIfLastInstance();
On 2017/03/31 00:24:59, bbudge wrote:
> I'm sorry if I led you astray here, the original test is !ppb_audios_.empty()
> while StopPlayback tests the reverse condition. So you should call
> NotifyPlaybackStopsOnEmpty if either array is non-empty.

It's totally my mistake, sorry about my careless copy/paste.

Fixed.

https://codereview.chromium.org/2755613002/diff/80001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_controller.cc:101: return;
On 2017/03/31 00:24:59, bbudge wrote:
> DCHECK(instance_);
> 
> Rather than if - statement, because of the tests in calling code, it's a
> programming error if this is null here.

Done.

https://codereview.chromium.org/2755613002/diff/80001/content/renderer/pepper...
content/renderer/pepper/pepper_audio_controller.cc:112: return;
On 2017/03/31 00:24:59, bbudge wrote:
> DCHECK(instance_)

Done.

https://codereview.chromium.org/2755613002/diff/80001/ppapi/api/dev/ppb_audio...
File ppapi/api/dev/ppb_audio_output_dev.idl (right):

https://codereview.chromium.org/2755613002/diff/80001/ppapi/api/dev/ppb_audio...
ppapi/api/dev/ppb_audio_output_dev.idl:23: * @param[in] sample_buffer A buffer
to fill with audio data.
On 2017/03/31 00:24:59, bbudge wrote:
> nit:s/in/out

Done.

https://codereview.chromium.org/2755613002/diff/80001/ppapi/thunk/ppb_audio_o...
File ppapi/thunk/ppb_audio_output_dev_thunk.cc (right):

https://codereview.chromium.org/2755613002/diff/80001/ppapi/thunk/ppb_audio_o...
ppapi/thunk/ppb_audio_output_dev_thunk.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2017/03/31 00:24:59, bbudge wrote:
> nit: regenerate files, date should match IDL

Done.

[bbudge, 2017-03-31 19:08:29]

LGTM

You'll need a security review for ppapi_messages.h and an OWNERS review for
histograms.xml. See the directories for people to add or I can do it.

[Xing, 2017-03-31 19:19:17]

On 2017/03/31 19:08:29, bbudge wrote:
> LGTM
> 
> You'll need a security review for ppapi_messages.h and an OWNERS review for
> histograms.xml. See the directories for people to add or I can do it.

Thanks bbudge!

I checked "ipc/SECURITY_OWNERS" and "tools/metrics/OWNERS", there are quite a
few people, I need your help on this part, Thanks!


ipc/SECURITY_OWNERS:

dcheng@chromium.org
estark@chromium.org
kenrb@chromium.org
meacer@chromium.org
mbarbella@chromium.org
mkwst@chromium.org
nasko@chromium.org
ochang@chromium.org
palmer@chromium.org
rsesek@chromium.org
tsepez@chromium.org
wfh@chromium.org

tools/metrics/OWNERS:

asvitkine@chromium.org
holte@chromium.org
isherman@chromium.org
jwd@chromium.org
mpearson@chromium.org
rkaplow@chromium.org

[bbudge, 2017-03-31 19:28:10]

bbudge@chromium.org changed reviewers:
+ isherman@chromium.org, tsepez@chromium.org

[bbudge, 2017-03-31 19:28:10]

+tsepez for ppapi_messages.h
+isherman for histograms

[bbudge, 2017-03-31 19:29:27]

On 2017/03/31 19:28:10, bbudge wrote:
> +tsepez for ppapi_messages.h
> +isherman for histograms

For questions about PPAPI, see laforge.

[Tom Sepez, 2017-03-31 19:30:46]

LGTM given the existing audio input messages, 
but what do we mean std::string /* device_id */,
and what happens when evil values are passed here?

[Xing, 2017-03-31 20:29:26]

On 2017/03/31 19:30:46, Tom Sepez wrote:
> LGTM given the existing audio input messages, 
> but what do we mean std::string /* device_id */,
> and what happens when evil values are passed here?

device_id is the name of the audio input/output device, it is passed around in
std::string object. If evil values are passed here, it should be rejected by
renderer_host who handles audio input/output request.

[Tom Sepez, 2017-03-31 20:32:13]

> device_id is the name of the audio input/output device, it is passed around in
> std::string object. If evil values are passed here, it should be rejected by
> renderer_host who handles audio input/output request.
Great, can you point me at the code that does this rejection?  Just paranoid
because it seems like this string might wind up in a syscall or something ...

[bbudge, 2017-03-31 21:10:57]

On 2017/03/31 20:32:13, Tom Sepez wrote:
> > device_id is the name of the audio input/output device, it is passed around
in
> > std::string object. If evil values are passed here, it should be rejected by
> > renderer_host who handles audio input/output request.
> Great, can you point me at the code that does this rejection?  Just paranoid
> because it seems like this string might wind up in a syscall or something ...

I traced this down and the renderer sends a message to the browser, which is
received here:

https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...

Then here:

https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...

And the string is validated here:

https://cs.chromium.org/chromium/src/content/public/browser/media_device_id.c...

[Tom Sepez, 2017-03-31 21:15:02]

On 2017/03/31 21:10:57, bbudge wrote:
> On 2017/03/31 20:32:13, Tom Sepez wrote:
> > > device_id is the name of the audio input/output device, it is passed
around
> in
> > > std::string object. If evil values are passed here, it should be rejected
by
> > > renderer_host who handles audio input/output request.
> > Great, can you point me at the code that does this rejection?  Just paranoid
> > because it seems like this string might wind up in a syscall or something
...
> 
> I traced this down and the renderer sends a message to the browser, which is
> received here:
> 
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...
> 
> Then here:
> 
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...
> 
> And the string is validated here:
> 
>
https://cs.chromium.org/chromium/src/content/public/browser/media_device_id.c...

Thanks for the effort. That looks OK to me.

[Xing, 2017-03-31 21:26:00]

On 2017/03/31 21:10:57, bbudge wrote:
> On 2017/03/31 20:32:13, Tom Sepez wrote:
> > > device_id is the name of the audio input/output device, it is passed
around
> in
> > > std::string object. If evil values are passed here, it should be rejected
by
> > > renderer_host who handles audio input/output request.
> > Great, can you point me at the code that does this rejection?  Just paranoid
> > because it seems like this string might wind up in a syscall or something
...
> 
> I traced this down and the renderer sends a message to the browser, which is
> received here:
> 
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...
> 
> Then here:
> 
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...
> 
> And the string is validated here:
> 
>
https://cs.chromium.org/chromium/src/content/public/browser/media_device_id.c...

Thanks bbudge!

For Open request, the following code also shows the validation process of
device_id:

PepperPlatformAudioInput::Initialize()
https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_platform_...

PepperMediaDeviceManager::OpenDevice()
https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_media_dev...

MediaStreamDispatcher::OpenDevice()
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_disp...

MediaStreamHostMsg_OpenDevice => OnOpenDevice
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

MediaStreamDispatcherHost::OnOpenDevice()
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

MediaStreamManager::OpenDevice()
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

MediaStreamManager::SetupRequest()
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

MediaStreamManager::SetupDeviceCaptureRequest()
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

MediaStreamManager::GetRequestedDeviceCaptureId()
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

MediaStreamManager::PickDeviceId()
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

GetDeviceIDFromHMAC():
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

[Ilya Sherman, 2017-03-31 21:41:03]

histograms.xml lgtm

[Xing, 2017-03-31 22:00:47]

Description was changed from

==========
Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, raymes@chromium.org, piman@chromium.org
==========

to

==========
Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, tsepez@chromium.org, isherman@chromium.org
==========

[Xing, 2017-03-31 22:02:42]

The CQ bit was checked by xzhang@adobe.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:03:46]

Dry run: 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-03-31 22:25:44]

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

[commit-bot: I haz the power, 2017-03-31 22:25:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xing, 2017-03-31 22:47:53]

The CQ bit was checked by xzhang@adobe.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:48:38]

Dry run: 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-04-01 01:20:14]

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

[commit-bot: I haz the power, 2017-04-01 01:20:15]

Dry run: This issue passed the CQ dry run.

[Xing, 2017-04-01 03:29:57]

The CQ bit was checked by xzhang@adobe.com

[Xing, 2017-04-01 03:29:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
bbudge@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2755613002/#ps120001
(title: "Fix trybot failures, Rebase")

[commit-bot: I haz the power, 2017-04-01 03:30:09]

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-04-01 03:42:45]

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

[commit-bot: I haz the power, 2017-04-01 03:42:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Xing, 2017-04-03 15:27:29]

xzhang@adobe.com changed reviewers:
+ jochen@chromium.org

[Xing, 2017-04-03 15:29:19]

+jochen for the following files:

chrome/common/ppapi_utils.cc
content/renderer/BUILD.gn

[jochen (gone - plz use gerrit), 2017-04-05 08:18:25]

Could somebody please give me some more background on why this change is needed
and desirable for chromium?

[Xing, 2017-04-05 14:34:31]

On 2017/04/05 08:18:25, jochen wrote:
> Could somebody please give me some more background on why this change is
needed
> and desirable for chromium?

Adobe got the requirement to support audio output device selection in Flash.
Since PPAPI does not provide the interface to enumerate and select audio output,
we have to hook it up so that Pepper Flash can support this feature.

[Xing, 2017-04-06 17:52:22]

Hi jochen,

Is there any further information you would like us to share about this feature
request?

Thanks,
Xing

[jochen (gone - plz use gerrit), 2017-04-07 06:54:24]

I was under the impression that the PPAPI was already frozen, but apparently
this is supposed to be the last change to it, so lgtm

[Xing, 2017-04-07 15:13:32]

The CQ bit was checked by xzhang@adobe.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 15:13:50]

Dry run: 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-04-07 16:26:25]

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

[commit-bot: I haz the power, 2017-04-07 16:26:26]

Dry run: This issue passed the CQ dry run.

[Xing, 2017-04-07 16:27:16]

The CQ bit was checked by xzhang@adobe.com

[Xing, 2017-04-07 16:27:17]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
jochen@chromium.org, tsepez@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2755613002/#ps140001
(title: "Fix format, Rebase")

[commit-bot: I haz the power, 2017-04-07 16:27:53]

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-04-07 16:34:38]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491582436935240,
"parent_rev": "26d2eea286b0c0ecc81ef2cf52815942d788ce1b", "commit_rev":
"3a2a470eed665050d284f1e5dd0e7a1c4949d190"}

[commit-bot: I haz the power, 2017-04-07 16:35:26]

Description was changed from

==========
Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, tsepez@chromium.org, isherman@chromium.org
==========

to

==========
Support audio output device enumeration and selection in PPAPI

- Keep the current |PPB_Audio| interface unchanged
- Add a new dev interface: |PPB_AudioOutput_Dev|
- Implement |PPB_AudioOutput_Dev| using the typical HostResource
  implementations like |PPB_AudioInput_Dev|
- Add ppapi/proxy/audio_output_resource.h/cc etc. to implement
  |AudioOutputResource|
- Add content/renderer/pepper/pepper_audio_output_host.h/cc etc.
  to implement |PepperAudioOutputHost|
- Keep ppb_audio_proxy.h/cc, ppb_audio_shared.h/cc, ppb_audio_impl.h/cc
  unchanged, so that the current |PPB_Audio| will not be affected.
- Modify content/renderer/pepper/pepper_platform_audio_output.h/cc to
  accept both |AudioHelper| and |PepperAudioOutputHost| clients
- The way to implement |PepperAudioOutputHost| is borrowed from
  "media/audio/audio_output_device.cc" where audio output selection isi
  supported for javascript.
- Add necessary IPC message definitions for |PPB_AudioOutput_Dev|

BUG=701584
R= bbudge@chromium.org, tsepez@chromium.org, isherman@chromium.org

Review-Url: https://codereview.chromium.org/2755613002
Cr-Commit-Position: refs/heads/master@{#462889}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a2a470eed665050d284f1e5dd0e...
==========

[commit-bot: I haz the power, 2017-04-07 16:35:34]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3a2a470eed665050d284f1e5dd0e...

[Xing, 2017-04-07 22:41:21]

On 2017/04/07 06:54:24, jochen wrote:
> I was under the impression that the PPAPI was already frozen, but apparently
> this is supposed to be the last change to it, so lgtm

Thanks jochen!