Clean up audio-related utility functions to compute buffer sizes

content/browser/renderer_host/media/audio_renderer_host.cc

Index: content/browser/renderer_host/media/audio_renderer_host.cc
diff --git a/content/browser/renderer_host/media/audio_renderer_host.cc b/content/browser/renderer_host/media/audio_renderer_host.cc
index ac5051ef1a9dae0216a0762ab116d033f0daca2b..8afd9451b8e6125fb30be403f2ea07812e279576 100644
--- a/content/browser/renderer_host/media/audio_renderer_host.cc
+++ b/content/browser/renderer_host/media/audio_renderer_host.cc
@@ -196,12 +196,8 @@ void AudioRendererHost::OnCreateStream(
   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());