Do not require a key before processing unencrypted frames within a potentially encrypted stream.

Do not require a key before processing unencrypted frames within a potentially encrypted stream.

BUG=168930
TEST=new unit tests


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176285

[ddorwin, 2013-01-09 20:33:17]

I need to commit new test file(s) and maybe another fix, but PTAL as I think the
current code won't change.

[xhwang, 2013-01-09 21:18:21]

https://codereview.chromium.org/11830024/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

https://codereview.chromium.org/11830024/diff/1/media/crypto/aes_decryptor.cc...
media/crypto/aes_decryptor.cc:243: } else {
move line 235 here?

https://codereview.chromium.org/11830024/diff/1/media/filters/pipeline_integr...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11830024/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:48: class AppBase {
Looks like AppBase isn't doing a lot of work and isn't expected to be called
directly. What do you think of making it a pure interface and move these
implementations to NoResponseApp?

[ddorwin, 2013-01-10 04:44:46]

https://codereview.chromium.org/11830024/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

https://codereview.chromium.org/11830024/diff/1/media/crypto/aes_decryptor.cc...
media/crypto/aes_decryptor.cc:243: } else {
On 2013/01/09 21:18:21, xhwang wrote:
> move line 235 here?

Done.

https://codereview.chromium.org/11830024/diff/1/media/filters/pipeline_integr...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/11830024/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:48: class AppBase {
On 2013/01/09 21:18:21, xhwang wrote:
> Looks like AppBase isn't doing a lot of work and isn't expected to be called
> directly. What do you think of making it a pure interface and move these
> implementations to NoResponseApp?

Done.
I left KeyError since that's likely to be common.

[xhwang, 2013-01-10 06:57:40]

lgtm

[commit-bot: I haz the power, 2013-01-11 03:09:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/11830024/8005

[commit-bot: I haz the power, 2013-01-11 03:09:46]

Failed to apply patch for media/filters/pipeline_integration_test.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/filters/pipeline_integration_test.cc
  Hunk #2 succeeded at 44 (offset -6 lines).
  Hunk #3 succeeded at 80 (offset -6 lines).
  Hunk #4 succeeded at 141 (offset -6 lines).
  Hunk #5 succeeded at 154 (offset -6 lines).
  Hunk #6 succeeded at 444 with fuzz 2 (offset -6 lines).
  Hunk #7 succeeded at 472 (offset -6 lines).
  Hunk #8 succeeded at 503 with fuzz 2 (offset -6 lines).
  Hunk #9 FAILED at 565.
  1 out of 9 hunks FAILED -- saving rejects to file
media/filters/pipeline_integration_test.cc.rej

Patch:       media/filters/pipeline_integration_test.cc
Index: media/filters/pipeline_integration_test.cc
diff --git a/media/filters/pipeline_integration_test.cc
b/media/filters/pipeline_integration_test.cc
index
8c18f7b0a14f441193aa2201fcf39b6b2fda9c79..31b2253752d1e32bd862c643517273d036c5e0ca
100644
--- a/media/filters/pipeline_integration_test.cc
+++ b/media/filters/pipeline_integration_test.cc
@@ -5,6 +5,7 @@
 #include "media/filters/pipeline_integration_test_base.h"
 
 #include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/string_util.h"
 #include "build/build_config.h"
 #include "media/base/decoder_buffer.h"
@@ -49,7 +50,35 @@ static const int k1280IsoFileDurationMs = 2736;
 // They do not exercise the Decrypting{Audio|Video}Decoder path.
 class FakeEncryptedMedia {
  public:
-  FakeEncryptedMedia()
+  // Defines the behavior of the "app" that responds to EME events.
+  class AppBase {
+   public:
+    virtual ~AppBase() {}
+
+    virtual void KeyAdded(const std::string& key_system,
+                          const std::string& session_id) = 0;
+
+    // Errors are not expected unless overridden.
+    virtual void KeyError(const std::string& key_system,
+                          const std::string& session_id,
+                          AesDecryptor::KeyError error_code,
+                          int system_code) {
+      FAIL() << "Unexpected Key Error";
+    }
+
+    virtual void KeyMessage(const std::string& key_system,
+                            const std::string& session_id,
+                            const std::string& message,
+                            const std::string& default_url) = 0;
+
+    virtual void NeedKey(const std::string& key_system,
+                         const std::string& session_id,
+                         const std::string& type,
+                         scoped_array<uint8> init_data, int init_data_length,
+                         AesDecryptor* decryptor) = 0;
+  };
+
+  FakeEncryptedMedia(AppBase* app)
       : decryptor_(base::Bind(&FakeEncryptedMedia::KeyAdded,
                               base::Unretained(this)),
                    base::Bind(&FakeEncryptedMedia::KeyError,
@@ -57,30 +86,59 @@ class FakeEncryptedMedia {
                    base::Bind(&FakeEncryptedMedia::KeyMessage,
                               base::Unretained(this)),
                    base::Bind(&FakeEncryptedMedia::NeedKey,
-                              base::Unretained(this))) {
+                              base::Unretained(this))),
+        app_(app) {
   }
 
   AesDecryptor* decryptor() {
     return &decryptor_;
   }
 
-  // Callbacks for firing key events.
+  // Callbacks for firing key events. Delegate to |app_|.
   void KeyAdded(const std::string& key_system, const std::string& session_id) {
-    EXPECT_EQ(kClearKeySystem, key_system);
-    EXPECT_FALSE(session_id.empty());
+    app_->KeyAdded(key_system, session_id);
   }
 
   void KeyError(const std::string& key_system,
                 const std::string& session_id,
                 AesDecryptor::KeyError error_code,
                 int system_code) {
-    FAIL() << "Unexpected Key Error";
+    app_->KeyError(key_system, session_id, error_code, system_code);
   }
 
   void KeyMessage(const std::string& key_system,
                   const std::string& session_id,
                   const std::string& message,
                   const std::string& default_url) {
+    app_->KeyMessage(key_system, session_id, message, default_url);
+  }
+
+  void NeedKey(const std::string& key_system,
+               const std::string& session_id,
+               const std::string& type,
+               scoped_array<uint8> init_data, int init_data_length) {
+    app_->NeedKey(key_system, session_id, type,
+                  init_data.Pass(), init_data_length, &decryptor_);
+  }
+
+ private:
+  AesDecryptor decryptor_;
+  scoped_ptr<AppBase> app_;
+};
+
+// Provides |kSecretKey| in response to needkey.
+class KeyProvidingApp : public FakeEncryptedMedia::AppBase {
+ public:
+  virtual void KeyAdded(const std::string& key_system,
+                        const std::string& session_id) OVERRIDE {
+    EXPECT_EQ(kClearKeySystem, key_system);
+    EXPECT_FALSE(session_id.empty());
+  }
+
+  virtual void KeyMessage(const std::string& key_system,
+                          const std::string& session_id,
+                          const std::string& message,
+                          const std::string& default_url) OVERRIDE {
     EXPECT_EQ(kClearKeySystem, key_system);
     EXPECT_FALSE(session_id.empty());
     EXPECT_FALSE(message.empty());
@@ -89,10 +147,11 @@ class FakeEncryptedMedia {
     current_session_id_ = session_id;
   }
 
-  void NeedKey(const std::string& key_system,
-               const std::string& session_id,
-               const std::string& type,
-               scoped_array<uint8> init_data, int init_data_length) {
+  virtual void NeedKey(const std::string& key_system,
+                       const std::string& session_id,
+                       const std::string& type,
+                       scoped_array<uint8> init_data, int init_data_length,
+                       AesDecryptor* decryptor) OVERRIDE {
     current_key_system_ = key_system;
     current_session_id_ = session_id;
 
@@ -101,22 +160,48 @@ class FakeEncryptedMedia {
     // session (which will call KeyMessage).
     if (current_key_system_.empty()) {
       EXPECT_TRUE(current_session_id_.empty());
-      EXPECT_TRUE(decryptor_.GenerateKeyRequest(
+      EXPECT_TRUE(decryptor->GenerateKeyRequest(
           kClearKeySystem, type, kInitData, arraysize(kInitData)));
     }
 
     EXPECT_FALSE(current_key_system_.empty());
     EXPECT_FALSE(current_session_id_.empty());
-    decryptor_.AddKey(current_key_system_, kSecretKey, arraysize(kSecretKey),
+    decryptor->AddKey(current_key_system_, kSecretKey, arraysize(kSecretKey),
                       init_data.get(), init_data_length, current_session_id_);
   }
 
- private:
-  AesDecryptor decryptor_;
   std::string current_key_system_;
   std::string current_session_id_;
 };
 
+// Ignores needkey and does not perform a license request
+class NoResponseApp : public FakeEncryptedMedia::AppBase {
+ public:
+  virtual void KeyAdded(const std::string& key_system,
+                        const std::string& session_id) OVERRIDE {
+    EXPECT_EQ(kClearKeySystem, key_system);
+    EXPECT_FALSE(session_id.empty());
+    FAIL() << "Unexpected KeyAdded";
+  }
+
+  virtual void KeyMessage(const std::string& key_system,
+                          const std::string& session_id,
+                          const std::string& message,
+                          const std::string& default_url) OVERRIDE {
+    EXPECT_EQ(kClearKeySystem, key_system);
+    EXPECT_FALSE(session_id.empty());
+    EXPECT_FALSE(message.empty());
+    FAIL() << "Unexpected KeyMessage";
+  }
+
+  virtual void NeedKey(const std::string& key_system,
+                       const std::string& session_id,
+                       const std::string& type,
+                       scoped_array<uint8> init_data, int init_data_length,
+                       AesDecryptor* decryptor) OVERRIDE {
+  }
+};
+
 // Helper class that emulates calls made on the ChunkDemuxer by the
 // Media Source API.
 class MockMediaSource {
@@ -365,7 +450,7 @@ TEST_F(PipelineIntegrationTest,
MediaSource_ConfigChange_WebM) {
 TEST_F(PipelineIntegrationTest, MediaSource_ConfigChange_Encrypted_WebM) {
   MockMediaSource source("bear-320x240-16x9-aspect-av_enc-av.webm", kWebM,
                          kAppendWholeFile);
-  FakeEncryptedMedia encrypted_media;
+  FakeEncryptedMedia encrypted_media(new KeyProvidingApp());
   StartPipelineWithEncryptedMedia(&source, &encrypted_media);
 
   scoped_refptr<DecoderBuffer> second_file =
@@ -393,7 +478,7 @@ TEST_F(PipelineIntegrationTest,
        MediaSource_ConfigChange_ClearThenEncrypted_WebM) {
   MockMediaSource source("bear-320x240-16x9-aspect.webm", kWebM,
                          kAppendWholeFile);
-  FakeEncryptedMedia encrypted_media;
+  FakeEncryptedMedia encrypted_media(new KeyProvidingApp());
   StartPipelineWithEncryptedMedia(&source, &encrypted_media);
 
   scoped_refptr<DecoderBuffer> second_file =
@@ -424,7 +509,7 @@ TEST_F(PipelineIntegrationTest,
        MediaSource_ConfigChange_EncryptedThenClear_WebM) {
   MockMediaSource source("bear-320x240-16x9-aspect-av_enc-av.webm", kWebM,
                          kAppendWholeFile);
-  FakeEncryptedMedia encrypted_media;
+  FakeEncryptedMedia encrypted_media(new KeyProvidingApp());
   StartPipelineWithEncryptedMedia(&source, &encrypted_media);
 
   scoped_refptr<DecoderBuffer> second_file =
@@ -480,9 +565,41 @@ TEST_F(PipelineIntegrationTest,
BasicPlayback_16x9AspectRatio) {
   ASSERT_TRUE(WaitUntilOnEnded());
 }
 
-TEST_F(PipelineIntegrationTest, EncryptedPlayback) {
+TEST_F(PipelineIntegrationTest, EncryptedPlayback_WebM) {
   MockMediaSource source("bear-320x240-av_enc-av.webm", kWebM, 219816);
-  FakeEncryptedMedia encrypted_media;
+  FakeEncryptedMedia encrypted_media(new KeyProvidingApp());
+  StartPipelineWithEncryptedMedia(&source, &encrypted_media);
+
+  source.EndOfStream();
+  ASSERT_EQ(PIPELINE_OK, pipeline_status_);
+
+  Play();
+
+  ASSERT_TRUE(WaitUntilOnEnded());
+  source.Abort();
…
(message too large)

[commit-bot: I haz the power, 2013-01-11 05:03:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/11830024/13001

[commit-bot: I haz the power, 2013-01-11 08:48:22]

Change committed as 176285