Make AudioParameters a class instead of a struct

media/audio/audio_parameters.h

 // 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.
 
 #ifndef MEDIA_AUDIO_AUDIO_PARAMETERS_H_
 #define MEDIA_AUDIO_AUDIO_PARAMETERS_H_
 
 #include "base/basictypes.h"
 #include "media/base/channel_layout.h"
 #include "media/base/media_export.h"
 
-// TODO(vrk): This should probably be changed to an immutable object instead of
-// a struct. See crbug.com/115902.
-struct MEDIA_EXPORT AudioParameters {
+class MEDIA_EXPORT AudioParameters {
+ public:
   // Compare is useful when AudioParameters is used as a key in std::map.
   class MEDIA_EXPORT Compare {
    public:
     bool operator()(const AudioParameters& a, const AudioParameters& b) const;
   };
 
   enum Format {
     AUDIO_PCM_LINEAR = 0,     // PCM is 'raw' amplitude samples.
     AUDIO_PCM_LOW_LATENCY,    // Linear PCM, low latency requested.
     AUDIO_MOCK,               // Creates a dummy AudioOutputStream object.
     AUDIO_LAST_FORMAT         // Only used for validation of format.y
   };
 
   // Telephone quality sample rate, mostly for speech-only audio.
   static const uint32 kTelephoneSampleRate = 8000;
   // CD sampling rate is 44.1 KHz or conveniently 2x2x3x3x5x5x7x7.
   static const uint32 kAudioCDSampleRate = 44100;
   // Digital Audio Tape sample rate.
   static const uint32 kAudioDATSampleRate = 48000;
 
   AudioParameters();
-  AudioParameters(Format format, ChannelLayout channel_layout, int sample_rate,
-                  int bits_per_sample, int samples_per_packet);
+  AudioParameters(Format format, ChannelLayout channel_layout,
+                  int samples_per_second, int bits_per_sample,

[Chris Rogers, 2012/03/12 20:07:13]

It's incorrect to change the name "sample_rate" to "samples_per_second" because
what you really mean is "sample_frames_per_second".  For interleaved audio
streams (such as we use):

samples_per_second = channels * sample_frames_per_second;

I would stick with the original name "sample_rate", or use
"sample_frames_per_second" (but in my opinion "sample_frames_per_second" is a
lot clunkier sounding)

[vrk (LEFT CHROMIUM), 2012/03/16 18:30:41]

sample_rate it is, as per offline discussion!

+                  int samples_per_packet);
+  void Reset(Format format, ChannelLayout channel_layout,
+             int samples_per_second, int bits_per_sample,
+             int samples_per_packet);
 
   // Checks that all values are in the expected range. All limits are specified
   // in media::Limits.
   bool IsValid() const;
 
   // Returns size of audio packets in bytes.
   int GetPacketSize() const;
 
   // Returns the number of bytes representing one second of audio.
   int GetBytesPerSecond() const;
 
-  Format format;                 // Format of the stream.
-  ChannelLayout channel_layout;  // Order of surround sound channels.
-  int sample_rate;               // Sampling frequency/rate.
-  int bits_per_sample;           // Number of bits per sample.
-  int samples_per_packet;        // Size of a packet in frames.
+  Format format() const { return format_; }

[tommi (sloooow) - chröme, 2012/03/10 10:11:32]

Would it be worth it to have a DCHECK(IsValid()) in some/all of these?

[vrk (LEFT CHROMIUM), 2012/03/16 18:30:41]

It's a good idea that we should be doing checks within the object to make sure
it's valid. However, instead of checking whether each member is valid on each
access, it would probably make more sense to DCHECK IsValid upon construction,
similar to how ValidateConfigs() works in AudioRendererAlgorithmBase upon
initialization. I'd like to do this in a follow-up CL, though, since I have a
feeling this may be a non-trivial change!

[tommi (sloooow) - chröme, 2012/03/16 20:57:39]

sounds good.

+  ChannelLayout channel_layout() const { return channel_layout_; }
+  int samples_per_second() const { return samples_per_second_; }

[tommi (sloooow) - chröme, 2012/03/10 10:11:32]

absolute nit: sample_rate()?
I realize you'd have to change this in multiple places so unless someone else
agrees, it's up to you.

[vrk (LEFT CHROMIUM), 2012/03/16 18:30:41]

sample_rate it is, as per offline discussion!

Aside: samples_per_second is used in a *lot* of the audio-tag specific code.
I'll fix that in a separate CL...

+  int bits_per_sample() const { return bits_per_sample_; }
+  int samples_per_packet() const { return samples_per_packet_; }
+  int channels() const { return channels_; }
 
-  int channels;                  // Number of channels. Value set based on
-                                 // |channel_layout|.
+ private:
+  Format format_;                 // Format of the stream.
+  ChannelLayout channel_layout_;  // Order of surround sound channels.
+  int samples_per_second_;        // Sampling frequency/rate.
+  int bits_per_sample_;           // Number of bits per sample.
+  int samples_per_packet_;        // Size of a packet in frames.
+
+  int channels_;                  // Number of channels. Value set based on
+                                  // |channel_layout|.
 };

[scherkus (not reviewing), 2012/03/09 21:48:59]

FYI now that this is a class I want to double check -- do we still want this to
be a copyable object?

(it's OK if it is as it's a POD)

[vrk (LEFT CHROMIUM), 2012/03/16 18:30:41]

Yup! Thanks for checking!

 
 #endif  // MEDIA_AUDIO_AUDIO_PARAMETERS_H_