Introduce VideoConfig/AudioConfig class for CMA backend

chromecast/public/media/decoder_config.h

Index: chromecast/public/media/decoder_config.h
diff --git a/chromecast/public/media/decoder_config.h b/chromecast/public/media/decoder_config.h
new file mode 100644
index 0000000000000000000000000000000000000000..1e0998fe69da42a7735854984c63050f08c5925b
--- /dev/null
+++ b/chromecast/public/media/decoder_config.h
@@ -0,0 +1,146 @@
+// Copyright 2015 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 CHROMECAST_PUBLIC_MEDIA_DECODER_CONFIG_H_
+#define CHROMECAST_PUBLIC_MEDIA_DECODER_CONFIG_H_
+
+#include <stdint.h>
+#include <vector>
+
+namespace chromecast {
+namespace media {
+
+namespace {
+
+// Maximum audio bytes per sample.
+static const int kMaxBytesPerSample = 4;
+
+// Maximum audio sampling rate.
+static const int kMaxSampleRate = 192000;
+
+}  // namespace
+
+enum AudioCodec {
+  kAudioCodecUnknown = -1,
+
+  kAudioCodecMin = 0,
+  kCodecAAC = kAudioCodecMin,
+  kCodecMP3,
+  kCodecPCM,
+  kCodecPCM_S16BE,
+  kCodecVorbis,
+  kCodecOpus,
+  kCodecEAC3,
+  kCodecAC3,
+  kCodecDTS,
+  kAudioCodecMax = kCodecDTS,
+};
+
+enum VideoCodec {
+  kVideoCodecUnknown = -1,
+
+  kVideoCodecMin = 0,
+  kCodecH264 = kVideoCodecMin,
+  kCodecVC1,
+  kCodecMPEG2,
+  kCodecMPEG4,
+  kCodecTheora,
+  kCodecVP8,
+  kCodecVP9,
+  kCodecHEVC,
+  kVideoCodecMax = kCodecHEVC,
+};
+
+// Profile for Video codec.
+enum VideoProfile {
+  kVideoProfileUnknown = -1,
+
+  kVideoProfileMin = 0,
+  kH264Baseline = kVideoProfileMin,
+  kH264Main = 1,
+  kH264Extended = 2,
+  kH264High = 3,
+  kH264High10 = 4,
+  kH264High422 = 5,
+  kH264High444Predictive = 6,
+  kH264ScalableBaseline = 7,
+  kH264ScalableHigh = 8,
+  kH264Stereohigh = 9,
+  kH264MultiviewHigh = 10,
+  kVP8ProfileAny = 11,
+  kVP9ProfileAny = 12,
+  kVideoProfileMax = kVP9ProfileAny,
+};
+
+struct AudioConfig {
+  AudioConfig()
+    : codec(kAudioCodecUnknown),
+      bytes_per_channel(0),
+      channel_number(0),
+      samples_per_second(0),
+      extra_data(nullptr),
+      extra_data_size(0),
+      is_encrypted(false) {}

[gunsch, 2015/05/05 16:54:22]

Hmm, do we need these ctors? Would prefer not to define any methods if possible
(which I think resolves the Clang style plugin issues you were describing last
night). Can we use default initializer?

[erickung1, 2015/05/05 17:23:45]

please see my next comment. I think we should, however, the current OEM
implementation relies on IsValidConfig and tends to reset the structure by
creating a new object(which happens in Chromium code everywhere). With ctors and
IsValidConfig(), we don't have to change a lot in OEM implementation

+
+  // Audio codec.
+  AudioCodec codec;
+  // Number of bytes in each channel.
+  int bytes_per_channel;
+  // Number of channels in this audio stream.
+  int channel_number;
+  // Number of audio samples per second.
+  int samples_per_second;
+  // Pointer to extra data buffer for certain codec initialization. Note the
+  // structure only passes the pointer of data buffer and does not own memory
+  // responsibility.
+  const uint8_t* extra_data;
+  // Size of extra data in bytes.

[gunsch, 2015/05/06 02:52:35]

Whoa, I just noticed this change... why did we change from std::vector<uint8_t>
to uint8_t* + size? That seems like a source of easy mistakes when OEMs are
implementing.

[erickung1, 2015/05/06 03:01:33]

using std:vector will require constructor/destructor implementation in .cc
file(Chromium C++ style guild). Although we can consider disabling this flag for
cast_public_api gyp, allocating memory(memory copy) is not the purposes for the
passive object

The reason AudioDecoderConfig/VideoDecoderConfig do that is only because it the
data won't be huge, using std::vector (memory copy) should be fine. But that's
different consideration, from my point of view

[gunsch, 2015/05/06 03:08:44]

I'm probably going to insist. The OEM implementation is most likely going to
have to memory-copy the std::vector *anyways* (as in your implementation).
Assuming that's the case:

Arguments against uint8*: our API now passes in a pointer that quickly becomes
invalid. If OEM implementation holds onto copies of the AudioConfig/VideoConfig
objects (reasonable), they're left with a dangling pointer.

Arguments against std::vector: Chromium style guide says we need ctor/dtor
implementation in .cc file.

To me, that's an easy choice: we should be preferring a better API.

+  int extra_data_size;
+  // content is encrypted or not.
+  bool is_encrypted;
+};
+
+struct VideoConfig {
+  VideoConfig()
+    : codec(kVideoCodecUnknown),
+      profile(kVideoProfileUnknown),
+      additional_config(nullptr),
+      extra_data(nullptr),
+      extra_data_size(0),
+      is_encrypted(false) {}
+
+  // Video codec.
+  VideoCodec codec;
+  // Video codec profile.
+  VideoProfile profile;
+  // Additional video config for the video stream if available.
+  VideoConfig* additional_config;
+  // Pointer to extra data buffer for certain codec initialization. Note the
+  // structure only passes the pointer of data buffer and does not own memory
+  // responsibility.
+  const uint8_t* extra_data;
+  // Size of extra data in bytes.
+  int extra_data_size;
+  // content is encrypted or not.
+  bool is_encrypted;
+};
+
+inline bool IsValidConfig(const AudioConfig& config) {

[gunsch, 2015/05/05 16:54:22]

Per Le-Chun's earlier question, do we actually need this or can we simply verify
the config is valid before passing it to the OEM?

[erickung1, 2015/05/05 17:23:44]

Existing OEM implementaion tends do following
1. keep the latest config in their code for future reference
2. reset the config due to state change by creating a new one(making invalid)
3. use IsValidConfig before further actions because of #1, #2

I think it's better to have this function for now to make less change in OEM
implementation part, unless we want to make a guideline to OEM implementation
that it is OK to keep a copy but it should honor the code to do proper action
when receiving the config.

[gunsch, 2015/05/05 17:30:35]

I'm hearing arguments that it makes migration easier (SGTM), but not for it
being a good long-term solution. I'm okay with having this here under that logic
if we also include a TODO to remove it and a note to not add new consumers.

[erickung1, 2015/05/05 20:18:05]

Done.

+  return config.codec >= kAudioCodecMin &&
+      config.codec <= kAudioCodecMax &&
+      config.channel_number > 0 &&
+      config.bytes_per_channel > 0 &&
+      config.bytes_per_channel <= kMaxBytesPerSample &&
+      config.samples_per_second > 0 &&
+      config.samples_per_second <= kMaxSampleRate;
+}
+
+inline bool IsValidConfig(const VideoConfig& config) {
+  return config.codec >= kVideoCodecMin && config.codec <= kVideoCodecMax;
+}
+
+}  // namespace media
+}  // namespace chromecast
+
+#endif  // CHROMECAST_PUBLIC_MEDIA_DECODER_CONFIG_H_