media config: expand is_encrypted to a struct.

media/base/encryption_scheme.h

+// 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 MEDIA_BASE_ENCRYPTION_SCHEME_H_
+#define MEDIA_BASE_ENCRYPTION_SCHEME_H_
+
+#include <stdint.h>
+
+#include "media/base/media_export.h"
+
+namespace media {
+
+// Specification of whether and how the stream is encrypted (in whole or part).
+class MEDIA_EXPORT EncryptionScheme {
+ public:
+  // Algorithm and mode used for encryption. Allows an indication that no

[ddorwin, 2016/03/03 18:38:04]

kCipherModeUnencrypted indicates that no... ?

[dougsteed, 2016/03/04 19:07:30]

Done.

+  // encryption has been applied.
+  enum CipherMode {
+    kCipherModeUnencrypted,

[xhwang, 2016/03/03 22:33:00]

In new code we should use UPPER_CASE for enums:

https://www.chromium.org/developers/coding-style#TOC-Naming

[dougsteed, 2016/03/04 19:07:30]

Prefer consistency with the other enums in media. Also we have already exposed
these names to our OEM partners, so would rather not change at this point.

[xhwang, 2016/03/04 20:00:48]

Those enums predate the requirement of using UPPER_CASE for enums in Chromium.
They are just not updated yet to meet the new requirement.

Also, it sounds a bit odd that media/base can't be changed because chromecast
OEM partners are using one of the interfaces...

[dougsteed, 2016/03/07 17:49:13]

I misspoke in my comment about OEMs. I was actually thinking of the similar
enums in chromecast/public/media. I completely agree that media/base should not
be influenced by that. It sounds like you are not concerned about the CipherMode
naming being inconsistent with the other enums in this same file (which was my
main concern) so I will go ahead and change.

+    kCipherModeAesCtr,
+    kCipherModeDefault = kCipherModeAesCtr,
+    kCipherModeAesCbc,
+    kCipherModeMax = kCipherModeAesCbc
+  };
+
+  // V3 of the CENC standard adds pattern encryption, through two new
+  // protection schemes 'cens' (with AES-CTR) and 'cbcs' (with AES-CBC).
+  // The pattern applies only to the 'encrypted' part of the frame (as

[xhwang, 2016/03/03 22:33:00]

Does the patter apply to all "encrypted" blocks (as defined by the subsample
entries) independently? I assume so, but it would be nicer to make this comment
more clear on that.

[dougsteed, 2016/03/04 19:07:30]

Done.

+  // defined by the relevant subsample entries), and reduces further the
+  // actual encryption applied through a repeating pattern of (encrypt:skip)
+  // 16 byte blocks. For example, in a (1:9) pattern, the first block is
+  // encrypted, and the next nine are skipped. This pattern is applied
+  // repeatedly until the end of the last 16-byte block in the subsample.
+  // Any remaining bytes are left clear.
+  // If either or both of encrypt_blocks or skip_blocks is 0, pattern

[xhwang, 2016/03/03 22:33:00]

nit: "or both of" is redundant

[dougsteed, 2016/03/04 19:07:29]

Done.

+  // encryption is disabled.
+  class PatternSpec {

[xhwang, 2016/03/03 22:33:00]

"Spec" is not in the standard and doesn't add any value here. Can we just call
it "Pattern"? You are calling the variable "pattern" already.

[dougsteed, 2016/03/04 19:07:30]

Done.

+   public:
+    PatternSpec();
+    PatternSpec(uint32_t encrypt_blocks, uint32_t skip_blocks);
+    ~PatternSpec();
+
+    bool Matches(const PatternSpec& other) const;
+
+    uint32_t encrypt_blocks() const { return encrypt_blocks_; }
+    uint32_t skip_blocks() const { return skip_blocks_; }
+
+    bool IsInEffect() const;
+
+   private:
+    uint32_t encrypt_blocks_;
+    uint32_t skip_blocks_;
+
+    // Allow copy and assignment.
+  };
+
+  EncryptionScheme();
+  explicit EncryptionScheme(CipherMode mode);
+  EncryptionScheme(CipherMode mode, const PatternSpec& pattern);
+  ~EncryptionScheme();
+
+  // Returns an instance of EncryptionScheme indicating no encryption.

[ddorwin, 2016/03/03 18:38:04]

This is good, but I meant to add this to the default constructor.

[dougsteed, 2016/03/04 19:07:30]

Done.

+  static EncryptionScheme Unencrypted();

[xhwang, 2016/03/03 22:33:00]

+ddorwin

We use EncryptionScheme::Unencrypted() a lot. Can we have a helper function with
a shorter name in media/base/media_util.cc to return an unencrypted scheme,
something like Unencrypted() or UnencrytpedScheme().

We already have EmptyExtraData() there.

Another choice is to just use EncryptionScheme(), which is okay but not as
readable.

We can also have a function AesCtrEncryptionScheme() to replace
EncryptionScheme(EncryptionScheme::kCipherModeAesCtr).

[dougsteed, 2016/03/04 19:07:30]

Done.

+
+  bool Matches(const EncryptionScheme& other) const;
+
+  bool is_encrypted() const;

[xhwang, 2016/03/03 22:33:00]

nit: you can inline this as well.

[dougsteed, 2016/03/04 19:07:30]

Done.

+  CipherMode mode() const { return mode_; }
+  const PatternSpec& pattern() const { return pattern_; }
+
+ private:
+  CipherMode mode_;

[xhwang, 2016/03/03 22:33:00]

You can specify default mode here. Personally I feel that's more clear
especially when there are multiple constructors.

See "Non-Static Class Member Initializers" in https://chromium-cpp.appspot.com/

Same for members in PatternSpec.

[dougsteed, 2016/03/04 19:07:30]

Done.

+  PatternSpec pattern_;
+
+  // Allow copy and assignment.
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_ENCRYPTION_SCHEME_H_