Merge AssociateStreamWithProducer message into CreateStream message for both audio output and input.

content/browser/renderer_host/media/audio_renderer_host.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 "content/browser/renderer_host/media/audio_renderer_host.h"
 
 #include "base/bind.h"
 #include "base/metrics/histogram.h"
 #include "base/process.h"
 #include "base/shared_memory.h"
@@ skipping 222 matching lines @@
   DeleteEntryOnError(entry);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // IPC Messages handler
 bool AudioRendererHost::OnMessageReceived(const IPC::Message& message,
                                           bool* message_was_ok) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP_EX(AudioRendererHost, message, *message_was_ok)
     IPC_MESSAGE_HANDLER(AudioHostMsg_CreateStream, OnCreateStream)
-    IPC_MESSAGE_HANDLER(AudioHostMsg_AssociateStreamWithProducer,
-                        OnAssociateStreamWithProducer)
     IPC_MESSAGE_HANDLER(AudioHostMsg_PlayStream, OnPlayStream)
     IPC_MESSAGE_HANDLER(AudioHostMsg_PauseStream, OnPauseStream)
     IPC_MESSAGE_HANDLER(AudioHostMsg_FlushStream, OnFlushStream)
     IPC_MESSAGE_HANDLER(AudioHostMsg_CloseStream, OnCloseStream)
     IPC_MESSAGE_HANDLER(AudioHostMsg_SetVolume, OnSetVolume)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP_EX()
 
   return handled;
 }
 
 void AudioRendererHost::OnCreateStream(
-    int stream_id, const media::AudioParameters& params) {
+    int stream_id, int render_view_id, const media::AudioParameters& params) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  DVLOG(1) << "AudioRendererHost@" << this
+           << "::OnCreateStream(stream_id=" << stream_id
+           << ", render_view_id=" << render_view_id << ")";
+  DCHECK_LT(0, render_view_id);

[DaleCurtis, 2013/03/05 23:29:54]

Drive your Smart Car elsewhere. http://what-if.xkcd.com/3/

+
   // media::AudioParameters is validated in the deserializer.
   int input_channels = params.input_channels();
   if (input_channels < 0 ||
       input_channels > media::limits::kMaxChannels ||
       LookupById(stream_id) != NULL) {
     SendErrorMessage(stream_id);
     return;
   }
 
   media::AudioParameters audio_params(params);
 
   // Calculate output and input memory size.
   int output_memory_size = AudioBus::CalculateMemorySize(audio_params);

[palmer, 2013/03/05 21:09:32]

This should be an unsigned type (size_t or uint32).

[miu, 2013/03/06 22:36:52]

I agree with you.  But, the style guide generally bans use of the unsigned
types.  This could be an exception case (use of size_t here?).  If you think
this should be changed, please add a note to http://crbug.com/120319.

[palmer, 2013/03/06 23:11:59]

Yeah, this is an area where the style guide is simply wrong, unfortunately.
Well, partly wrong. "int" means "integer" and the use of unsigned integers for
integers whose value you never expect to be < 0 is arguable.

But in any case, "size_t" means "this is a size or offset", which is clearly
correct; and in any case, integer overflow behavior is undefined for signed
integers but it is defined for unsigned integer types (all, not just size_t). So
for security and correctness reasons, where integer overflow can happen, an
unsigned type (usually size_t) is correct and the style guide's dictum does not
apply in those cases.

I'll comment on that bug, thanks for the pointer.

 
   int frames = audio_params.frames_per_buffer();
   int input_memory_size =

[palmer, 2013/03/05 21:09:32]

This should be an unsigned type (size_t or uint32).

       AudioBus::CalculateMemorySize(input_channels, frames);
 
   scoped_ptr<AudioEntry> entry(new AudioEntry());
 
   // Create the shared memory and share with the renderer process.
   // For synchronized I/O (if input_channels > 0) then we allocate
   // extra memory after the output data for the input data.
   uint32 io_buffer_size = output_memory_size + input_memory_size;

[palmer, 2013/03/05 21:09:32]

How do we know this addition won't overflow? |output_memory_size| and
|input_memory_size| should be constrained to sane values, preferably values that
when added cannot overflow a uint32. (Otherwise you have to manually check for
overflow).

[DaleCurtis, 2013/03/05 23:29:54]

Same AudioParameters verification ensures CalculateMemorySize is sane.

 
   uint32 shared_memory_size =
       media::TotalSharedMemorySizeInBytes(io_buffer_size);
   if (!entry->shared_memory.CreateAndMapAnonymous(shared_memory_size)) {
     // If creation of shared memory failed then send an error message.
     SendErrorMessage(stream_id);
     return;
   }
 
   // Create sync reader and try to initialize it.
   scoped_ptr<AudioSyncReader> reader(
       new AudioSyncReader(&entry->shared_memory, params, input_channels));
 
   if (!reader->Init()) {
     SendErrorMessage(stream_id);
     return;
   }
 
   // If we have successfully created the SyncReader then assign it to the
   // entry and construct an AudioOutputController.
   entry->reader.reset(reader.release());
   entry->controller = media::AudioOutputController::Create(
       audio_manager_, this, audio_params, entry->reader.get());
 
   if (!entry->controller) {
     SendErrorMessage(stream_id);
     return;
   }
 
-  // If we have created the controller successfully, create an entry and add it
+  // Since the controller was created successfully, create an entry and add it
   // to the map.
   entry->stream_id = stream_id;
+  entry->render_view_id = render_view_id;
+  if (mirroring_manager_) {
+    mirroring_manager_->AddDiverter(
+        render_process_id_, entry->render_view_id, entry->controller);
+  }
   audio_entries_.insert(std::make_pair(stream_id, entry.release()));
   if (media_internals_)
     media_internals_->OnSetAudioStreamStatus(this, stream_id, "created");
 }
 
-void AudioRendererHost::OnAssociateStreamWithProducer(int stream_id,
-                                                      int render_view_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
-  DVLOG(1) << "AudioRendererHost@" << this
-           << "::OnAssociateStreamWithProducer(stream_id=" << stream_id
-           << ", render_view_id=" << render_view_id << ")";
-
-  AudioEntry* const entry = LookupById(stream_id);
-  if (!entry) {
-    SendErrorMessage(stream_id);
-    return;
-  }
-
-  if (entry->render_view_id == render_view_id)
-    return;
-
-  // TODO(miu): Merge "AssociateWithProducer" message into "CreateStream"
-  // message so AudioRendererHost can assume a simpler "render_view_id is set
-  // once" scheme. http://crbug.com/166779
-  if (mirroring_manager_) {
-    mirroring_manager_->RemoveDiverter(
-        render_process_id_, entry->render_view_id, entry->controller);
-  }
-  entry->render_view_id = render_view_id;
-  if (mirroring_manager_) {
-    mirroring_manager_->AddDiverter(
-        render_process_id_, entry->render_view_id, entry->controller);
-  }
-}
-
 void AudioRendererHost::OnPlayStream(int stream_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   AudioEntry* entry = LookupById(stream_id);
   if (!entry) {
     SendErrorMessage(stream_id);
     return;
   }
 
   entry->controller->Play();
@@ skipping 165 matching lines @@
   return NULL;
 }
 
 media::AudioOutputController* AudioRendererHost::LookupControllerByIdForTesting(
     int stream_id) {
   AudioEntry* const entry = LookupById(stream_id);
   return entry ? entry->controller : NULL;
 }
 
 }  // namespace content