Return from |Configure()| asynchronously, and allow |InitBuffers()| to return errors.

Return from |Configure()| asynchronously, and allow |InitBuffers()| to return errors.

BUG=330851
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289856

[dmichael (off chromium), 2014-07-31 18:13:34]

teravest: can you cover this from the Pepper OWNERS side?

[teravest, 2014-07-31 20:49:57]

On 2014/07/31 18:13:34, dmichael (OOO 7.31-8.1) wrote:
> teravest: can you cover this from the Pepper OWNERS side?

Yep

[teravest, 2014-07-31 20:54:55]

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:112: //
Initialize later in OnSetFormat if bytes_per_second_ is not know yet.
s/know/known/

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
(pending_configure_reply_.is_valid()) {
I think you could DCHECK(pending_configure_reply_.is_valid()) here as well.

https://codereview.chromium.org/430943004/diff/1/ppapi/tests/test_media_strea...
File ppapi/tests/test_media_stream_audio_track.cc (right):

https://codereview.chromium.org/430943004/diff/1/ppapi/tests/test_media_strea...
ppapi/tests/test_media_stream_audio_track.cc:207: // Do nothing.
What's this comment for?

[Anand Mistry (off Chromium), 2014-07-31 23:54:48]

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:112: //
Initialize later in OnSetFormat if bytes_per_second_ is not know yet.
On 2014/07/31 20:54:55, teravest wrote:
> s/know/known/

Done.

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
(pending_configure_reply_.is_valid()) {
On 2014/07/31 20:54:55, teravest wrote:
> I think you could DCHECK(pending_configure_reply_.is_valid()) here as well.

I don't think so. This function will be called by InitBuffers(), which may be
called by SetFormatOnMainThread(), which is unrelated to the Configure() IPC. On
startup, this will happen, and there may not be a pending Configure() IPC call
in progress.

https://codereview.chromium.org/430943004/diff/1/ppapi/tests/test_media_strea...
File ppapi/tests/test_media_stream_audio_track.cc (right):

https://codereview.chromium.org/430943004/diff/1/ppapi/tests/test_media_strea...
ppapi/tests/test_media_stream_audio_track.cc:207: // Do nothing.
On 2014/07/31 20:54:55, teravest wrote:
> What's this comment for?

Expanded.

[teravest, 2014-08-01 00:15:44]

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
(pending_configure_reply_.is_valid()) {
On 2014/07/31 23:54:48, Anand Mistry wrote:
> On 2014/07/31 20:54:55, teravest wrote:
> > I think you could DCHECK(pending_configure_reply_.is_valid()) here as well.
> 
> I don't think so. This function will be called by InitBuffers(), which may be
> called by SetFormatOnMainThread(), which is unrelated to the Configure() IPC.
On
> startup, this will happen, and there may not be a pending Configure() IPC call
> in progress.

Hmm, okay.
How is it ensured that a reply will always be sent for a Configure message?

Suppose at line 111, changed is true, but bytes_per_second_ is set. What happens
to pending_configure_reply_?

[Anand Mistry (off Chromium), 2014-08-01 00:38:53]

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
(pending_configure_reply_.is_valid()) {
On 2014/08/01 00:15:43, teravest wrote:
> On 2014/07/31 23:54:48, Anand Mistry wrote:
> > On 2014/07/31 20:54:55, teravest wrote:
> > > I think you could DCHECK(pending_configure_reply_.is_valid()) here as
well.
> > 
> > I don't think so. This function will be called by InitBuffers(), which may
be
> > called by SetFormatOnMainThread(), which is unrelated to the Configure()
IPC.
> On
> > startup, this will happen, and there may not be a pending Configure() IPC
call
> > in progress.
> 
> Hmm, okay.
> How is it ensured that a reply will always be sent for a Configure message?
> 
> Suppose at line 111, changed is true, but bytes_per_second_ is set. What
happens
> to pending_configure_reply_?

In this case, it will call InitBuffer(), which will call into
SendConfigureReply(). Since pending_configure_reply_ is valid, because it was
set on line 100, the IPC will complete.

Now, if bytes_per_second_ is not set, then the IPC will remain pending until the
AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will complete
the IPC.

So, there are three cases to consider:
1. No changes. Then the reply will happen via line 116.
2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply will
happen via InitBuffer().
3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the reply
will happen later when AudioSink::OnSetFormat() is run.

[Peng, 2014-08-01 14:27:44]

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
(pending_configure_reply_.is_valid()) {
On 2014/08/01 00:38:52, Anand Mistry wrote:
> On 2014/08/01 00:15:43, teravest wrote:
> > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > On 2014/07/31 20:54:55, teravest wrote:
> > > > I think you could DCHECK(pending_configure_reply_.is_valid()) here as
> well.
> > > 
> > > I don't think so. This function will be called by InitBuffers(), which may
> be
> > > called by SetFormatOnMainThread(), which is unrelated to the Configure()
> IPC.
> > On
> > > startup, this will happen, and there may not be a pending Configure() IPC
> call
> > > in progress.
> > 
> > Hmm, okay.
> > How is it ensured that a reply will always be sent for a Configure message?
> > 
> > Suppose at line 111, changed is true, but bytes_per_second_ is set. What
> happens
> > to pending_configure_reply_?
> 
> In this case, it will call InitBuffer(), which will call into
> SendConfigureReply(). Since pending_configure_reply_ is valid, because it was
> set on line 100, the IPC will complete.
> 
> Now, if bytes_per_second_ is not set, then the IPC will remain pending until
the
> AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will complete
> the IPC.
> 
> So, there are three cases to consider:
> 1. No changes. Then the reply will happen via line 116.
> 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply will
> happen via InitBuffer().
> 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the reply
> will happen later when AudioSink::OnSetFormat() is run.

Is this CL related to the GetAttrib() CL? What will GetAttrib() return, if the
Configure() is not called by plugin, but OnSetFormat() has been called in host
side?

[teravest, 2014-08-01 20:30:50]

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
(pending_configure_reply_.is_valid()) {
On 2014/08/01 00:38:52, Anand Mistry wrote:
> On 2014/08/01 00:15:43, teravest wrote:
> > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > On 2014/07/31 20:54:55, teravest wrote:
> > > > I think you could DCHECK(pending_configure_reply_.is_valid()) here as
> well.
> > > 
> > > I don't think so. This function will be called by InitBuffers(), which may
> be
> > > called by SetFormatOnMainThread(), which is unrelated to the Configure()
> IPC.
> > On
> > > startup, this will happen, and there may not be a pending Configure() IPC
> call
> > > in progress.
> > 
> > Hmm, okay.
> > How is it ensured that a reply will always be sent for a Configure message?
> > 
> > Suppose at line 111, changed is true, but bytes_per_second_ is set. What
> happens
> > to pending_configure_reply_?
> 
> In this case, it will call InitBuffer(), which will call into
> SendConfigureReply(). Since pending_configure_reply_ is valid, because it was
> set on line 100, the IPC will complete.
> 
> Now, if bytes_per_second_ is not set, then the IPC will remain pending until
the
> AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will complete
> the IPC.
> 
> So, there are three cases to consider:
> 1. No changes. Then the reply will happen via line 116.
> 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply will
> happen via InitBuffer().
> 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the reply
> will happen later when AudioSink::OnSetFormat() is run.

Is OnSetFormat() guaranteed to get called?

[Anand Mistry (off Chromium), 2014-08-04 00:20:20]

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
(pending_configure_reply_.is_valid()) {
On 2014/08/01 14:27:44, Peng wrote:
> On 2014/08/01 00:38:52, Anand Mistry wrote:
> > On 2014/08/01 00:15:43, teravest wrote:
> > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > I think you could DCHECK(pending_configure_reply_.is_valid()) here as
> > well.
> > > > 
> > > > I don't think so. This function will be called by InitBuffers(), which
may
> > be
> > > > called by SetFormatOnMainThread(), which is unrelated to the Configure()
> > IPC.
> > > On
> > > > startup, this will happen, and there may not be a pending Configure()
IPC
> > call
> > > > in progress.
> > > 
> > > Hmm, okay.
> > > How is it ensured that a reply will always be sent for a Configure
message?
> > > 
> > > Suppose at line 111, changed is true, but bytes_per_second_ is set. What
> > happens
> > > to pending_configure_reply_?
> > 
> > In this case, it will call InitBuffer(), which will call into
> > SendConfigureReply(). Since pending_configure_reply_ is valid, because it
was
> > set on line 100, the IPC will complete.
> > 
> > Now, if bytes_per_second_ is not set, then the IPC will remain pending until
> the
> > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will
complete
> > the IPC.
> > 
> > So, there are three cases to consider:
> > 1. No changes. Then the reply will happen via line 116.
> > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply
will
> > happen via InitBuffer().
> > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the reply
> > will happen later when AudioSink::OnSetFormat() is run.
> 
> Is this CL related to the GetAttrib() CL? What will GetAttrib() return, if the
> Configure() is not called by plugin, but OnSetFormat() has been called in host
> side?

Related, but orthogonal. dmichael's comments (and my reply) touched on this. I
think we have two options:
1. Error.
2. Return some hard-coded set of defaults.

But I think that discussion should be held on the other CL.

https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
(pending_configure_reply_.is_valid()) {
On 2014/08/01 20:30:50, teravest wrote:
> On 2014/08/01 00:38:52, Anand Mistry wrote:
> > On 2014/08/01 00:15:43, teravest wrote:
> > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > I think you could DCHECK(pending_configure_reply_.is_valid()) here as
> > well.
> > > > 
> > > > I don't think so. This function will be called by InitBuffers(), which
may
> > be
> > > > called by SetFormatOnMainThread(), which is unrelated to the Configure()
> > IPC.
> > > On
> > > > startup, this will happen, and there may not be a pending Configure()
IPC
> > call
> > > > in progress.
> > > 
> > > Hmm, okay.
> > > How is it ensured that a reply will always be sent for a Configure
message?
> > > 
> > > Suppose at line 111, changed is true, but bytes_per_second_ is set. What
> > happens
> > > to pending_configure_reply_?
> > 
> > In this case, it will call InitBuffer(), which will call into
> > SendConfigureReply(). Since pending_configure_reply_ is valid, because it
was
> > set on line 100, the IPC will complete.
> > 
> > Now, if bytes_per_second_ is not set, then the IPC will remain pending until
> the
> > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will
complete
> > the IPC.
> > 
> > So, there are three cases to consider:
> > 1. No changes. Then the reply will happen via line 116.
> > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply
will
> > happen via InitBuffer().
> > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the reply
> > will happen later when AudioSink::OnSetFormat() is run.
> 
> Is OnSetFormat() guaranteed to get called?

Hmmm. I guess not. I can think of two cases where it won't.
1. The user closes the track immediately after calling configure, called right
after opening the track. In that case, there hasn't been time for the
OnSetFormat() to happen. When the track is closed, it is disconnected from the
audio source, and so OnSetFormat() will never be called. I've fixed this case by
replying with an ABORTED error for the pending configure when closing.
2. If there's an error with the audio input system, then we might not get
OnSetFormat() if there are no audio buffers received (since calls to
OnSetFormat() appear to be tied to receiving audio buffers in the webtrc code).
Unfortunately, we don't appear to get any indication this is happening. However,
the client can use a timeout and close the track if the configure call times
out. Closing the track will then finish the configure call.

[teravest, 2014-08-04 20:39:24]

On 2014/08/04 00:20:20, Anand Mistry wrote:
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):
> 
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
> (pending_configure_reply_.is_valid()) {
> On 2014/08/01 14:27:44, Peng wrote:
> > On 2014/08/01 00:38:52, Anand Mistry wrote:
> > > On 2014/08/01 00:15:43, teravest wrote:
> > > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > > I think you could DCHECK(pending_configure_reply_.is_valid()) here
as
> > > well.
> > > > > 
> > > > > I don't think so. This function will be called by InitBuffers(), which
> may
> > > be
> > > > > called by SetFormatOnMainThread(), which is unrelated to the
Configure()
> > > IPC.
> > > > On
> > > > > startup, this will happen, and there may not be a pending Configure()
> IPC
> > > call
> > > > > in progress.
> > > > 
> > > > Hmm, okay.
> > > > How is it ensured that a reply will always be sent for a Configure
> message?
> > > > 
> > > > Suppose at line 111, changed is true, but bytes_per_second_ is set. What
> > > happens
> > > > to pending_configure_reply_?
> > > 
> > > In this case, it will call InitBuffer(), which will call into
> > > SendConfigureReply(). Since pending_configure_reply_ is valid, because it
> was
> > > set on line 100, the IPC will complete.
> > > 
> > > Now, if bytes_per_second_ is not set, then the IPC will remain pending
until
> > the
> > > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will
> complete
> > > the IPC.
> > > 
> > > So, there are three cases to consider:
> > > 1. No changes. Then the reply will happen via line 116.
> > > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply
> will
> > > happen via InitBuffer().
> > > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the
reply
> > > will happen later when AudioSink::OnSetFormat() is run.
> > 
> > Is this CL related to the GetAttrib() CL? What will GetAttrib() return, if
the
> > Configure() is not called by plugin, but OnSetFormat() has been called in
host
> > side?
> 
> Related, but orthogonal. dmichael's comments (and my reply) touched on this. I
> think we have two options:
> 1. Error.
> 2. Return some hard-coded set of defaults.
> 
> But I think that discussion should be held on the other CL.
> 
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
> (pending_configure_reply_.is_valid()) {
> On 2014/08/01 20:30:50, teravest wrote:
> > On 2014/08/01 00:38:52, Anand Mistry wrote:
> > > On 2014/08/01 00:15:43, teravest wrote:
> > > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > > I think you could DCHECK(pending_configure_reply_.is_valid()) here
as
> > > well.
> > > > > 
> > > > > I don't think so. This function will be called by InitBuffers(), which
> may
> > > be
> > > > > called by SetFormatOnMainThread(), which is unrelated to the
Configure()
> > > IPC.
> > > > On
> > > > > startup, this will happen, and there may not be a pending Configure()
> IPC
> > > call
> > > > > in progress.
> > > > 
> > > > Hmm, okay.
> > > > How is it ensured that a reply will always be sent for a Configure
> message?
> > > > 
> > > > Suppose at line 111, changed is true, but bytes_per_second_ is set. What
> > > happens
> > > > to pending_configure_reply_?
> > > 
> > > In this case, it will call InitBuffer(), which will call into
> > > SendConfigureReply(). Since pending_configure_reply_ is valid, because it
> was
> > > set on line 100, the IPC will complete.
> > > 
> > > Now, if bytes_per_second_ is not set, then the IPC will remain pending
until
> > the
> > > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will
> complete
> > > the IPC.
> > > 
> > > So, there are three cases to consider:
> > > 1. No changes. Then the reply will happen via line 116.
> > > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply
> will
> > > happen via InitBuffer().
> > > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the
reply
> > > will happen later when AudioSink::OnSetFormat() is run.
> > 
> > Is OnSetFormat() guaranteed to get called?
> 
> Hmmm. I guess not. I can think of two cases where it won't.
> 1. The user closes the track immediately after calling configure, called right
> after opening the track. In that case, there hasn't been time for the
> OnSetFormat() to happen. When the track is closed, it is disconnected from the
> audio source, and so OnSetFormat() will never be called. I've fixed this case
by
> replying with an ABORTED error for the pending configure when closing.
> 2. If there's an error with the audio input system, then we might not get
> OnSetFormat() if there are no audio buffers received (since calls to
> OnSetFormat() appear to be tied to receiving audio buffers in the webtrc
code).
> Unfortunately, we don't appear to get any indication this is happening.
However,
> the client can use a timeout and close the track if the configure call times
> out. Closing the track will then finish the configure call.

Can you clarify who "the client" is in (2)? This is a requirement of
applications that use the Pepper interface? Is this documented anywhere?

I'm nervous about having a complicated state machine where a message from the
plugin won't get a reply.

[Anand Mistry (off Chromium), 2014-08-04 23:57:32]

On 2014/08/04 20:39:24, teravest wrote:
> On 2014/08/04 00:20:20, Anand Mistry wrote:
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > File content/renderer/pepper/pepper_media_stream_audio_track_host.cc
(right):
> > 
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
> > (pending_configure_reply_.is_valid()) {
> > On 2014/08/01 14:27:44, Peng wrote:
> > > On 2014/08/01 00:38:52, Anand Mistry wrote:
> > > > On 2014/08/01 00:15:43, teravest wrote:
> > > > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > > > I think you could DCHECK(pending_configure_reply_.is_valid()) here
> as
> > > > well.
> > > > > > 
> > > > > > I don't think so. This function will be called by InitBuffers(),
which
> > may
> > > > be
> > > > > > called by SetFormatOnMainThread(), which is unrelated to the
> Configure()
> > > > IPC.
> > > > > On
> > > > > > startup, this will happen, and there may not be a pending
Configure()
> > IPC
> > > > call
> > > > > > in progress.
> > > > > 
> > > > > Hmm, okay.
> > > > > How is it ensured that a reply will always be sent for a Configure
> > message?
> > > > > 
> > > > > Suppose at line 111, changed is true, but bytes_per_second_ is set.
What
> > > > happens
> > > > > to pending_configure_reply_?
> > > > 
> > > > In this case, it will call InitBuffer(), which will call into
> > > > SendConfigureReply(). Since pending_configure_reply_ is valid, because
it
> > was
> > > > set on line 100, the IPC will complete.
> > > > 
> > > > Now, if bytes_per_second_ is not set, then the IPC will remain pending
> until
> > > the
> > > > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will
> > complete
> > > > the IPC.
> > > > 
> > > > So, there are three cases to consider:
> > > > 1. No changes. Then the reply will happen via line 116.
> > > > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply
> > will
> > > > happen via InitBuffer().
> > > > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the
> reply
> > > > will happen later when AudioSink::OnSetFormat() is run.
> > > 
> > > Is this CL related to the GetAttrib() CL? What will GetAttrib() return, if
> the
> > > Configure() is not called by plugin, but OnSetFormat() has been called in
> host
> > > side?
> > 
> > Related, but orthogonal. dmichael's comments (and my reply) touched on this.
I
> > think we have two options:
> > 1. Error.
> > 2. Return some hard-coded set of defaults.
> > 
> > But I think that discussion should be held on the other CL.
> > 
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
> > (pending_configure_reply_.is_valid()) {
> > On 2014/08/01 20:30:50, teravest wrote:
> > > On 2014/08/01 00:38:52, Anand Mistry wrote:
> > > > On 2014/08/01 00:15:43, teravest wrote:
> > > > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > > > I think you could DCHECK(pending_configure_reply_.is_valid()) here
> as
> > > > well.
> > > > > > 
> > > > > > I don't think so. This function will be called by InitBuffers(),
which
> > may
> > > > be
> > > > > > called by SetFormatOnMainThread(), which is unrelated to the
> Configure()
> > > > IPC.
> > > > > On
> > > > > > startup, this will happen, and there may not be a pending
Configure()
> > IPC
> > > > call
> > > > > > in progress.
> > > > > 
> > > > > Hmm, okay.
> > > > > How is it ensured that a reply will always be sent for a Configure
> > message?
> > > > > 
> > > > > Suppose at line 111, changed is true, but bytes_per_second_ is set.
What
> > > > happens
> > > > > to pending_configure_reply_?
> > > > 
> > > > In this case, it will call InitBuffer(), which will call into
> > > > SendConfigureReply(). Since pending_configure_reply_ is valid, because
it
> > was
> > > > set on line 100, the IPC will complete.
> > > > 
> > > > Now, if bytes_per_second_ is not set, then the IPC will remain pending
> until
> > > the
> > > > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will
> > complete
> > > > the IPC.
> > > > 
> > > > So, there are three cases to consider:
> > > > 1. No changes. Then the reply will happen via line 116.
> > > > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the reply
> > will
> > > > happen via InitBuffer().
> > > > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the
> reply
> > > > will happen later when AudioSink::OnSetFormat() is run.
> > > 
> > > Is OnSetFormat() guaranteed to get called?
> > 
> > Hmmm. I guess not. I can think of two cases where it won't.
> > 1. The user closes the track immediately after calling configure, called
right
> > after opening the track. In that case, there hasn't been time for the
> > OnSetFormat() to happen. When the track is closed, it is disconnected from
the
> > audio source, and so OnSetFormat() will never be called. I've fixed this
case
> by
> > replying with an ABORTED error for the pending configure when closing.
> > 2. If there's an error with the audio input system, then we might not get
> > OnSetFormat() if there are no audio buffers received (since calls to
> > OnSetFormat() appear to be tied to receiving audio buffers in the webtrc
> code).
> > Unfortunately, we don't appear to get any indication this is happening.
> However,
> > the client can use a timeout and close the track if the configure call times
> > out. Closing the track will then finish the configure call.
> 

> Can you clarify who "the client" is in (2)?
Sorry. I meant Pepper plugin (such as a NaCl module).

> This is a requirement of applications that use the Pepper interface?
> Is this documented anywhere?
There isn't really any documentation on this interface. There's
https://developer.chrome.com/native-client/pepper_stable/c/struct_p_p_b___med...,
but that doesn't say much.
 
> I'm nervous about having a complicated state machine where a message from the
> plugin won't get a reply.

Understood. I guess I could add a timeout in the host, to abort a pending
Configure() if it's taking too long. Would that alleviate your concerns? Albeit,
probably with an even more complicated state machine.

[Anand Mistry (off Chromium), 2014-08-12 00:05:19]

Ping!

[teravest, 2014-08-12 15:12:17]

On 2014/08/04 23:57:32, Anand Mistry wrote:
> On 2014/08/04 20:39:24, teravest wrote:
> > On 2014/08/04 00:20:20, Anand Mistry wrote:
> > >
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > > File content/renderer/pepper/pepper_media_stream_audio_track_host.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > > content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
> > > (pending_configure_reply_.is_valid()) {
> > > On 2014/08/01 14:27:44, Peng wrote:
> > > > On 2014/08/01 00:38:52, Anand Mistry wrote:
> > > > > On 2014/08/01 00:15:43, teravest wrote:
> > > > > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > > > > I think you could DCHECK(pending_configure_reply_.is_valid())
here
> > as
> > > > > well.
> > > > > > > 
> > > > > > > I don't think so. This function will be called by InitBuffers(),
> which
> > > may
> > > > > be
> > > > > > > called by SetFormatOnMainThread(), which is unrelated to the
> > Configure()
> > > > > IPC.
> > > > > > On
> > > > > > > startup, this will happen, and there may not be a pending
> Configure()
> > > IPC
> > > > > call
> > > > > > > in progress.
> > > > > > 
> > > > > > Hmm, okay.
> > > > > > How is it ensured that a reply will always be sent for a Configure
> > > message?
> > > > > > 
> > > > > > Suppose at line 111, changed is true, but bytes_per_second_ is set.
> What
> > > > > happens
> > > > > > to pending_configure_reply_?
> > > > > 
> > > > > In this case, it will call InitBuffer(), which will call into
> > > > > SendConfigureReply(). Since pending_configure_reply_ is valid, because
> it
> > > was
> > > > > set on line 100, the IPC will complete.
> > > > > 
> > > > > Now, if bytes_per_second_ is not set, then the IPC will remain pending
> > until
> > > > the
> > > > > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will
> > > complete
> > > > > the IPC.
> > > > > 
> > > > > So, there are three cases to consider:
> > > > > 1. No changes. Then the reply will happen via line 116.
> > > > > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the
reply
> > > will
> > > > > happen via InitBuffer().
> > > > > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the
> > reply
> > > > > will happen later when AudioSink::OnSetFormat() is run.
> > > > 
> > > > Is this CL related to the GetAttrib() CL? What will GetAttrib() return,
if
> > the
> > > > Configure() is not called by plugin, but OnSetFormat() has been called
in
> > host
> > > > side?
> > > 
> > > Related, but orthogonal. dmichael's comments (and my reply) touched on
this.
> I
> > > think we have two options:
> > > 1. Error.
> > > 2. Return some hard-coded set of defaults.
> > > 
> > > But I think that discussion should be held on the other CL.
> > > 
> > >
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > > content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
> > > (pending_configure_reply_.is_valid()) {
> > > On 2014/08/01 20:30:50, teravest wrote:
> > > > On 2014/08/01 00:38:52, Anand Mistry wrote:
> > > > > On 2014/08/01 00:15:43, teravest wrote:
> > > > > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > > > > I think you could DCHECK(pending_configure_reply_.is_valid())
here
> > as
> > > > > well.
> > > > > > > 
> > > > > > > I don't think so. This function will be called by InitBuffers(),
> which
> > > may
> > > > > be
> > > > > > > called by SetFormatOnMainThread(), which is unrelated to the
> > Configure()
> > > > > IPC.
> > > > > > On
> > > > > > > startup, this will happen, and there may not be a pending
> Configure()
> > > IPC
> > > > > call
> > > > > > > in progress.
> > > > > > 
> > > > > > Hmm, okay.
> > > > > > How is it ensured that a reply will always be sent for a Configure
> > > message?
> > > > > > 
> > > > > > Suppose at line 111, changed is true, but bytes_per_second_ is set.
> What
> > > > > happens
> > > > > > to pending_configure_reply_?
> > > > > 
> > > > > In this case, it will call InitBuffer(), which will call into
> > > > > SendConfigureReply(). Since pending_configure_reply_ is valid, because
> it
> > > was
> > > > > set on line 100, the IPC will complete.
> > > > > 
> > > > > Now, if bytes_per_second_ is not set, then the IPC will remain pending
> > until
> > > > the
> > > > > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and will
> > > complete
> > > > > the IPC.
> > > > > 
> > > > > So, there are three cases to consider:
> > > > > 1. No changes. Then the reply will happen via line 116.
> > > > > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the
reply
> > > will
> > > > > happen via InitBuffer().
> > > > > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then the
> > reply
> > > > > will happen later when AudioSink::OnSetFormat() is run.
> > > > 
> > > > Is OnSetFormat() guaranteed to get called?
> > > 
> > > Hmmm. I guess not. I can think of two cases where it won't.
> > > 1. The user closes the track immediately after calling configure, called
> right
> > > after opening the track. In that case, there hasn't been time for the
> > > OnSetFormat() to happen. When the track is closed, it is disconnected from
> the
> > > audio source, and so OnSetFormat() will never be called. I've fixed this
> case
> > by
> > > replying with an ABORTED error for the pending configure when closing.
> > > 2. If there's an error with the audio input system, then we might not get
> > > OnSetFormat() if there are no audio buffers received (since calls to
> > > OnSetFormat() appear to be tied to receiving audio buffers in the webtrc
> > code).
> > > Unfortunately, we don't appear to get any indication this is happening.
> > However,
> > > the client can use a timeout and close the track if the configure call
times
> > > out. Closing the track will then finish the configure call.
> > 
> 
> > Can you clarify who "the client" is in (2)?
> Sorry. I meant Pepper plugin (such as a NaCl module).
> 
> > This is a requirement of applications that use the Pepper interface?
> > Is this documented anywhere?
> There isn't really any documentation on this interface. There's
>
https://developer.chrome.com/native-client/pepper_stable/c/struct_p_p_b___med...,
> but that doesn't say much.
>  
> > I'm nervous about having a complicated state machine where a message from
the
> > plugin won't get a reply.
> 
> Understood. I guess I could add a timeout in the host, to abort a pending
> Configure() if it's taking too long. Would that alleviate your concerns?
Albeit,
> probably with an even more complicated state machine.

I think adding a timeout is going to make things even messier. I suppose that in
the case of (2), if OnSetFormat() is never received, the Configure() call will
still get called with ABORTED when the track is closed, now?

[Anand Mistry (off Chromium), 2014-08-13 03:10:35]

On 2014/08/12 15:12:17, teravest wrote:
> On 2014/08/04 23:57:32, Anand Mistry wrote:
> > On 2014/08/04 20:39:24, teravest wrote:
> > > On 2014/08/04 00:20:20, Anand Mistry wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > > > File content/renderer/pepper/pepper_media_stream_audio_track_host.cc
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > > > content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
> > > > (pending_configure_reply_.is_valid()) {
> > > > On 2014/08/01 14:27:44, Peng wrote:
> > > > > On 2014/08/01 00:38:52, Anand Mistry wrote:
> > > > > > On 2014/08/01 00:15:43, teravest wrote:
> > > > > > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > > > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > > > > > I think you could DCHECK(pending_configure_reply_.is_valid())
> here
> > > as
> > > > > > well.
> > > > > > > > 
> > > > > > > > I don't think so. This function will be called by InitBuffers(),
> > which
> > > > may
> > > > > > be
> > > > > > > > called by SetFormatOnMainThread(), which is unrelated to the
> > > Configure()
> > > > > > IPC.
> > > > > > > On
> > > > > > > > startup, this will happen, and there may not be a pending
> > Configure()
> > > > IPC
> > > > > > call
> > > > > > > > in progress.
> > > > > > > 
> > > > > > > Hmm, okay.
> > > > > > > How is it ensured that a reply will always be sent for a Configure
> > > > message?
> > > > > > > 
> > > > > > > Suppose at line 111, changed is true, but bytes_per_second_ is
set.
> > What
> > > > > > happens
> > > > > > > to pending_configure_reply_?
> > > > > > 
> > > > > > In this case, it will call InitBuffer(), which will call into
> > > > > > SendConfigureReply(). Since pending_configure_reply_ is valid,
because
> > it
> > > > was
> > > > > > set on line 100, the IPC will complete.
> > > > > > 
> > > > > > Now, if bytes_per_second_ is not set, then the IPC will remain
pending
> > > until
> > > > > the
> > > > > > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and
will
> > > > complete
> > > > > > the IPC.
> > > > > > 
> > > > > > So, there are three cases to consider:
> > > > > > 1. No changes. Then the reply will happen via line 116.
> > > > > > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the
> reply
> > > > will
> > > > > > happen via InitBuffer().
> > > > > > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then
the
> > > reply
> > > > > > will happen later when AudioSink::OnSetFormat() is run.
> > > > > 
> > > > > Is this CL related to the GetAttrib() CL? What will GetAttrib()
return,
> if
> > > the
> > > > > Configure() is not called by plugin, but OnSetFormat() has been called
> in
> > > host
> > > > > side?
> > > > 
> > > > Related, but orthogonal. dmichael's comments (and my reply) touched on
> this.
> > I
> > > > think we have two options:
> > > > 1. Error.
> > > > 2. Return some hard-coded set of defaults.
> > > > 
> > > > But I think that discussion should be held on the other CL.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/430943004/diff/1/content/renderer/pepper/pepp...
> > > > content/renderer/pepper/pepper_media_stream_audio_track_host.cc:123: if
> > > > (pending_configure_reply_.is_valid()) {
> > > > On 2014/08/01 20:30:50, teravest wrote:
> > > > > On 2014/08/01 00:38:52, Anand Mistry wrote:
> > > > > > On 2014/08/01 00:15:43, teravest wrote:
> > > > > > > On 2014/07/31 23:54:48, Anand Mistry wrote:
> > > > > > > > On 2014/07/31 20:54:55, teravest wrote:
> > > > > > > > > I think you could DCHECK(pending_configure_reply_.is_valid())
> here
> > > as
> > > > > > well.
> > > > > > > > 
> > > > > > > > I don't think so. This function will be called by InitBuffers(),
> > which
> > > > may
> > > > > > be
> > > > > > > > called by SetFormatOnMainThread(), which is unrelated to the
> > > Configure()
> > > > > > IPC.
> > > > > > > On
> > > > > > > > startup, this will happen, and there may not be a pending
> > Configure()
> > > > IPC
> > > > > > call
> > > > > > > > in progress.
> > > > > > > 
> > > > > > > Hmm, okay.
> > > > > > > How is it ensured that a reply will always be sent for a Configure
> > > > message?
> > > > > > > 
> > > > > > > Suppose at line 111, changed is true, but bytes_per_second_ is
set.
> > What
> > > > > > happens
> > > > > > > to pending_configure_reply_?
> > > > > > 
> > > > > > In this case, it will call InitBuffer(), which will call into
> > > > > > SendConfigureReply(). Since pending_configure_reply_ is valid,
because
> > it
> > > > was
> > > > > > set on line 100, the IPC will complete.
> > > > > > 
> > > > > > Now, if bytes_per_second_ is not set, then the IPC will remain
pending
> > > until
> > > > > the
> > > > > > AudioSink::OnSetFormat() is run, which triggers InitBuffer() and
will
> > > > complete
> > > > > > the IPC.
> > > > > > 
> > > > > > So, there are three cases to consider:
> > > > > > 1. No changes. Then the reply will happen via line 116.
> > > > > > 2. Changes and bytes_per_second_/bytes_per_frame_ is set. Then the
> reply
> > > > will
> > > > > > happen via InitBuffer().
> > > > > > 3. Changes and bytes_per_second_/bytes_per_frame_ is NOT set. Then
the
> > > reply
> > > > > > will happen later when AudioSink::OnSetFormat() is run.
> > > > > 
> > > > > Is OnSetFormat() guaranteed to get called?
> > > > 
> > > > Hmmm. I guess not. I can think of two cases where it won't.
> > > > 1. The user closes the track immediately after calling configure, called
> > right
> > > > after opening the track. In that case, there hasn't been time for the
> > > > OnSetFormat() to happen. When the track is closed, it is disconnected
from
> > the
> > > > audio source, and so OnSetFormat() will never be called. I've fixed this
> > case
> > > by
> > > > replying with an ABORTED error for the pending configure when closing.
> > > > 2. If there's an error with the audio input system, then we might not
get
> > > > OnSetFormat() if there are no audio buffers received (since calls to
> > > > OnSetFormat() appear to be tied to receiving audio buffers in the webtrc
> > > code).
> > > > Unfortunately, we don't appear to get any indication this is happening.
> > > However,
> > > > the client can use a timeout and close the track if the configure call
> times
> > > > out. Closing the track will then finish the configure call.
> > > 
> > 
> > > Can you clarify who "the client" is in (2)?
> > Sorry. I meant Pepper plugin (such as a NaCl module).
> > 
> > > This is a requirement of applications that use the Pepper interface?
> > > Is this documented anywhere?
> > There isn't really any documentation on this interface. There's
> >
>
https://developer.chrome.com/native-client/pepper_stable/c/struct_p_p_b___med...,
> > but that doesn't say much.
> >  
> > > I'm nervous about having a complicated state machine where a message from
> the
> > > plugin won't get a reply.
> > 
> > Understood. I guess I could add a timeout in the host, to abort a pending
> > Configure() if it's taking too long. Would that alleviate your concerns?
> Albeit,
> > probably with an even more complicated state machine.
> 
> I think adding a timeout is going to make things even messier. I suppose that
in
> the case of (2), if OnSetFormat() is never received, the Configure() call will
> still get called with ABORTED when the track is closed, now?

Yup. TestConfigureClose() I added exercises this case (but not always, since it
depends on timing). If you call Configure() after calling Close(), it returns
immediately with PP_ERROR_FAILED and never contacts the host.

[teravest, 2014-08-14 16:12:35]

lgtm

[Peng, 2014-08-14 16:36:11]

On 2014/08/14 16:12:35, teravest wrote:
> lgtm

lgtm

[Anand Mistry (off Chromium), 2014-08-14 23:14:03]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-08-14 23:16:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/amistry@chromium.org/430943004/60001

[commit-bot: I haz the power, 2014-08-15 05:24:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[commit-bot: I haz the power, 2014-08-15 05:40:02]

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

[commit-bot: I haz the power, 2014-08-15 05:40:03]

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

[Anand Mistry (off Chromium), 2014-08-15 05:54:36]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-08-15 05:57:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/amistry@chromium.org/430943004/60001

[commit-bot: I haz the power, 2014-08-15 14:56:25]

Committed patchset #4 (60001) as 289856