Add mojo interface for audio rendering.

Split into https://codereview.chromium.org/2697793002/ + https://codereview.chromium.org/2697663003/

BUG=425368
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

[Max Morin, 2016-09-07 08:35:55]

Description was changed from

==========
Add interface.

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

to

==========
Add mojo interface for audio rendering.

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

[Max Morin, 2016-09-07 08:35:55]

maxmorin@chromium.org changed reviewers:
+ grunell@chromium.org, olka@chromium.org

[Max Morin, 2016-09-07 08:38:59]

New Cl for this. Maybe the implementation should go into this CL as well for the
mojo security review?

[Henrik Grunell, 2016-09-12 10:33:00]

On 2016/09/07 08:38:59, Max Morin Chromium wrote:
> New Cl for this. Maybe the implementation should go into this CL as well for
the
> mojo security review?

According to nasko@, yes. See https://codereview.chromium.org/1896883002/#msg127

[Max Morin, 2016-09-28 07:41:41]

Description was changed from

==========
Add mojo interface for audio rendering.

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

to

==========
Add mojo interface for audio rendering. Class diagram:
https://docs.google.com/a/chromium.org/drawings/d/1C4nc2ZCDNr3jgxZJFYA-1hbW7t....
Current state: AudioOutputImpl and mojom ready for review. No point in looking
at AudioOutputServiceImpl right now.

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

[Max Morin, 2016-09-28 07:55:05]

Olga: If you like, you can have a sneak peek at AudioOutputImpl while I'm
working on AudioOutputServiceImpl.

[o1ka, 2016-09-28 10:24:07]

Thank you, it was an interesting reading :)
Some random comments below.

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_impl.cc (right):

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:91: Close();
Here and in other places: what is the purpose of Close() here? Is it error
processing? Would DCHECKs be enough? Anyways, some logging and comments needed
to explain what has happened

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:108: if (!IsStarted()
|| volume < 0.0 || volume > 1.0)
Where these volume limits come from? And what if they change there?

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:117:
base::Bind(&AudioOutputImpl::DoCompleteCreation, base::Unretained(this)));
"this" won't survive

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:120: void
AudioOutputImpl::OnPlaying() {
Thread check?  and what if it is called after Close has been executed?

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:124:
host_->NotifyStreamStateChanged(id_, false);
Same as above

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:128: FROM_HERE,
base::Bind(&AudioOutputImpl::Close, base::Unretained(this)));
What is the guarantee that "this" will survive until Close() execution? (looks
like there is none)

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:144:
DCHECK(task_runner_->BelongsToCurrentThread());
Do you really need it to be constructed on this thread? If so, you don't need
task_runner as a parameter, you can just use the current one.

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:149: if (is_closing)
is it actually "closing" or "closed"?

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:184: return;
Is it a normal situation? If not - DCHECK? (we can't fail silently)

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:212:
output->task_runner_->DeleteSoon(FROM_HERE, output);
This deletion is a lot of fun, but can we probably simplify it?

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_impl.h (right):

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:97:
base::SingleThreadTaskRunner* const task_runner_;
I guess we need scoped_refptr here, because we don't want it to be deleted while
we are using it.

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:101: Host* const host_;
const

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:103: const int32_t id_;
Why do we need id? I think id is a host-side property; a host can bind id to the
interface if it needs one.

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:110:
scoped_refptr<media::AudioOutputController> controller_;
Should it be scoped? If yes, we need a comment on why

[Max Morin, 2016-09-28 12:47:33]

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_impl.cc (right):

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:91: Close();
On 2016/09/28 10:24:06, o1ka wrote:
> Here and in other places: what is the purpose of Close() here? Is it error
> processing? Would DCHECKs be enough? Anyways, some logging and comments needed
> to explain what has happened

Ok, in this case the client might call Play() before Start(), which is an error
of the client, not something we should DCHECK on.

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:108: if (!IsStarted()
|| volume < 0.0 || volume > 1.0)
On 2016/09/28 10:24:07, o1ka wrote:
> Where these volume limits come from? And what if they change there?

Good question. Needs better comments.

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:128: FROM_HERE,
base::Bind(&AudioOutputImpl::Close, base::Unretained(this)));
On 2016/09/28 10:24:06, o1ka wrote:
> What is the guarantee that "this" will survive until Close() execution? (looks
> like there is none)

Ok, so we should maybe have weak pointers to *this. I'll think more.

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:149: if (is_closing)
On 2016/09/28 10:24:06, o1ka wrote:
> is it actually "closing" or "closed"?

I'll rework this bit.

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:184: return;
On 2016/09/28 10:24:06, o1ka wrote:
> Is it a normal situation? If not - DCHECK? (we can't fail silently)

Should probably be a DCHECK, it happens if we can't allocate shared memory (out
of memory?) or if we can't make a socket pair (probably too many file
descriptors?).

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_impl.h (right):

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:103: const int32_t id_;
On 2016/09/28 10:24:07, o1ka wrote:
> Why do we need id? I think id is a host-side property; a host can bind id to
the
> interface if it needs one.

Agree.

[o1ka, 2016-09-28 15:17:20]

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_impl.h (right):

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:112: StartCallback
start_callback_;
BTW it's "render" callback - the one which will be periodically called to get
audio.

[Max Morin, 2016-09-28 16:13:18]

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_impl.h (right):

https://codereview.chromium.org/2319493002/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:112: StartCallback
start_callback_;
On 2016/09/28 15:17:19, o1ka wrote:
> BTW it's "render" callback - the one which will be periodically called to get
> audio.

This is the callback for the response to the mojo Start call. We store it since
we have to wait for AudioOutputController to finish creating before we respond
to Start.

[Max Morin, 2016-12-14 09:57:18]

Description was changed from

==========
Add mojo interface for audio rendering. Class diagram:
https://docs.google.com/a/chromium.org/drawings/d/1C4nc2ZCDNr3jgxZJFYA-1hbW7t....
Current state: AudioOutputImpl and mojom ready for review. No point in looking
at AudioOutputServiceImpl right now.

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

to

==========
Add mojo interface for audio rendering. Current state: Prototype CL, no need to
review details.

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

[Max Morin, 2017-01-09 13:03:10]

Description was changed from

==========
Add mojo interface for audio rendering. Current state: Prototype CL, no need to
review details.

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

to

==========
Add mojo interface for audio rendering. Current state: Prototype CL, no need to
review details. Must be split before real review.

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

[Max Morin, 2017-01-20 16:16:00]

Description was changed from

==========
Add mojo interface for audio rendering. Current state: Prototype CL, no need to
review details. Must be split before real review.

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

to

==========
Add mojo interface for audio rendering. Current state: No need to review
details. Must be split before real review.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...

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

[Max Morin, 2017-01-20 16:29:39]

I updated this CL with some code and a diagram (in CL description). Olka: Maybe
we can have a high-level discussion of interfaces and such on Monday/Tuesday?

State of code right now: Missing a a bit of code. Code to plug it into ARH is
not written. Missing comments/tests. Open questions: Should mojom files be in
content instead? Are class names ok? I'm thinking of starting to carve parts of
this off for testing and review next week.

[Max Morin, 2017-01-20 16:55:53]

+CC ossu FYI in case you want some bedtime reading :).

[Max Morin, 2017-01-20 17:20:00]

Description was changed from

==========
Add mojo interface for audio rendering. Current state: No need to review
details. Must be split before real review.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...

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

to

==========
Add mojo interface for audio rendering. Current state: No need to review
details. Must be split before real review.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...

Preemptive code review answers:
 * Context seems much more common than factory for factories of mojo interfaces
from what I have seen. The AudioOutputServiceContext object also holds some
things which the AudioOutputServiceImpl needs, making "context" appropriate.
 * The "callback with pointer for when this object wants to be deleted" pattern
also seems like a common solution for mojo interface implementations.

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

[o1ka, 2017-01-25 15:15:21]

Just looked through it without going into details - some comments.

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
File content/browser/renderer_host/media/audio_output_impl.cc (right):

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.cc:78: if (volume < 0 ||
volume > 1)
Where the limitation comes from?  a comment is needed

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
File content/browser/renderer_host/media/audio_output_impl.h (right):

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:21: public
AudioOutputDelegate::EventHandler {
Can we use aggregation instead of inheritance (wherever possible)?

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:23: using
DelegateFactoryCallback =
CreateAudioOutputDelegateCallback maybe? Oe CreateDelegateCallback?
DelegateFactoryCallback sounds confusing to me, as if it's a callback to pass to
a delegate factory, and it's not clear what is produces.

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:28: using
FinishedCallback = base::OnceCallback<void(AudioOutputImpl*)>;
What this name stands for?

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_output_impl.h:37: // May only be
called via mojo, not directly.
What does it mean?

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
File content/browser/renderer_host/media/audio_output_service_context.h (right):

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_output_service_context.h:65:
media::AudioManager* const audio_manager_;
It would be great to avoid duplications between AudioRendererHost and Mojo
version of the code.

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_output_service_context.h:72:
std::vector<std::unique_ptr<AudioOutputServiceImpl>> services_;
Can't you manage it with StrongBindingSet instead?
(https://cs.chromium.org/chromium/src/media/mojo/services/strong_binding_set.h...)
It will do all the lifetime management for us.

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
File content/browser/renderer_host/media/audio_output_service_impl.h (right):

https://codereview.chromium.org/2319493002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_output_service_impl.h:21: class
CONTENT_EXPORT AudioOutputServiceImpl
RenderFrameAudioOutputService?
Let's make names as much self-descriptive as possible.

https://codereview.chromium.org/2319493002/diff/160001/media/base/media_switc...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2319493002/diff/160001/media/base/media_switc...
media/base/media_switches.cc:181: "mojo-audio-output-stream-control",
base::FEATURE_DISABLED_BY_DEFAULT};
Can we land the unhooked code with unit tests first, and feature + factory
separately (the latter requires a launch procedure)?

https://codereview.chromium.org/2319493002/diff/160001/media/mojo/interfaces/...
File media/mojo/interfaces/audio_output.mojom (right):

https://codereview.chromium.org/2319493002/diff/160001/media/mojo/interfaces/...
media/mojo/interfaces/audio_output.mojom:20: };
We want these two interfaces it live in separate mojoms, since they will be
hosted in separate processes.

https://codereview.chromium.org/2319493002/diff/160001/media/mojo/interfaces/...
media/mojo/interfaces/audio_output.mojom:22: interface AudioOutputService {
RendererAudioOutputService? Or some other more descriptive name?

[Max Morin, 2017-02-06 16:41:23]

Description was changed from

==========
Add mojo interface for audio rendering. Current state: No need to review
details. Must be split before real review.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...

Preemptive code review answers:
 * Context seems much more common than factory for factories of mojo interfaces
from what I have seen. The AudioOutputServiceContext object also holds some
things which the AudioOutputServiceImpl needs, making "context" appropriate.
 * The "callback with pointer for when this object wants to be deleted" pattern
also seems like a common solution for mojo interface implementations.

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

to

==========
Add mojo interface for audio rendering. Current state: No need to review
details. Must be split before real review.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...

Preemptive code review answers:
 * Context seems much more common than factory for factories of mojo interfaces
from what I have seen. The AudioOutputServiceContext object also holds some
things which the AudioOutputServiceImpl needs, making "context" appropriate.
 * The "callback with pointer for when this object wants to be deleted" pattern
also seems like a common solution for mojo interface implementations.

BUG=425368
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
==========

[Max Morin, 2017-02-08 12:00:09]

Patchset #13 (id:240001) has been deleted

[Max Morin, 2017-02-08 12:00:14]

Patchset #12 (id:220001) has been deleted

[Max Morin, 2017-02-08 12:49:40]

Patchset #12 (id:260001) has been deleted

[Max Morin, 2017-02-09 17:00:56]

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

[commit-bot: I haz the power, 2017-02-09 17:02:21]

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-02-09 17:12:07]

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

[commit-bot: I haz the power, 2017-02-09 17:12:08]

Dry run: 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...)

[Max Morin, 2017-02-09 17:43:29]

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

[commit-bot: I haz the power, 2017-02-09 17:44:16]

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-02-09 18:33:48]

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

[commit-bot: I haz the power, 2017-02-09 18:33:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Max Morin, 2017-02-10 09:02:53]

Patchset #15 (id:340001) has been deleted

[Max Morin, 2017-02-10 09:02:59]

Patchset #14 (id:320001) has been deleted

[Max Morin, 2017-02-10 09:03:04]

Patchset #13 (id:300001) has been deleted

[Max Morin, 2017-02-10 09:03:11]

Patchset #12 (id:280001) has been deleted

[Max Morin, 2017-02-10 09:03:16]

Patchset #11 (id:200001) has been deleted

[Max Morin, 2017-02-10 09:03:22]

Patchset #10 (id:180001) has been deleted

[Max Morin, 2017-02-10 09:35:27]

Patchset #8 (id:140001) has been deleted

[Max Morin, 2017-02-10 09:35:38]

Patchset #7 (id:120001) has been deleted

[Max Morin, 2017-02-10 09:35:47]

Patchset #5 (id:80001) has been deleted

[Max Morin, 2017-02-10 09:35:52]

Patchset #4 (id:60001) has been deleted

[Max Morin, 2017-02-10 09:35:55]

Patchset #3 (id:40001) has been deleted

[Max Morin, 2017-02-10 09:35:59]

Patchset #2 (id:20001) has been deleted

[Max Morin, 2017-02-10 10:05:22]

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

[commit-bot: I haz the power, 2017-02-10 10:05:41]

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-02-10 11:20:36]

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

[commit-bot: I haz the power, 2017-02-10 11:20: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-02-10 13:03:58]

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

[commit-bot: I haz the power, 2017-02-10 13:04:13]

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-02-10 14:35:03]

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

[commit-bot: I haz the power, 2017-02-10 14:35:04]

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

[Max Morin, 2017-02-13 14:09:19]

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

[commit-bot: I haz the power, 2017-02-13 14:09: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-02-13 16:09:10]

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

[commit-bot: I haz the power, 2017-02-13 16:09:12]

Dry run: This issue passed the CQ dry run.

[Max Morin, 2017-02-13 16:41:47]

Patchset #6 (id:400001) has been deleted

[Max Morin, 2017-02-13 16:42:28]

Patchset #5 (id:380001) has been deleted

[Max Morin, 2017-02-13 16:42:39]

Patchset #4 (id:360001) has been deleted

[Max Morin, 2017-02-13 16:49:08]

Description was changed from

==========
Add mojo interface for audio rendering. Current state: No need to review
details. Must be split before real review.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...

Preemptive code review answers:
 * Context seems much more common than factory for factories of mojo interfaces
from what I have seen. The AudioOutputServiceContext object also holds some
things which the AudioOutputServiceImpl needs, making "context" appropriate.
 * The "callback with pointer for when this object wants to be deleted" pattern
also seems like a common solution for mojo interface implementations.

BUG=425368
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

==========
Add mojo interface for audio rendering. Current state: No need to review
details. Must be split before real review.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...

Preemptive code review answers:
 * Context seems much more common than factory for factories of mojo interfaces
from what I have seen. The AudioOutputServiceContext object also holds some
things which the AudioOutputServiceImpl needs, making "context" appropriate.
 * The "callback with pointer for when this object wants to be deleted" pattern
also seems like a common solution for mojo interface implementations.

BUG=425368
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
==========

[Max Morin, 2017-02-13 16:49:34]

Description was changed from

==========
Add mojo interface for audio rendering. Current state: No need to review
details. Must be split before real review.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...

Preemptive code review answers:
 * Context seems much more common than factory for factories of mojo interfaces
from what I have seen. The AudioOutputServiceContext object also holds some
things which the AudioOutputServiceImpl needs, making "context" appropriate.
 * The "callback with pointer for when this object wants to be deleted" pattern
also seems like a common solution for mojo interface implementations.

BUG=425368
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

==========
Add mojo interface for audio rendering.

Summary:
 * The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
 * content/common/media/audio_output.mojom defines the IPC interface.
 * AudioOutputImpl is a class that receives IPC messages for a single stream on
the browser side.
 * RenderFrameAudioOutputService manages all the stream creation messages for a
single render frame.
 * AudioOutputServiceContext provides utility functions for the
RenderFrameAudioOutputServices of a single render process.
 * AudioOutputImpl and RenderFrameAudioOutputService are unit tested, and there
is an integration test which sets up an audio stream over IPC and verifies that
the data arrives by using a mock stream.

Renderer-side client of these interfaces, as well as hooking them up in the
browser, will be done in Separate CLs.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...
(renderer-side not added to diagram yet).

Preemptive code review answers:
 * Using a StrongBindingSet for either the AudioOutputs or the
RendererAudioOutputServices is not possible, since these classes may wish to be
deleted when encountering an error, so they need some sort of way to control
their lifetime. The "callback with pointer for when this object wants to be
deleted" pattern seems like a common solution for mojo interface
implementations.
 * AFAIK sync sockets and shared memory is not welcome in media/, so all this
code lives in content/. Feel free to tell me if I got this wrong, as the
directory hierarchy is unclear to me :).

BUG=425368
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
==========

[Max Morin, 2017-02-13 17:33:12]

Description was changed from

==========
Add mojo interface for audio rendering.

Summary:
 * The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
 * content/common/media/audio_output.mojom defines the IPC interface.
 * AudioOutputImpl is a class that receives IPC messages for a single stream on
the browser side.
 * RenderFrameAudioOutputService manages all the stream creation messages for a
single render frame.
 * AudioOutputServiceContext provides utility functions for the
RenderFrameAudioOutputServices of a single render process.
 * AudioOutputImpl and RenderFrameAudioOutputService are unit tested, and there
is an integration test which sets up an audio stream over IPC and verifies that
the data arrives by using a mock stream.

Renderer-side client of these interfaces, as well as hooking them up in the
browser, will be done in Separate CLs.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...
(renderer-side not added to diagram yet).

Preemptive code review answers:
 * Using a StrongBindingSet for either the AudioOutputs or the
RendererAudioOutputServices is not possible, since these classes may wish to be
deleted when encountering an error, so they need some sort of way to control
their lifetime. The "callback with pointer for when this object wants to be
deleted" pattern seems like a common solution for mojo interface
implementations.
 * AFAIK sync sockets and shared memory is not welcome in media/, so all this
code lives in content/. Feel free to tell me if I got this wrong, as the
directory hierarchy is unclear to me :).

BUG=425368
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

==========
Add mojo interface for audio rendering.

Summary:
 * The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
 * content/common/media/audio_output.mojom defines the IPC interface.
 * AudioOutputImpl is a class that receives IPC messages for a single stream on
the browser side.
 * RenderFrameAudioOutputService manages all the stream creation messages for a
single render frame.
 * AudioOutputServiceContext provides utility functions for the
RenderFrameAudioOutputServices of a single render process.
 * AudioOutputImpl and RenderFrameAudioOutputService are unit tested, and there
is an integration test which sets up an audio stream over IPC and verifies that
the data arrives by using a mock stream in the Audio Manager.

Renderer-side client of these interfaces, as well as hooking them up in the
browser, will be done in Separate CLs.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...
(renderer-side not added to diagram yet).

Preemptive code review answers:
 * Using a StrongBindingSet for either the AudioOutputs or the
RendererAudioOutputServices is not possible, since these classes may wish to be
deleted when encountering an error, so they need some sort of way to control
their lifetime. The "callback with pointer for when this object wants to be
deleted" pattern seems like a common solution for mojo interface
implementations.
 * AFAIK sync sockets and shared memory is not welcome in media/, so all this
code lives in content/. Feel free to tell me if I got this wrong, as the
directory hierarchy is unclear to me :).

BUG=425368
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
==========

[Max Morin, 2017-02-13 17:33:38]

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

[Max Morin, 2017-02-13 17:38:48]

Description was changed from

==========
Add mojo interface for audio rendering.

Summary:
 * The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
 * content/common/media/audio_output.mojom defines the IPC interface.
 * AudioOutputImpl is a class that receives IPC messages for a single stream on
the browser side.
 * RenderFrameAudioOutputService manages all the stream creation messages for a
single render frame.
 * AudioOutputServiceContext provides utility functions for the
RenderFrameAudioOutputServices of a single render process.
 * AudioOutputImpl and RenderFrameAudioOutputService are unit tested, and there
is an integration test which sets up an audio stream over IPC and verifies that
the data arrives by using a mock stream in the Audio Manager.

Renderer-side client of these interfaces, as well as hooking them up in the
browser, will be done in Separate CLs.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...
(renderer-side not added to diagram yet).

Preemptive code review answers:
 * Using a StrongBindingSet for either the AudioOutputs or the
RendererAudioOutputServices is not possible, since these classes may wish to be
deleted when encountering an error, so they need some sort of way to control
their lifetime. The "callback with pointer for when this object wants to be
deleted" pattern seems like a common solution for mojo interface
implementations.
 * AFAIK sync sockets and shared memory is not welcome in media/, so all this
code lives in content/. Feel free to tell me if I got this wrong, as the
directory hierarchy is unclear to me :).

BUG=425368
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

==========
Add mojo interface for audio rendering.

Summary:
 * The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
 * content/common/media/audio_output.mojom defines the IPC interface.
 * AudioOutputImpl is a class that receives IPC messages for a single stream on
the browser side.
 * RenderFrameAudioOutputService manages all the stream creation messages for a
single render frame.
 * AudioOutputServiceContext provides utility functions for the
RenderFrameAudioOutputServices of a single render process.
 * AudioOutputImpl and RenderFrameAudioOutputService are unit tested, and there
is an integration test which sets up an audio stream over IPC and verifies that
the data arrives by using a mock stream in the Audio Manager.

Renderer-side client of these interfaces, as well as hooking them up in the
browser, will be done in separate CLs.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...
(renderer-side not added to diagram yet).

Preemptive code review answers:
 * Using a StrongBindingSet for either the AudioOutputs or the
RendererAudioOutputServices is not possible, since these classes may wish to be
deleted when encountering an error, so they need some sort of way to control
their lifetime. The "callback with pointer for when this object wants to be
deleted" pattern seems like a common solution for mojo interface
implementations.
 * AFAIK sync sockets and shared memory is not welcome in media/, so all this
code lives in content/. Feel free to tell me if I got this wrong, as the
directory hierarchy is unclear to me :).

BUG=425368
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
==========

[Max Morin, 2017-02-13 17:42:56]

Hello Dale. I think this code is ready for review now, I just have a couple of
questions before that. Do you think the namespacing makes sense (everything in
content, essentially)? Would you prefer smaller reviews (e.g. interface is one
CL, AudioOutputImpl is one, and
RenderFrameAudioOutputService+AudioOutputServiceContext is one)? Finally, do you
have any other notes/comments before proper review?

[DaleCurtis, 2017-02-13 19:16:01]

Haven't looked too much, but:
- mojo security reviewers want the mojom and implementations.
- lets review one mojom at a time though, plus a followup CL to connect them up
if needed.
- as much code as possible should live in media/ and not content/

[Max Morin, 2017-02-14 10:48:18]

Description was changed from

==========
Add mojo interface for audio rendering.

Summary:
 * The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
 * content/common/media/audio_output.mojom defines the IPC interface.
 * AudioOutputImpl is a class that receives IPC messages for a single stream on
the browser side.
 * RenderFrameAudioOutputService manages all the stream creation messages for a
single render frame.
 * AudioOutputServiceContext provides utility functions for the
RenderFrameAudioOutputServices of a single render process.
 * AudioOutputImpl and RenderFrameAudioOutputService are unit tested, and there
is an integration test which sets up an audio stream over IPC and verifies that
the data arrives by using a mock stream in the Audio Manager.

Renderer-side client of these interfaces, as well as hooking them up in the
browser, will be done in separate CLs.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...
(renderer-side not added to diagram yet).

Preemptive code review answers:
 * Using a StrongBindingSet for either the AudioOutputs or the
RendererAudioOutputServices is not possible, since these classes may wish to be
deleted when encountering an error, so they need some sort of way to control
their lifetime. The "callback with pointer for when this object wants to be
deleted" pattern seems like a common solution for mojo interface
implementations.
 * AFAIK sync sockets and shared memory is not welcome in media/, so all this
code lives in content/. Feel free to tell me if I got this wrong, as the
directory hierarchy is unclear to me :).

BUG=425368
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

==========
Add mojo interface for audio rendering.

Summary:
 * The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
 * content/common/media/audio_output.mojom defines the IPC interface.
 * AudioOutputImpl is a class that receives IPC messages for a single stream on
the browser side.
 * RenderFrameAudioOutputService manages all the stream creation messages for a
single render frame.
 * AudioOutputServiceContext provides utility functions for the
RenderFrameAudioOutputServices of a single render process.
 * AudioOutputImpl and RenderFrameAudioOutputService are unit tested, and there
is an integration test which sets up an audio stream over IPC and verifies that
the data arrives by using a mock stream in the Audio Manager.

Renderer-side client of these interfaces, as well as hooking them up in the
browser, will be done in separate CLs.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...
(renderer-side not added to diagram yet).

Preemptive code review answers:
 * Using a StrongBindingSet for either the AudioOutputs or the
RendererAudioOutputServices is not possible, since these classes may wish to be
deleted when encountering an error, so they need some sort of way to control
their lifetime. The "callback with pointer for when this object wants to be
deleted" pattern seems like a common solution for mojo interface
implementations.
 * AFAIK sync sockets and shared memory is not welcome in media/, so all this
code lives in content/. Feel free to tell me if I got this wrong, as the
directory hierarchy is unclear to me :).

Split into https://codereview.chromium.org/2697793002/ +
https://codereview.chromium.org/2697663003/

BUG=425368
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
==========

[Max Morin, 2017-02-17 13:20:51]

Description was changed from

==========
Add mojo interface for audio rendering.

Summary:
 * The changes in media/mojo/interfaces/, media/base/ipc, and
content/common/media/audio_messages.h mojofy OutputDeviceStatus.
 * content/common/media/audio_output.mojom defines the IPC interface.
 * AudioOutputImpl is a class that receives IPC messages for a single stream on
the browser side.
 * RenderFrameAudioOutputService manages all the stream creation messages for a
single render frame.
 * AudioOutputServiceContext provides utility functions for the
RenderFrameAudioOutputServices of a single render process.
 * AudioOutputImpl and RenderFrameAudioOutputService are unit tested, and there
is an integration test which sets up an audio stream over IPC and verifies that
the data arrives by using a mock stream in the Audio Manager.

Renderer-side client of these interfaces, as well as hooking them up in the
browser, will be done in separate CLs.

Picture of how it will be hooked up in RenderFrameHost:
https://docs.google.com/drawings/d/1KsGJnLhNY54EaNcOKVhtzKEpoPrpqy2BanA62vGlu...
(renderer-side not added to diagram yet).

Preemptive code review answers:
 * Using a StrongBindingSet for either the AudioOutputs or the
RendererAudioOutputServices is not possible, since these classes may wish to be
deleted when encountering an error, so they need some sort of way to control
their lifetime. The "callback with pointer for when this object wants to be
deleted" pattern seems like a common solution for mojo interface
implementations.
 * AFAIK sync sockets and shared memory is not welcome in media/, so all this
code lives in content/. Feel free to tell me if I got this wrong, as the
directory hierarchy is unclear to me :).

Split into https://codereview.chromium.org/2697793002/ +
https://codereview.chromium.org/2697663003/

BUG=425368
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

==========
Split into https://codereview.chromium.org/2697793002/ +
https://codereview.chromium.org/2697663003/

BUG=425368
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
==========