Make AudioParameters a class instead of a struct

media/audio/audio_parameters.h

Index: media/audio/audio_parameters.h
diff --git a/media/audio/audio_parameters.h b/media/audio/audio_parameters.h
index 244d8b9f0fb0cc3b7427ff939847d2b42a78ebb6..f19dc3c0298e24865593146e30db81ae49584259 100644
--- a/media/audio/audio_parameters.h
+++ b/media/audio/audio_parameters.h
@@ -9,9 +9,8 @@
 #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:
@@ -33,8 +32,12 @@ struct MEDIA_EXPORT AudioParameters {
   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.
@@ -46,14 +49,22 @@ struct MEDIA_EXPORT AudioParameters {
   // 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_