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

content/renderer/pepper/pepper_media_stream_audio_track_host.cc

Index: content/renderer/pepper/pepper_media_stream_audio_track_host.cc
diff --git a/content/renderer/pepper/pepper_media_stream_audio_track_host.cc b/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
index 3069988773563454c958d8eb78e19e2d6786a1c6..1cbf0c94f84e626a6d12fbf262da0b63a2a9a77a 100644
--- a/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
+++ b/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
@@ -16,6 +16,7 @@
 #include "ppapi/c/ppb_audio_buffer.h"
 #include "ppapi/host/dispatch_host_message.h"
 #include "ppapi/host/host_message_context.h"
+#include "ppapi/host/ppapi_host.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/shared_impl/media_stream_audio_track_shared.h"
 #include "ppapi/shared_impl/media_stream_buffer.h"
@@ -88,9 +89,16 @@ void PepperMediaStreamAudioTrackHost::AudioSink::EnqueueBuffer(int32_t index) {
   buffers_.push_back(index);
 }
 
-void PepperMediaStreamAudioTrackHost::AudioSink::Configure(
-    int32_t number_of_buffers, int32_t duration) {
+int32_t PepperMediaStreamAudioTrackHost::AudioSink::Configure(
+    int32_t number_of_buffers, int32_t duration,
+    const ppapi::host::ReplyMessageContext& context) {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
+
+  if (pending_configure_reply_.is_valid()) {
+    return PP_ERROR_INPROGRESS;
+  }
+  pending_configure_reply_ = context;
+
   bool changed = false;
   if (number_of_buffers != number_of_buffers_)
     changed = true;
@@ -100,9 +108,25 @@ void PepperMediaStreamAudioTrackHost::AudioSink::Configure(
   }
   number_of_buffers_ = number_of_buffers;
 
-  // Initialize later in OnSetFormat if bytes_per_second_ is not know yet.
-  if (changed && bytes_per_second_ > 0 && bytes_per_frame_ > 0)
-    InitBuffers();
+  if (changed) {
+    // Initialize later in OnSetFormat if bytes_per_second_ is not know yet.

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

s/know/known/

[Anand Mistry (off Chromium), 2014/07/31 23:54:47]

Done.

+    if (bytes_per_second_ > 0 && bytes_per_frame_ > 0)
+      InitBuffers();
+  } else {
+    SendConfigureReply(PP_OK);
+  }
+  return PP_OK_COMPLETIONPENDING;
+}
+
+void PepperMediaStreamAudioTrackHost::AudioSink::SendConfigureReply(
+    int32_t result) {
+  if (pending_configure_reply_.is_valid()) {

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

I think you could DCHECK(pending_configure_reply_.is_valid()) here as well.

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

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.

[teravest, 2014/08/01 00:15:43]

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:52]

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]

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]

Is OnSetFormat() guaranteed to get called?

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

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.

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

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.

+    pending_configure_reply_.params.set_result(result);
+    host_->host()->SendReply(
+        pending_configure_reply_,
+        PpapiPluginMsg_MediaStreamAudioTrack_ConfigureReply());
+    pending_configure_reply_ = ppapi::host::ReplyMessageContext();
+  }
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::SetFormatOnMainThread(
@@ -140,8 +164,10 @@ void PepperMediaStreamAudioTrackHost::AudioSink::InitBuffers() {
   bool result = host_->InitBuffers(number_of_buffers_,
                                    buffer_size.ValueOrDie(),
                                    kRead);
-  // TODO(penghuang): Send PP_ERROR_NOMEMORY to plugin.
-  CHECK(result);
+  if (!result) {
+    SendConfigureReply(PP_ERROR_NOMEMORY);
+    return;
+  }
 
   // Fill the |buffers_|, so the audio thread can continue receiving audio data.
   base::AutoLock lock(lock_);
@@ -151,6 +177,8 @@ void PepperMediaStreamAudioTrackHost::AudioSink::InitBuffers() {
     DCHECK_GE(index, 0);
     buffers_.push_back(index);
   }
+
+  SendConfigureReply(PP_OK);
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::
@@ -320,10 +348,8 @@ int32_t PepperMediaStreamAudioTrackHost::OnHostMsgConfigure(
   int32_t buffers = attributes.buffers
                         ? std::min(kMaxNumberOfBuffers, attributes.buffers)
                         : kDefaultNumberOfBuffers;
-  audio_sink_.Configure(buffers, attributes.duration);
-
-  context->reply_msg = PpapiPluginMsg_MediaStreamAudioTrack_ConfigureReply();
-  return PP_OK;
+  return audio_sink_.Configure(buffers, attributes.duration,
+                               context->MakeReplyMessageContext());
 }
 
 void PepperMediaStreamAudioTrackHost::OnClose() {