Set audio format before adding pepper sink to audio track.

Set audio format before adding pepper sink to audio track.

Some audio input devices don't generate caputure data in a continuous basis, and
instead only generate when some event happens. This may be an indefinite amount
of time. However, |AudioSink::OnSetFormat| is only called whenever new samples
come in. For pepper plugins, this means that |MediaStreamAudioTrack::Configure|
can be blocked indefintely on these devices. To work around this, get the
format before attaching to the audio track.

BUG=403138
Committed: https://crrev.com/ec4c42f4ef67badad8b5d13190d4630aa31a9188
Cr-Commit-Position: refs/heads/master@{#313458}

[Anand Mistry (off Chromium), 2015-01-19 23:16:02]

amistry@chromium.org changed reviewers:
+ dalecurtis@chromium.org, dmichael@chromium.org

[Anand Mistry (off Chromium), 2015-01-19 23:16:02]

dmichael@chromium.org: Please review changes in pepper

dalecurtis@chromium.org: Please review changes in everything else

[dmichael (off chromium), 2015-01-20 17:07:03]

https://codereview.chromium.org/857093002/diff/20001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.cc:49: // return a reference.
I think it would be a lot better to just return by-value.

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:45: static const
media::AudioParameters empty_params;
ditto...  it's almost always better to just return by-value. Is this object
really big or something? If there's an actual performance issue with returning
by value, a better alternative is an out-param (but even that is usually not
worthwhile for an object that just carries some fields).

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.h:72: // Called on the main
render thread.
It looks like you could check that with main_render_thread_checker_.

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:370: // can't
happen until the sink is added to the audio track below.
This seems kind of dangerous. Is there any way for you to get a TaskRunner for
the audio capture thread to post it there?

At the very least, there ought to be an obvious comment on
AudioSink::OnSetFormat to indicate that it must be callable from the main thread
OR the audio capture thread. It looks like it's probably safe for now, but who
knows what a future coder might do...

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:57: friend class
PepperMediaStreamAudioTrackHost;
I think I'd rather you either:
1) Make OnSetFormat public (OnData can be as well)
or
2) At the place you need to call OnSetFormat,
static_cast<MediaStreamAudioSink*>(host_)

Either option would better preserve encapsulation of AudioSink.

[DaleCurtis, 2015-01-20 18:46:56]

https://codereview.chromium.org/857093002/diff/20001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.cc:50: static const
media::AudioParameters null_format;
This seems unnecessary, just return media::AudioParameters() below. Is there a
reason you are static const'ing for speed? :)

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:45: static const
media::AudioParameters empty_params;
On 2015/01/20 17:07:03, dmichael wrote:
> ditto...  it's almost always better to just return by-value. Is this object
> really big or something? If there's an actual performance issue with returning
> by value, a better alternative is an out-param (but even that is usually not
> worthwhile for an object that just carries some fields).

+1, just return media::AudioParameters().

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:370: // can't
happen until the sink is added to the audio track below.
On 2015/01/20 17:07:03, dmichael wrote:
> This seems kind of dangerous. Is there any way for you to get a TaskRunner for
> the audio capture thread to post it there?
> 
> At the very least, there ought to be an obvious comment on
> AudioSink::OnSetFormat to indicate that it must be callable from the main
thread
> OR the audio capture thread. It looks like it's probably safe for now, but who
> knows what a future coder might do...

Yeah the API contract should document this. I'm not sure how the pepper sink
works, so there may also be a task runner you can use. There isn't one for
normal input capture though.

[Anand Mistry (off Chromium), 2015-01-27 02:12:10]

https://codereview.chromium.org/857093002/diff/20001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.cc:49: // return a reference.
On 2015/01/20 17:07:03, dmichael wrote:
> I think it would be a lot better to just return by-value.

Done.

https://codereview.chromium.org/857093002/diff/20001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.cc:50: static const
media::AudioParameters null_format;
On 2015/01/20 18:46:56, DaleCurtis wrote:
> This seems unnecessary, just return media::AudioParameters() below. Is there a
> reason you are static const'ing for speed? :)

Because it needs to survive after the function call sicne it's returned by
reference. Removed since I'm now returning by value.

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:45: static const
media::AudioParameters empty_params;
On 2015/01/20 18:46:56, DaleCurtis wrote:
> On 2015/01/20 17:07:03, dmichael wrote:
> > ditto...  it's almost always better to just return by-value. Is this object
> > really big or something? If there's an actual performance issue with
returning
> > by value, a better alternative is an out-param (but even that is usually not
> > worthwhile for an object that just carries some fields).
> 
> +1, just return media::AudioParameters().

Done. I just figured returning const reference was good practice to avoid
unnecessary copies. Ya know, death by a thousand cuts, and all that.

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.h:72: // Called on the main
render thread.
On 2015/01/20 17:07:03, dmichael wrote:
> It looks like you could check that with main_render_thread_checker_.

Done.

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:370: // can't
happen until the sink is added to the audio track below.
On 2015/01/20 18:46:56, DaleCurtis wrote:
> On 2015/01/20 17:07:03, dmichael wrote:
> > This seems kind of dangerous. Is there any way for you to get a TaskRunner
for
> > the audio capture thread to post it there?

Nope. The audio capture thread is a real-time thread that blocks on a socket. No
TaskRunner available. That's this issue this CL works around.

> > 
> > At the very least, there ought to be an obvious comment on
> > AudioSink::OnSetFormat to indicate that it must be callable from the main
> thread
> > OR the audio capture thread. It looks like it's probably safe for now, but
who
> > knows what a future coder might do...
> 
> Yeah the API contract should document this. I'm not sure how the pepper sink
> works, so there may also be a task runner you can use. There isn't one for
> normal input capture though.

I've extended the comment in the header file to try and make this clear.

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:57: friend class
PepperMediaStreamAudioTrackHost;
On 2015/01/20 17:07:03, dmichael wrote:
> I think I'd rather you either:
> 1) Make OnSetFormat public (OnData can be as well)
> or
> 2) At the place you need to call OnSetFormat,
> static_cast<MediaStreamAudioSink*>(host_)
> 
> Either option would better preserve encapsulation of AudioSink.

Done #1.

[dmichael (off chromium), 2015-01-27 18:24:10]

just nits left. And some commentary on why to prefer return-by-value in this
case.

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/857093002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:45: static const
media::AudioParameters empty_params;
On 2015/01/27 02:12:10, Anand Mistry wrote:
> On 2015/01/20 18:46:56, DaleCurtis wrote:
> > On 2015/01/20 17:07:03, dmichael wrote:
> > > ditto...  it's almost always better to just return by-value. Is this
object
> > > really big or something? If there's an actual performance issue with
> returning
> > > by value, a better alternative is an out-param (but even that is usually
not
> > > worthwhile for an object that just carries some fields).
> > 
> > +1, just return media::AudioParameters().
> 
> Done. I just figured returning const reference was good practice to avoid
> unnecessary copies. Ya know, death by a thousand cuts, and all that.
Copying a small-ish object that doesn't do any heap allocations is pretty cheap.
In fact, the compiler can sometimes avoid the copy altogether. Function-local
statics, meanwhile, require the compiler to generate code that ensures they are
initialized on the first invocation of the function, so that might actually be
more expensive. Also, in Chromium, these function-local statics are not
thread-safe (they're not guaranteed thread safe by the language...  compilers
may make them thread-safe, but we turn that off). And...  this kind of
optimization is so small it's unlikely it would ever show up on any benchmark or
profiler (even if the static way is faster, which it may not be). We should aim
for code clarity in this kind of situation.

https://codereview.chromium.org/857093002/diff/60001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/857093002/diff/60001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:42: static const
media::AudioParameters GetFormatFromAudioTrack(
the return type shouldn't be  const. No need to dictate what the call-site can
do.

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.h:73: const
media::AudioParameters GetOutputFormat() const;
ditto, drop the const before the return type.

[DaleCurtis, 2015-01-27 19:50:09]

lgtm % our comments.

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:110: const
media::AudioParameters& GetOutputFormat() const;
Remove const&

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:45: if
(webaudio_source_.get()) {
Is .get() necessary? I prefer ternary for this, but up to you :)

[Anand Mistry (off Chromium), 2015-01-27 23:14:51]

https://codereview.chromium.org/857093002/diff/60001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.h (right):

https://codereview.chromium.org/857093002/diff/60001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.h:42: static const
media::AudioParameters GetFormatFromAudioTrack(
On 2015/01/27 18:24:10, dmichael wrote:
> the return type shouldn't be  const. No need to dictate what the call-site can
> do.

Done.

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:110: const
media::AudioParameters& GetOutputFormat() const;
On 2015/01/27 19:50:09, DaleCurtis wrote:
> Remove const&

Done.

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:45: if
(webaudio_source_.get()) {
On 2015/01/27 19:50:09, DaleCurtis wrote:
> Is .get() necessary? I prefer ternary for this, but up to you :)

.get() appears to be the convention in this file, so I've stuck with it.

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/857093002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.h:73: const
media::AudioParameters GetOutputFormat() const;
On 2015/01/27 18:24:10, dmichael wrote:
> ditto, drop the const before the return type.

Done.

[dmichael (off chromium), 2015-01-27 23:16:13]

lgtm

[Anand Mistry (off Chromium), 2015-01-27 23:17:29]

amistry@chromium.org changed reviewers:
+ avi@chromium.org

[Anand Mistry (off Chromium), 2015-01-27 23:17:30]

avi@chromium.org: For content/public owners

[Avi (use Gerrit), 2015-01-27 23:25:17]

https://codereview.chromium.org/857093002/diff/80001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/857093002/diff/80001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.cc:50: LOG(ERROR) << "Can't get
format from a remote audio track";
Why LOG rather than DLOG? Especially since this would be a programming error.

(This comment applies to all the LOG statements in this file.)

[Anand Mistry (off Chromium), 2015-01-28 04:24:32]

https://codereview.chromium.org/857093002/diff/80001/content/public/renderer/...
File content/public/renderer/media_stream_audio_sink.cc (right):

https://codereview.chromium.org/857093002/diff/80001/content/public/renderer/...
content/public/renderer/media_stream_audio_sink.cc:50: LOG(ERROR) << "Can't get
format from a remote audio track";
On 2015/01/27 23:25:17, Avi wrote:
> Why LOG rather than DLOG? Especially since this would be a programming error.
> 
> (This comment applies to all the LOG statements in this file.)

I don't know anything about webrtc, but presumably it's possible for a webrtc
user to hit this case from Javascript and hence having this in a release build
is useful for a web developer. Then again, maybe not.

[Avi (use Gerrit), 2015-01-28 04:42:52]

On 2015/01/28 04:24:32, Anand Mistry wrote:
>
https://codereview.chromium.org/857093002/diff/80001/content/public/renderer/...
> File content/public/renderer/media_stream_audio_sink.cc (right):
> 
>
https://codereview.chromium.org/857093002/diff/80001/content/public/renderer/...
> content/public/renderer/media_stream_audio_sink.cc:50: LOG(ERROR) << "Can't
get
> format from a remote audio track";
> On 2015/01/27 23:25:17, Avi wrote:
> > Why LOG rather than DLOG? Especially since this would be a programming
error.
> > 
> > (This comment applies to all the LOG statements in this file.)
> 
> I don't know anything about webrtc, but presumably it's possible for a webrtc
> user to hit this case from Javascript and hence having this in a release build
> is useful for a web developer. Then again, maybe not.

:/ But then they have to know to look...

LGTM for now, but it would be nice to handle this better.

[Anand Mistry (off Chromium), 2015-01-28 05:20:49]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2015-01-28 05:21:16]

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

[commit-bot: I haz the power, 2015-01-28 05:39:51]

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

[commit-bot: I haz the power, 2015-01-28 05:39:52]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Anand Mistry (off Chromium), 2015-01-28 06:05:59]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2015-01-28 06:06:42]

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

[commit-bot: I haz the power, 2015-01-28 07:31:50]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-28 07:32:56]

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