Ensures that input Pepper Flash supports the newly added AudioBus interface

ppapi/proxy/audio_input_resource.cc

Index: ppapi/proxy/audio_input_resource.cc
diff --git a/ppapi/proxy/audio_input_resource.cc b/ppapi/proxy/audio_input_resource.cc
index 0ae1831afa8cf9c66409cde60cce814683696ce5..17c7a4567f6ea12a4c2e87c82e1a316c8efc77f6 100644
--- a/ppapi/proxy/audio_input_resource.cc
+++ b/ppapi/proxy/audio_input_resource.cc
@@ -8,6 +8,7 @@
 #include "base/logging.h"
 #include "ipc/ipc_platform_file.h"
 #include "media/audio/audio_parameters.h"
+#include "media/base/audio_bus.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/resource_message_params.h"
@@ -33,7 +34,9 @@ AudioInputResource::AudioInputResource(
       audio_input_callback_(NULL),
       user_data_(NULL),
       enumeration_helper_(this),
-      bytes_per_second_(0) {
+      bytes_per_second_(0),
+      sample_frame_count_(0),
+      client_buffer_size_bytes_(0) {
   SendCreate(RENDERER, PpapiHostMsg_AudioInput_Create());
 }
 
@@ -188,6 +191,20 @@ void AudioInputResource::SetStreamInfo(
         PP_LOGLEVEL_WARNING,
         std::string(),
         "Failed to map shared memory for PPB_AudioInput_Shared.");
+  } else {
+    // Create a new audio bus and wrap the audio data section in shared memory.
+    media::AudioInputBuffer* buffer =
+        reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory());

[piman, 2014/07/08 17:48:35]

I see you're sharing metadata (AudioInputBufferParameters) via shared memory
between trusted and untrusted side. Can anything bad happen if the untrusted
side writes to the shared memory, modifying, e.g. the size?

[piman, 2014/07/08 17:48:35]

nit: static_cast instead of reinterpret_cast.

[tommi (sloooow) - chröme, 2014/07/09 09:32:11]

Henrik was asking me about this, so I figured I'd simply reply in the code
review, hope you don't mind.

From http://en.cppreference.com/w/cpp/language/reinterpret_cast:
"Unlike static_cast, but like const_cast, the reinterpret_cast expression does
not compile to any CPU instructions. It is purely a compiler directive which
instructs the compiler to treat the sequence of bits (object representation) of
expression as if it had the type new_type."

In this case, memory() returns void* which is not a related type to
AudioInputBuffer* so there's nothing the compiler can or should do to convert
between these two types, so reinterpret_cast is the right type of cast. 
static_cast<> won't do anything in practice in exactly this case, but
technically, reinterpret_cast<> is correct and in order to not propagate
incorrect use of static_cast, I'd like to keep it as is.

[henrika (OOO until Aug 14), 2014/07/09 09:39:51]

Nop. I was following a pattern done in a corresponding output class (see
PPB_Audio_Shared). 

[henrika (OOO until Aug 14), 2014/07/09 09:39:51]

Great comment; please allow me to come back and answer this one next round.
Please note that I have not added any meta data in this CL; I just use already
existing as before. I'll try to improve by adding some DCHECKs here.

[henrika (OOO until Aug 14), 2014/07/09 11:35:14]

Thanks Tommi; will stick to reinterpret unless piman@ has other input.

[piman, 2014/07/09 20:36:27]

Possibly, but you're exposing it in new ways (and I want to check that we didn't
miss anything before).
It's not so much about DCHECKs, it's more about having meta-data being writable
by the untrusted process. If the trusted process relies on that data not
changing / having particular values, then there is an attack surface (that
DCHECKs, especially on the untrusted side, won't protect you against) - e.g. if
the trusted side recycles the buffer and relies on the size value to be
accurate.

So I guess the question is: is it the case that the meta-data is write-only by
the trusted side (never reads it)? If so we're fine. If the trusted side reads
it, does it need to defend against it changing?

[henrika (OOO until Aug 14), 2014/07/10 09:50:16]

We are fine since the trusted process only writes (never reads) this meta data
and the meta data is set up once (during initialization) and is not (or can't
be) changed during a session.

+    audio_bus_ = media::AudioBus::WrapMemory(
+        kAudioInputChannels, sample_frame_count_, buffer->audio);
+
+    // Create an extra integer audio buffer for user audio data callbacks.
+    // Data in shared memory will be copied to this buffer, after interleaving
+    // and truncation, before each input callback to match the format expected
+    // by the client.
+    client_buffer_size_bytes_ = audio_bus_->frames() * audio_bus_->channels() *
+                                kBitsPerAudioInputSample / 8;

[piman, 2014/07/08 17:48:35]

nit: indent (git cl format?)

[henrika (OOO until Aug 14), 2014/07/09 09:39:51]

I did run git cl format and it does suggest this indentation. Do you want me to
change it anyhow?

[piman, 2014/07/09 20:36:27]

It looks like a bug in git cl format.... line continuations should be +4.

[henrika (OOO until Aug 14), 2014/07/10 09:50:16]

Acknowledged.

+    client_buffer_.reset(new uint8_t[client_buffer_size_bytes_]);
   }
 
   // There is a pending capture request before SetStreamInfo().
@@ -202,7 +219,8 @@ void AudioInputResource::SetStreamInfo(
 void AudioInputResource::StartThread() {
   // Don't start the thread unless all our state is set up correctly.
   if ((!audio_input_callback_0_3_ && !audio_input_callback_) ||
-      !socket_.get() || !capturing_ || !shared_memory_->memory()) {
+      !socket_.get() || !capturing_ || !shared_memory_->memory() ||
+      !audio_bus_.get() || !client_buffer_.get()) {
     return;
   }
   DCHECK(!audio_input_thread_.get());
@@ -223,16 +241,28 @@ void AudioInputResource::StopThread() {
 
 void AudioInputResource::Run() {
   // The shared memory represents AudioInputBufferParameters and the actual data
-  // buffer.
+  // buffer stored as an audio bus.
   media::AudioInputBuffer* buffer =
       static_cast<media::AudioInputBuffer*>(shared_memory_->memory());
   uint32_t data_buffer_size =
       shared_memory_size_ - sizeof(media::AudioInputBufferParameters);
-  int pending_data;
 

[no longer working on chromium, 2014/07/08 15:33:48]

if you move the audio_wrapper_ setup code here, will you save the
sample_frame_count_ and client_buffer_size_bytes_?
Like:
if (!audio_wrapper_) {
   audio_wrapper_ = media::AudioBus::WrapMemory(
         kAudioInputChannels, data_buffer_size, buffer->audio);
DCHECK_EQ(buffer->params.size, data_buffer_size * kBitsPerAudioInputSample / 8);
client_buffer_.reset(new uint8_t[buffer->params.size]);
}

[henrika (OOO until Aug 14), 2014/07/09 09:39:51]

Thanks for the suggestion but, as stated before, I would like to match the
corresponding output code as much as possible. Also, I don't want to create the
members on this special audio thread but on the main thread where all other
members are created.

-  while (sizeof(pending_data) == socket_->Receive(&pending_data,

[no longer working on chromium, 2014/07/08 15:33:48]

may I ask why you changed the condition? It looks the same functionality, or do
I miss something?

-                                                  sizeof(pending_data)) &&
-         pending_data >= 0) {
+  while (true) {
+    int pending_data = 0;
+    size_t bytes_read = socket_->Receive(&pending_data, sizeof(pending_data));
+    if (bytes_read != sizeof(pending_data)) {
+      DCHECK_EQ(bytes_read, 0U);
+      break;
+    }
+    if (pending_data < 0)
+      break;
+
+    // Convert an AudioBus from deinterleaved float to interleaved integer data.
+    // Store the result in a preallocated |client_buffer_|.
+    audio_bus_->ToInterleaved(audio_bus_->frames(),
+                              kBitsPerAudioInputSample / 8,
+                              client_buffer_.get());
+
     // While closing the stream, we may receive buffers whose size is different
     // from |data_buffer_size|.
     CHECK_LE(buffer->params.size, data_buffer_size);
@@ -240,11 +270,13 @@ void AudioInputResource::Run() {
       if (audio_input_callback_) {
         PP_TimeDelta latency =
             static_cast<double>(pending_data) / bytes_per_second_;
-        audio_input_callback_(&buffer->audio[0], buffer->params.size, latency,
+        audio_input_callback_(client_buffer_.get(),
+                              client_buffer_size_bytes_,

[no longer working on chromium, 2014/07/08 15:33:48]

client_buffer_size_bytes_ should be the same as buffer->params.size, if so,
please previous code instead of making a new member.

[henrika (OOO until Aug 14), 2014/07/09 09:39:51]

Actually that is not the case since params.size reflects the size of the audio
bus which is in float. A typical case for Pepper is the params.size is 8192 and
client_buffer_size_bytes_ is 4096 (since we use 16-bit integers instead of
float).

+                              latency,
                               user_data_);
       } else {
-        audio_input_callback_0_3_(&buffer->audio[0], buffer->params.size,
-                                  user_data_);
+        audio_input_callback_0_3_(
+            client_buffer_.get(), client_buffer_size_bytes_, user_data_);
       }
     }
   }
@@ -287,6 +319,7 @@ int32_t AudioInputResource::CommonOpen(
   open_callback_ = callback;
   bytes_per_second_ = kAudioInputChannels * (kBitsPerAudioInputSample / 8) *
                       enter_config.object()->GetSampleRate();
+  sample_frame_count_ = enter_config.object()->GetSampleFrameCount();
 
   PpapiHostMsg_AudioInput_Open msg(
       device_id, enter_config.object()->GetSampleRate(),