Introduce AudioIPCFactory.

Introduce AudioIPCFactory.

AudioIPCFactory is introduced to manage RendererAudioOutputStreamFactory
pointers, which is needed for various reasons.
* When processing an authorization request, it is not safe to use other
  threads than the IO thread, as they may be blocked waiting for the
  response. Thus, AudioIPCFactory is introduced to hold a mapping
  (frame id->remote factory) on the IO thread. This also means we have
  to create/destruct the RendererAudioOutputStreamFactoryPtr with the
  RenderFrameImpl instead of the preferrable way of doing it lazily.
  This should be revisited if we stop blocking while waiting for
  authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
  needed. I have verified that the browser tests pass. As the pepper
  code doesn't seem well covered by tests, I also manually verified
  that flash sound output works.
* There's a class diagram at https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbrn8/edit?usp=sharing

BUG=425368
Review-Url: https://codereview.chromium.org/2890753003
Cr-Commit-Position: refs/heads/master@{#476612}
Committed: https://chromium.googlesource.com/chromium/src/+/017ba8c602a378a6f508cf642ef81ed46eea5dbe

[Max Morin, 2017-05-17 11:22:14]

Patchset #1 (id:1) has been deleted

[Max Morin, 2017-05-17 12:41:13]

Patchset #1 (id:20001) has been deleted

[Max Morin, 2017-05-17 12:43:21]

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

[commit-bot: I haz the power, 2017-05-17 12:43:40]

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-05-17 14:38:35]

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

[commit-bot: I haz the power, 2017-05-17 14:38:36]

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

[Max Morin, 2017-05-19 10:01:11]

Patchset #1 (id:40001) has been deleted

[Max Morin, 2017-05-22 14:00:20]

Patchset #1 (id:60001) has been deleted

[Max Morin, 2017-05-22 14:00:28]

Patchset #1 (id:80001) has been deleted

[Max Morin, 2017-05-22 14:00:49]

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

[commit-bot: I haz the power, 2017-05-22 14:01:08]

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-05-22 15:19:20]

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

[commit-bot: I haz the power, 2017-05-22 15:19:21]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Max Morin, 2017-05-30 15:40:05]

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

[commit-bot: I haz the power, 2017-05-30 15:40:27]

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-05-30 18:22:55]

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

[commit-bot: I haz the power, 2017-05-30 18:22:56]

Dry run: This issue passed the CQ dry run.

[Max Morin, 2017-05-31 11:01:36]

Patchset #2 (id:120001) has been deleted

[Max Morin, 2017-05-31 11:01:44]

Patchset #2 (id:140001) has been deleted

[Max Morin, 2017-05-31 11:01:57]

Patchset #3 (id:180001) has been deleted

[Max Morin, 2017-05-31 11:02:05]

Patchset #3 (id:200001) has been deleted

[Max Morin, 2017-05-31 11:02:12]

Patchset #3 (id:220001) has been deleted

[Max Morin, 2017-05-31 11:02:38]

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

[commit-bot: I haz the power, 2017-05-31 11:02:45]

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-05-31 12:29:21]

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

[commit-bot: I haz the power, 2017-05-31 12:29:22]

Dry run: This issue passed the CQ dry run.

[Max Morin, 2017-05-31 15:26:58]

maxmorin@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Max Morin, 2017-05-31 15:26:59]

Hey Dale. Olga mostly reviewed this before I separated it out from the last CL.
PTAL.

[DaleCurtis, 2017-05-31 20:48:10]

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
File content/renderer/media/audio_ipc_factory.cc (right):

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.cc:67: if
(!io_task_runner_->BelongsToCurrentThread()) {
Since MaybeRegisterRemoteFactory() doesn't add the frame_id to factory_ptrs, it
seems you may never remove it if register / deregister happen back to back? Add
a unittest for this case too.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
File content/renderer/media/audio_ipc_factory.h (right):

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.h:43: const
scoped_refptr<AudioMessageFilter>& audio_message_filter,
No const&.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.h:49: const
scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner() const {
Ditto.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.h:53: // Enables |this| to create
MojoAudioOutputIPCs for the specified frame.
Comment needs to qualify why this is labeled Maybe.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.h:58: // Every call to the above method
must be matched by a call to this one when
Ditto.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1226: if (AudioIPCFactory::get())
If you like, you can write this as:

if (auto* factory = get())
  factory->...()

[Max Morin, 2017-06-01 12:34:28]

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

[commit-bot: I haz the power, 2017-06-01 12:34:38]

Dry run: CQ is trying da patch.

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

[Max Morin, 2017-06-01 13:58:42]

maxmorin@chromium.org changed reviewers:
+ avi@chromium.org, bbudge@chromium.org

[Max Morin, 2017-06-01 13:58:43]

Thanks Dale.
Bbudge: PTAL at c/r/pepper.
Avi: PTAL at content other than pepper and media.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
File content/renderer/media/audio_ipc_factory.cc (right):

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.cc:67: if
(!io_task_runner_->BelongsToCurrentThread()) {
On 2017/05/31 20:48:09, DaleCurtis wrote:
> Since MaybeRegisterRemoteFactory() doesn't add the frame_id to factory_ptrs,
it
> seems you may never remove it if register / deregister happen back to back?
Add
> a unittest for this case too.

Hmm, this is only called from the main thread in practice, so it's safe because
of that. On the other hand, adding such conditions to the API isn't ideal, so
I'll change this to always post, and move the erase call to a separate function.
This will ensure that tasks are executed on the IO thread in the order we expect
them to. Also added test.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
File content/renderer/media/audio_ipc_factory.h (right):

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.h:43: const
scoped_refptr<AudioMessageFilter>& audio_message_filter,
On 2017/05/31 20:48:09, DaleCurtis wrote:
> No const&.

Done.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.h:49: const
scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner() const {
On 2017/05/31 20:48:09, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.h:53: // Enables |this| to create
MojoAudioOutputIPCs for the specified frame.
On 2017/05/31 20:48:09, DaleCurtis wrote:
> Comment needs to qualify why this is labeled Maybe.

Done.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/media...
content/renderer/media/audio_ipc_factory.h:58: // Every call to the above method
must be matched by a call to this one when
On 2017/05/31 20:48:09, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2890753003/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1226: if (AudioIPCFactory::get())
On 2017/05/31 20:48:09, DaleCurtis wrote:
> If you like, you can write this as:
> 
> if (auto* factory = get())
>   factory->...()

Done.

[commit-bot: I haz the power, 2017-06-01 14:03:51]

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

[commit-bot: I haz the power, 2017-06-01 14:03:52]

Dry run: This issue passed the CQ dry run.

[Max Morin, 2017-06-01 14:06:28]

Description was changed from

==========
Introduce AudioIPCFactory.

AudioIPCFactory is introduced to manage RendererAudioOutputStreamFactory
pointers, which is needed for various reasons.
* When processing an authorization request, it is not safe to use other
  threads than the IO thread, as they may be blocked waiting for the
  response. Thus, AudioIPCFactory is introduced to hold a mapping
  (frame id->remote factory) on the IO thread. This also means we have
  to create/destruct the RendererAudioOutputStreamFactoryPtr with the
  RenderFrameImpl instead of the preferrable way of doing it lazily.
  This should be revisited if we stop blocking while waiting for
  authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
  needed. I have verified that the browser tests pass. As the pepper
  code doesn't seem well covered by tests, I also manually verified
  that flash sound output works.
* There's a class diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...

BUG=
==========

to

==========
Introduce AudioIPCFactory.

AudioIPCFactory is introduced to manage RendererAudioOutputStreamFactory
pointers, which is needed for various reasons.
* When processing an authorization request, it is not safe to use other
  threads than the IO thread, as they may be blocked waiting for the
  response. Thus, AudioIPCFactory is introduced to hold a mapping
  (frame id->remote factory) on the IO thread. This also means we have
  to create/destruct the RendererAudioOutputStreamFactoryPtr with the
  RenderFrameImpl instead of the preferrable way of doing it lazily.
  This should be revisited if we stop blocking while waiting for
  authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
  needed. I have verified that the browser tests pass. As the pepper
  code doesn't seem well covered by tests, I also manually verified
  that flash sound output works.
* There's a class diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...

BUG=425368
==========

[Avi (use Gerrit), 2017-06-01 15:16:12]

lgtm

[DaleCurtis, 2017-06-01 17:05:03]

lgtm

[bbudge, 2017-06-01 17:26:04]

content/renderer/pepper lgtm

[Max Morin, 2017-06-02 10:18:08]

The CQ bit was checked by maxmorin@chromium.org

[commit-bot: I haz the power, 2017-06-02 10:18:20]

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-06-02 10:23:12]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496398688844530,
"parent_rev": "efffaf05f0a47a357ab6955c0a3caf7a4a06ef65", "commit_rev":
"017ba8c602a378a6f508cf642ef81ed46eea5dbe"}

[commit-bot: I haz the power, 2017-06-02 10:23:23]

Description was changed from

==========
Introduce AudioIPCFactory.

AudioIPCFactory is introduced to manage RendererAudioOutputStreamFactory
pointers, which is needed for various reasons.
* When processing an authorization request, it is not safe to use other
  threads than the IO thread, as they may be blocked waiting for the
  response. Thus, AudioIPCFactory is introduced to hold a mapping
  (frame id->remote factory) on the IO thread. This also means we have
  to create/destruct the RendererAudioOutputStreamFactoryPtr with the
  RenderFrameImpl instead of the preferrable way of doing it lazily.
  This should be revisited if we stop blocking while waiting for
  authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
  needed. I have verified that the browser tests pass. As the pepper
  code doesn't seem well covered by tests, I also manually verified
  that flash sound output works.
* There's a class diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...

BUG=425368
==========

to

==========
Introduce AudioIPCFactory.

AudioIPCFactory is introduced to manage RendererAudioOutputStreamFactory
pointers, which is needed for various reasons.
* When processing an authorization request, it is not safe to use other
  threads than the IO thread, as they may be blocked waiting for the
  response. Thus, AudioIPCFactory is introduced to hold a mapping
  (frame id->remote factory) on the IO thread. This also means we have
  to create/destruct the RendererAudioOutputStreamFactoryPtr with the
  RenderFrameImpl instead of the preferrable way of doing it lazily.
  This should be revisited if we stop blocking while waiting for
  authorization (crbug.com/668275).
* For running browser tests with this, the browser-side glue code is
  needed. I have verified that the browser tests pass. As the pepper
  code doesn't seem well covered by tests, I also manually verified
  that flash sound output works.
* There's a class diagram at
https://docs.google.com/drawings/d/1U1e-0gH3WVSV9Td9ZCEcPDBMC7WWWeh58sRYAdXbr...

BUG=425368

Review-Url: https://codereview.chromium.org/2890753003
Cr-Commit-Position: refs/heads/master@{#476612}
Committed:
https://chromium.googlesource.com/chromium/src/+/017ba8c602a378a6f508cf642ef8...
==========

[commit-bot: I haz the power, 2017-06-02 10:23:24]

Committed patchset #4 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/017ba8c602a378a6f508cf642ef8...