[Chromecast] Make audio/video config easier to use correctly

Previously AudioConfig and VideoConfig contained a raw pointer
to extra_data; if the config structure was copied, the pointed-to
data needed to be copied. If it was not copied, then it would cause
use-after-free / bad pointer bugs.

By making extra_data a vector, consumers of audio/video config can
simply copy the struct as desired, simplifying the code and making it
less error-prone.

BUG= internal b/25003220

Review URL: https://codereview.chromium.org/1422763002

Cr-Commit-Position: refs/heads/master@{#355633}

2 files changed, 39 insertions, 47 deletions

diff --git a/chromecast/media/cma/base/decoder_config_adapter.cc b/chromecast/media/cma/base/decoder_config_adapter.cc
index 248edd0..f2caba8 100644
--- a/chromecast/media/cma/base/decoder_config_adapter.cc
+++ b/chromecast/media/cma/base/decoder_config_adapter.cc
@@ -170,9 +170,8 @@ AudioConfig DecoderConfigAdapter::ToCastAudioConfig(
     StreamId id,
     const ::media::AudioDecoderConfig& config) {
   AudioConfig audio_config;
-  if (!config.IsValidConfig()) {
+  if (!config.IsValidConfig())
     return audio_config;
-  }
 
   audio_config.id = id;
   audio_config.codec = ToAudioCodec(config.codec());
@@ -181,9 +180,7 @@ AudioConfig DecoderConfigAdapter::ToCastAudioConfig(
   audio_config.channel_number =
       ::media::ChannelLayoutToChannelCount(config.channel_layout()),
   audio_config.samples_per_second = config.samples_per_second();
-  audio_config.extra_data =
-      (config.extra_data().empty()) ? nullptr : &config.extra_data()[0];
-  audio_config.extra_data_size = config.extra_data().size();
+  audio_config.extra_data = config.extra_data();
   audio_config.is_encrypted = config.is_encrypted();
   return audio_config;
 }
@@ -191,16 +188,11 @@ AudioConfig DecoderConfigAdapter::ToCastAudioConfig(
 // static
 ::media::AudioDecoderConfig DecoderConfigAdapter::ToMediaAudioDecoderConfig(
     const AudioConfig& config) {
-  std::vector<uint8_t> extra_data;
-  if (config.extra_data_size > 0)
-    extra_data.assign(config.extra_data,
-                      config.extra_data + config.extra_data_size);
-
   return ::media::AudioDecoderConfig(
       ToMediaAudioCodec(config.codec),
       ToMediaSampleFormat(config.sample_format),
       ToMediaChannelLayout(config.channel_number), config.samples_per_second,
-      extra_data, config.is_encrypted);
+      config.extra_data, config.is_encrypted);
 }
 
 // static
@@ -215,9 +207,7 @@ VideoConfig DecoderConfigAdapter::ToCastVideoConfig(
   video_config.id = id;
   video_config.codec = ToVideoCodec(config.codec());
   video_config.profile = ToVideoProfile(config.profile());
-  video_config.extra_data = (config.extra_data().empty()) ?
-      nullptr : &config.extra_data()[0];
-  video_config.extra_data_size = config.extra_data().size();
+  video_config.extra_data = config.extra_data();
   video_config.is_encrypted = config.is_encrypted();
   return video_config;
 }
diff --git a/chromecast/public/media/decoder_config.h b/chromecast/public/media/decoder_config.h
index 7652c3c..01a977c 100644
--- a/chromecast/public/media/decoder_config.h
+++ b/chromecast/public/media/decoder_config.h
@@ -89,16 +89,8 @@ enum VideoProfile {
 // create a new object to reset the configuration and use IsValidConfig() to
 // determine if the configuration is still valid or not.
 struct AudioConfig {
-  AudioConfig()
-      : id(kPrimary),
-        codec(kAudioCodecUnknown),
-        sample_format(kUnknownSampleFormat),
-        bytes_per_channel(0),
-        channel_number(0),
-        samples_per_second(0),
-        extra_data(nullptr),
-        extra_data_size(0),
-        is_encrypted(false) {}
+  AudioConfig();
+  ~AudioConfig();
 
   // Stream id.
   StreamId id;
@@ -112,28 +104,31 @@ struct AudioConfig {
   int channel_number;
   // Number of audio samples per second.
   int samples_per_second;
-  // Pointer to extra data buffer for certain codec initialization. The memory
-  // is allocated outside this structure. Consumers of the structure should make
-  // a copy if it is expected to be used beyond the function ends.
-  const uint8_t* extra_data;
-  // Size of extra data in bytes.
-  int extra_data_size;
+  // Extra data buffer for certain codec initialization.
+  std::vector<uint8_t> extra_data;
   // content is encrypted or not.
   bool is_encrypted;
 };
 
+inline AudioConfig::AudioConfig()
+    : id(kPrimary),
+      codec(kAudioCodecUnknown),
+      sample_format(kUnknownSampleFormat),
+      bytes_per_channel(0),
+      channel_number(0),
+      samples_per_second(0),
+      is_encrypted(false) {
+}
+
+inline AudioConfig::~AudioConfig() {
+}
+
 // TODO(erickung): Remove constructor once CMA backend implementation does't
 // create a new object to reset the configuration and use IsValidConfig() to
 // determine if the configuration is still valid or not.
 struct VideoConfig {
-  VideoConfig()
-    : id(kPrimary),
-      codec(kVideoCodecUnknown),
-      profile(kVideoProfileUnknown),
-      additional_config(nullptr),
-      extra_data(nullptr),
-      extra_data_size(0),
-      is_encrypted(false) {}
+  VideoConfig();
+  ~VideoConfig();
 
   // Stream Id.
   StreamId id;
@@ -141,20 +136,27 @@ struct VideoConfig {
   VideoCodec codec;
   // Video codec profile.
   VideoProfile profile;
-  // Both |additional_config| and |extra_data| are the pointers to the object
-  // memory that are allocated outside this structure. Consumers of the
-  // structure should make a copy if it is expected to be used beyond the
-  // function ends.
-  // Additional video config for the video stream if available.
+  // Additional video config for the video stream if available. Consumers of
+  // this structure should make an explicit copy of |additional_config| if it
+  // will be used after SetConfig() finishes.
   VideoConfig* additional_config;
-  // Pointer to extra data buffer for certain codec initialization.
-  const uint8_t* extra_data;
-  // Size of extra data in bytes.
-  int extra_data_size;
+  // Extra data buffer for certain codec initialization.
+  std::vector<uint8_t> extra_data;
   // content is encrypted or not.
   bool is_encrypted;
 };
 
+inline VideoConfig::VideoConfig()
+    : id(kPrimary),
+      codec(kVideoCodecUnknown),
+      profile(kVideoProfileUnknown),
+      additional_config(nullptr),
+      is_encrypted(false) {
+}
+
+inline VideoConfig::~VideoConfig() {
+}
+
 // TODO(erickung): Remove following two inline IsValidConfig() functions. These
 // are to keep existing CMA backend implementation consistent until the clean up
 // is done. These SHOULD NOT be used in New CMA backend implementation.