Ensures that input Pepper Flash supports the newly added AudioBus interface

ppapi/proxy/audio_input_resource.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ppapi/proxy/audio_input_resource.h"
 
 #include "base/bind.h"
 #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"
 #include "ppapi/proxy/serialized_handle.h"
 #include "ppapi/shared_impl/ppapi_globals.h"
 #include "ppapi/shared_impl/ppb_audio_config_shared.h"
 #include "ppapi/shared_impl/resource_tracker.h"
 #include "ppapi/shared_impl/tracked_callback.h"
 #include "ppapi/thunk/enter.h"
 #include "ppapi/thunk/ppb_audio_config_api.h"
 
 namespace ppapi {
 namespace proxy {
 
 AudioInputResource::AudioInputResource(
     Connection connection,
     PP_Instance instance)
     : PluginResource(connection, instance),
       open_state_(BEFORE_OPEN),
       capturing_(false),
       shared_memory_size_(0),
       audio_input_callback_0_3_(NULL),
       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());
 }
 
 AudioInputResource::~AudioInputResource() {
   Close();
 }
 
 thunk::PPB_AudioInput_API* AudioInputResource::AsPPB_AudioInput_API() {
   return this;
 }
@@ skipping 128 matching lines @@
 }
 
 void AudioInputResource::SetStreamInfo(
     base::SharedMemoryHandle shared_memory_handle,
     size_t shared_memory_size,
     base::SyncSocket::Handle socket_handle) {
   socket_.reset(new base::CancelableSyncSocket(socket_handle));
   shared_memory_.reset(new base::SharedMemory(shared_memory_handle, false));
   shared_memory_size_ = shared_memory_size;
 
   if (!shared_memory_->Map(shared_memory_size_)) {

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

The code becomes a bit hard to read with the new change, can you change it to:
if (shared_memory_->Map(shared_memory_size_)) {
  // Create a new audio bus and wrap the audio data section in shared memory.
   Do the setup.

} else {
  PpapiGlobals::Get()->LogWithSource(
}

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

Done.

[henrika (OOO until Aug 14), 2014/07/09 09:41:01]

I followed the example on the output side. I will try to make it more clear. 

     PpapiGlobals::Get()->LogWithSource(
         pp_instance(),
         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().
   if (capturing_) {
     // Set |capturing_| to false so that the state looks consistent to
     // StartCapture(), which will reset it to true.
     capturing_ = false;
     StartCapture();
   }
 }
 
 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());
   audio_input_thread_.reset(new base::DelegateSimpleThread(
       this, "plugin_audio_input_thread"));
   audio_input_thread_->Start();
 }
 
 void AudioInputResource::StopThread() {
   // Shut down the socket to escape any hanging |Receive|s.
   if (socket_.get())
     socket_->Shutdown();
   if (audio_input_thread_.get()) {
     audio_input_thread_->Join();
     audio_input_thread_.reset();
   }
 }
 
 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);
     if (buffer->params.size > 0) {
       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_);
       }
     }
   }
 }
 
 int32_t AudioInputResource::CommonOpen(
     PP_Resource device_ref,
     PP_Resource config,
     PPB_AudioInput_Callback_0_3 audio_input_callback_0_3,
     PPB_AudioInput_Callback audio_input_callback,
@@ skipping 22 matching lines @@
   if (enter_config.failed())
     return PP_ERROR_BADARGUMENT;
 
   config_ = config;
   audio_input_callback_0_3_ = audio_input_callback_0_3;
   audio_input_callback_ = audio_input_callback;
   user_data_ = user_data;
   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(),
       enter_config.object()->GetSampleFrameCount());
   Call<PpapiPluginMsg_AudioInput_OpenReply>(
       RENDERER, msg,
       base::Bind(&AudioInputResource::OnPluginMsgOpenReply,
                  base::Unretained(this)));
   return PP_OK_COMPLETIONPENDING;
 }
 }  // namespace proxy
 }  // namespace ppapi