Clean up audio-related utility functions to compute buffer sizes

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 178 matching lines @@
 
   return handled;
 }
 
 void AudioRendererHost::OnCreateStream(
     int stream_id, const AudioParameters& params) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(LookupById(stream_id) == NULL);
 
   AudioParameters audio_params(params);
+  DCHECK_GT(audio_params.samples_per_packet, 0);

[scherkus (not reviewing), 2012/02/23 18:27:43]

does it make sense to move this DCHECK into AudioParameters ctor to avoid having
clients check for this parameter?

[Chris Rogers, 2012/02/23 19:00:21]

Is there any way to signal an error and return early here in release builds?

[vrk (LEFT CHROMIUM), 2012/02/23 22:33:18]

Hmmmmmm!

Well, so if the only change that was made was putting a DCHECK into
AudioParameters' constructor, then no, it wouldn't make sense. AudioParameters
is a struct whose values can change at any point, and we happen to set the
samples_per_packet field after it's constructed.

BUT after some brief investigation of the code, looks like this doesn't have to
be the case... I think we could make AudioParameters a class w/ private fields,
change some long parameter lists to take AudioParameters instead, etc...
separate CL. wdyt?

[vrk (LEFT CHROMIUM), 2012/02/23 22:33:18]

So, we could add
  if (audio_params.samples_per_packet == 0) {
    SendErrorMessage(stream_id);
    return;
  }

But I believe having audio_params.samples_per_packet == 0 would be a programmer
error, not an error that we expect and should handle safely. (For this same
reason, we don't signal an error and return if LookupById(stream_id) != NULL,
for example.)

If you're concerned because we're not 100% confident in changing this to a
DCHECK, maybe we could temporarily change this to a CHECK for a few days and see
if there are crashes in the canaries?

[scherkus (not reviewing), 2012/02/23 23:49:09]

Sounds like something worth investigating post-commit!

For example you could go the Audio/VideoDecoderConfig route where they're
(mostly) immutable objects that are passed around const-ref

 
-  // Select the hardware packet size if not specified.
-  if (!audio_params.samples_per_packet) {
-    audio_params.samples_per_packet =
-        media::SelectSamplesPerPacket(audio_params.sample_rate);
-  }
   uint32 packet_size = audio_params.GetPacketSize();
 
   scoped_ptr<AudioEntry> entry(new AudioEntry());
 
   // Create the shared memory and share with the renderer process.
   uint32 shared_memory_size =
       media::TotalSharedMemorySizeInBytes(packet_size);
   if (!entry->shared_memory.CreateAndMapAnonymous(shared_memory_size)) {
     // If creation of shared memory failed then send an error message.
     SendErrorMessage(stream_id);
@@ skipping 171 matching lines @@
   }
   return NULL;
 }
 
 MediaObserver* AudioRendererHost::media_observer() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!media_observer_)
     media_observer_ = resource_context_->GetMediaObserver();
   return media_observer_;
 }