EME: Correctly handle container-only contentType strings

third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp

 // Copyright 2014 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.
 
 #include "modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h"
 
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "bindings/core/v8/ScriptState.h"
 #include "core/dom/DOMException.h"
 #include "core/dom/Document.h"
@@ skipping 93 matching lines @@
     }
 
 private:
     // For widevine key system, generate warning and report to UMA if
     // |m_supportedConfigurations| contains any video capability with empty
     // robustness string.
     // TODO(xhwang): Remove after we handle empty robustness correctly.
     // See http://crbug.com/482277
     void checkVideoCapabilityRobustness() const;
 
+    // Generate warning and report to UMA if |config| contains a container-only

[xhwang, 2016/04/25 22:14:08]

Can't find |config| in the declaration :)

[ddorwin, 2016/04/25 22:58:52]

nit: I think it's clearer with "to" removed.

[jrummell, 2016/04/26 21:23:56]

Done.

[jrummell, 2016/04/26 21:23:56]

presubmit checks made me remove |config| as the parameter name. Fixed.

+    // contentType string.
+    // TODO(jrummell): Remove once this is no longer allowed.
+    // See http://crbug.com/605661.
+    void checkNoContainerOnlyContentTypeStrings(const WebMediaKeySystemConfiguration&);

[xhwang, 2016/04/25 22:14:08]

The function name is a bit hard to read.. How about checkEmptyCodecs()?

[jrummell, 2016/04/26 21:23:56]

Done.

+
     Member<ScriptPromiseResolver> m_resolver;
     const String m_keySystem;
     WebVector<WebMediaKeySystemConfiguration> m_supportedConfigurations;
 };
 
 MediaKeySystemAccessInitializer::MediaKeySystemAccessInitializer(ScriptState* scriptState, const String& keySystem, const HeapVector<MediaKeySystemConfiguration>& supportedConfigurations)
     : m_resolver(ScriptPromiseResolver::create(scriptState))
     , m_keySystem(keySystem)
     , m_supportedConfigurations(supportedConfigurations.size())
 {
@@ skipping 23 matching lines @@
         // If |label| is not present, it will be a null string.
         webConfig.label = config.label();
         m_supportedConfigurations[i] = webConfig;
     }
 
     checkVideoCapabilityRobustness();
 }
 
 void MediaKeySystemAccessInitializer::requestSucceeded(WebContentDecryptionModuleAccess* access)
 {
+    checkNoContainerOnlyContentTypeStrings(access->getConfiguration());
+
     m_resolver->resolve(new MediaKeySystemAccess(m_keySystem, adoptPtr(access)));
     m_resolver.clear();
 }
 
 void MediaKeySystemAccessInitializer::requestNotSupported(const WebString& errorMessage)
 {
     m_resolver->reject(DOMException::create(NotSupportedError, errorMessage));
     m_resolver.clear();
 }
 
@@ skipping 28 matching lines @@
         emptyRobustnessHistogram.count(hasEmptyRobustness);
     }
 
     if (hasEmptyRobustness) {
         m_resolver->getExecutionContext()->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel,
             "It is recommended that a robustness level be specified. Not specifying the robustness level could "
             "result in unexpected behavior in the future, potentially including failure to play."));
     }
 }
 
+void MediaKeySystemAccessInitializer::checkNoContainerOnlyContentTypeStrings(const WebMediaKeySystemConfiguration& config)
+{
+    // This is only checking for empty codecs in the selected configuration,
+    // as users may pass container only contentType strings for compatibility

[ddorwin, 2016/04/25 22:58:51]

s/users/apps/

[jrummell, 2016/04/26 21:23:56]

Done.

+    // with other systems.

[ddorwin, 2016/04/25 22:58:51]

s/systems/implementations/

[jrummell, 2016/04/26 21:23:56]

Done.

+    bool emptyAudioCodecs = false;

[ddorwin, 2016/04/25 22:58:51]

How about isAudioCodecsEmpty?

[jrummell, 2016/04/26 21:23:56]

Done.

+    if (!config.audioCapabilities.isEmpty()) {

[xhwang, 2016/04/25 22:14:08]

The logic here is good.

Just to be clear, this check covers both the case where audioCapabilities
doesn't exist, and audioCapabilities exists but is empty:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[jrummell, 2016/04/26 21:23:56]

Added additional check to make it clearer.

+        for (const auto& audio : config.audioCapabilities) {
+            if (audio.codecs.isEmpty()) {
+                emptyAudioCodecs = true;

[ddorwin, 2016/04/25 22:58:51]

This will currently trigger if there is capabilities with codecs followed by one
without codecs. This seems like it might be a useful mechanism for compatibility
with other implementations^ without having to duplicate the entire
configuration.

Should we not warn in that case? Or maybe we should have Chromium reject
codec-less strings once a one supported item has been found. Then, we could
leave this code as is.


^ This does not work for Edge
(https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7223299/),
and I'm not sure what other user agents there might be an issue with. Perhaps
this isn't a useful pattern. Still, the proposal above seems reasonable.

[jrummell, 2016/04/26 21:23:56]

Changed to only track if all returned capabilities don't supply codecs=. Thus
configurations that have both accepted won't get a message, since they will
continue to succeed once the code enforces the check.

+                break;
+            }
+        }
+
+        DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, emptyAudioCodecsHistogram,
+            new EnumerationHistogram("Media.EME.RequestMediaKeySystemAccess.HasEmptyAudioCodecs", 2));

[ddorwin, 2016/04/25 22:58:51]

We might want to rename this to something like
SelectedAudioContentTypeWithoutCodecs based on how the implementation changes.

[jrummell, 2016/04/26 21:23:56]

Done.

+        emptyAudioCodecsHistogram.count(emptyAudioCodecs);
+    }
+
+    bool emptyVideoCodecs = false;
+    if (!config.videoCapabilities.isEmpty()) {
+        for (const auto& video : config.videoCapabilities) {
+            if (video.codecs.isEmpty()) {
+                emptyVideoCodecs = true;
+                break;
+            }
+        }
+
+        DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, emptyVideoCodecsHistogram,
+            new EnumerationHistogram("Media.EME.RequestMediaKeySystemAccess.HasEmptyVideoCodecs", 2));
+        emptyVideoCodecsHistogram.count(emptyVideoCodecs);
+    }
+
+    if (emptyAudioCodecs || emptyVideoCodecs) {
+        m_resolver->getExecutionContext()->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel,
+            "Container only contentType strings that do not imply a specific set of codecs will not be "

[ddorwin, 2016/04/25 22:58:51]

Since this is a deprecation message, we can be more specific:
contentType strings without codecs will...

[jrummell, 2016/04/26 21:23:56]

Done.

+            "supported in the future. Please specify the desired codec as part of the contentType."));

[ddorwin, 2016/04/25 22:58:51]

"codec(s)"

[jrummell, 2016/04/26 21:23:56]

Done.

+    }
+}
+
 } // namespace
 
 ScriptPromise NavigatorRequestMediaKeySystemAccess::requestMediaKeySystemAccess(
     ScriptState* scriptState,
     Navigator& navigator,
     const String& keySystem,
     const HeapVector<MediaKeySystemConfiguration>& supportedConfigurations)
 {
     WTF_LOG(Media, "NavigatorRequestMediaKeySystemAccess::requestMediaKeySystemAccess()");
 
@@ skipping 42 matching lines @@
     //    initialize the MediaKeySystemAccess object.
     MediaKeysController* controller = MediaKeysController::from(document->page());
     WebEncryptedMediaClient* mediaClient = controller->encryptedMediaClient(executionContext);
     mediaClient->requestMediaKeySystemAccess(WebEncryptedMediaRequest(initializer));
 
     // 8. Return promise.
     return promise;
 }
 
 } // namespace blink