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>
+
+namespace media {
+
+// Algorithm and mode that was used to encrypt the stream.
+enum CipherMode {

[ddorwin, 2015/12/10 18:36:01]

Should this be a member of the class?

[dougsteed, 2015/12/14 21:19:01]

Done.

+  kCipherModeUnknown,
+  kCipherModeAesCtr,
+  kCipherModeAesCbc,
+  kCipherModeMax = kCipherModeAesCbc

[ddorwin, 2015/12/10 18:36:01]

Do we need Max?

[dougsteed, 2015/12/14 21:19:02]

Yes, needed for IPC ParamTraits.

+};
+
+// Specification of whether and how the stream is encrypted (in whole or part).
+class EncryptionScheme {
+ public:
+  // V3 of the CENC standard will add pattern encryption, through two new

[ddorwin, 2015/12/10 18:36:01]

s/will add/adds/ to avoid comment rot?

[dougsteed, 2015/12/14 21:19:01]

Done.

+  // protection schemes 'cens' (with AES-CTR) and 'cbcs' (with AES-CBC).
+  // The pattern applies only to the 'encrypted' part of the frame (as
+  // 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

[ddorwin, 2015/12/10 18:36:01]

Where does this apply? Is this a rule for this class? Instead can we just
disallow passing 0 to the non-default constructor?

Better yet, can we use static CreateNoPattern() and CreatePattern(e, s)
functions? (See discussion below.)

[dougsteed, 2015/12/14 21:19:01]

I am trying to model the concept of CENCv3, where pattern encryption is in
effect if both values are non-zero, and not in effect otherwise. So my
interpretation is that either or both being zero is permitted by the class, and
has the meaning of rendering pattern encryption not in effect. There is also a
similar class in chromecast, and it is much more convenient to translate the
objects back and forward by just copying the two values.

+  // encryption is disabled.
+  class PatternSpec {
+   public:
+    PatternSpec();
+    PatternSpec(uint32_t encrypt_blocks, uint32_t skip_blocks);
+    ~PatternSpec() {}

[ddorwin, 2015/12/10 18:36:01]

Do not inline.

[dougsteed, 2015/12/14 21:19:01]

Done.

+
+    void Initialize(uint32_t encrypt_blocks, uint32_t skip_blocks);

[ddorwin, 2015/12/10 18:36:01]

Why do we need this and the matching constructor?

[dougsteed, 2015/12/14 21:19:01]

Done.

+    bool Matches(const PatternSpec& other) const;
+
+    uint32_t encrypt_blocks() const { return encrypt_blocks_; }

[ddorwin, 2015/12/10 18:36:01]

DCHECK in_effect()? Will need to move to .cc file.

[dougsteed, 2015/12/14 21:19:01]

No. As mentioned above I still want to access the individual fields even when
in_effect is false.

+    uint32_t skip_blocks() const { return skip_blocks_; }
+
+    bool in_effect() const { return encrypt_blocks_ != 0 || skip_blocks_ != 0; }

[ddorwin, 2015/12/10 18:36:01]

It's unclear whether this is a "simple accessor"
(https://www.chromium.org/developers/coding-style#TOC-Inline-functions) or
should be in the .cc file. If not, it would also be isInEffect().

In any case, it should use "is" or something similar. Perhaps hasPattern() or
usesPattern().

[dougsteed, 2015/12/14 21:19:01]

Done.

+
+   private:
+    uint32_t encrypt_blocks_;

[ddorwin, 2015/12/10 18:36:01]

Can these be const?
(I guess not as long as we have Initialize(), but do we need that?)

[dougsteed, 2015/12/14 21:19:01]

No, because want to have copy and assign. Latter requires them not to be const.

+    uint32_t skip_blocks_;
+  };
+
+  EncryptionScheme();

[ddorwin, 2015/12/10 18:36:01]

As above, these constructors would be better as CreateFoo functions. This seems
especially true after looking at the implementations in the .cc file as since
the object that is created is unclear from some of these signatures.

Of course, that would require copying the object or returning a pointer. Copying
might not be too bad since the objects are so small.

At a minimum, we should reduce the number of different constructors so it is
clear what is happening when this object is created.

Also, add comments documenting what the remaining ones do. For example, this one
creates a "not encrypted" scheme.

[dougsteed, 2015/12/14 21:19:01]

Reduced the number of constructors. Hopefully it is clear now.

+  EncryptionScheme(bool is_encrypted);

[ddorwin, 2015/12/10 18:36:01]

explicit

[ddorwin, 2015/12/10 18:36:01]

Do we need this one? Wouldn't the ones above and below it suffice?

[dougsteed, 2015/12/14 21:19:01]

Ouch. This caused a lot of extra files to need to be touched!

+  explicit EncryptionScheme(CipherMode mode);
+  EncryptionScheme(CipherMode mode, const PatternSpec& pattern);
+  EncryptionScheme(bool is_encrypted,

[ddorwin, 2015/12/10 18:36:01]

Why do we need this one? Why would you ever pass false with a mode? See also
line 72.

[dougsteed, 2015/12/14 21:19:01]

Done.

+                   CipherMode mode,
+                   const PatternSpec& pattern);
+  ~EncryptionScheme() {}

[ddorwin, 2015/12/10 18:36:01]

Do not inline.

[dougsteed, 2015/12/14 21:19:01]

Done.

+
+  void Initialize(bool is_encrypted,
+                  CipherMode mode,
+                  const PatternSpec& pattern);
+  bool Matches(const EncryptionScheme& other) const;
+
+  bool is_encrypted() const { return is_encrypted_; }
+  CipherMode mode() const { return mode_; }
+  const PatternSpec& pattern() const { return pattern_; }
+
+ private:

[ddorwin, 2015/12/10 18:36:01]

Same question about const members.

[dougsteed, 2015/12/14 21:19:01]

Same answer about copy and assign.

+  bool is_encrypted_;

[ddorwin, 2015/12/10 18:36:01]

Do we need this? We could rename kCipherModeUnknown to kCipherModeNone or
kCipherModeClear and just use that? (This might also help minimize the number of
constructors.)

[xhwang, 2015/12/10 20:03:38]

It's a bit odd that you have an EncryptionScheme with |is_encrypted| being false
for an unencrypted stream. In {Audio|Video}DecoderConfig, can we actually just
hold a scoped_ptr<EncryptionScheme> so that when it's null the config is NOT
encrypted? That way, we can drop |is_encrypted| here in this class. Also, we can
disallow copy and assign. Another benefit is that in cases where the config is
not encrypted, you can simply use a nullptr for EncryptionScheme.

[dougsteed, 2015/12/14 21:19:01]

Done.

[dougsteed, 2015/12/14 21:19:01]

I tried this and it doesn't work (at least not without a bunch of extra
refactoring) because {Audio|Video}DecoderConfig have automatically-provided copy
operators, and these are not provided for scopers. 

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