[Android, Media] Don't unlock MediaStream elements; instead allow MS to autoplay without a gesture.

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1013 matching lines @@
     setPlayerPreload();
 
     if (fastHasAttribute(mutedAttr))
         m_muted = true;
     updateVolume();
 
     DCHECK(!m_mediaSource);
 
     bool attemptLoad = true;
 
+    // MediaSource is a blob URL that is not a media stream.

[foolip, 2016/06/28 15:28:51]

This comment doesn't seem quite accurate, if it's explaining when
isStreamOrBlobUrl is true at least.

Given that you've changed loadType() to always (?) work, may I suggest
simplifying like this?

m_mediaSource = HTMLMediaSource::lookup(url.getString());
if (m_mediaSource) { ... attach ... }
bool isObject = loadType() != WebMediaPlayer::LoadTypeURL;

AFAICT, there's no reason to check the protocol at all, and the constant name
mediaSourceBlobProtocol is misleading.

Then, use isObject below for canLoadResource and for bypassing deferLoad().

[whywhat, 2016/06/28 16:13:50]

Done. I inlined |isObject| since it made the expression positive (==
LoadTypeURL). Can we be sure a null url will not crash HTMLMediaSource::lookup?
StringImpl::rawHash() crashed on a null string in loadType().

[whywhat, 2016/06/28 18:06:34]

So loadType() doesn't catch corrupt or revoked media source URLs (see the
failing mediasource-preload.html test in patchset 7) so I changed it to
isStreamOrBlobUrl only inlined it in the if statement.

[foolip, 2016/06/29 09:30:27]

Actually I think that it's the test that should change here. Because
https://url.spec.whatwg.org/#concept-url-object talks about an associated object
and not about the URL scheme, the most natural way to spec this in
https://github.com/whatwg/html/pull/1037 was to just check if there's an object
associated with the URL, like what you had in PS6.

Can you revert back to PS6 and change the test to assert that an invalid "blob"
URL is treated like any other invalid URL? For example, the URL "blobzzz:x"
together with preload none would fire the suspend event, and then when the load
really starts you get an error.

MediaSourceRegistry::lookup(const String& url) has a null check, which is why it
didn't crash there.

     bool isStreamOrBlobUrl = source.isMediaStream() || url.protocolIs(mediaSourceBlobProtocol);

[foolip, 2016/06/28 15:28:51]

While you're here, may I suggest replacing mediaSourceBlobProtocol with just
"blob"? It's inline elsewhere in core/, and the name of this constant is
misleading.

[whywhat, 2016/06/28 16:13:50]

Done.

-    if (isStreamOrBlobUrl) {
-        bool isMediaStream = source.isMediaStream() || (source.isURL() && isMediaStreamURL(url.getString()));
-        if (isMediaStream) {
-            m_autoplayHelper->unlockUserGesture(GesturelessPlaybackEnabledByStream);
-        } else {
-            m_mediaSource = HTMLMediaSource::lookup(url.getString());
+    if (isStreamOrBlobUrl && !source.isMediaStream() && (!source.isURL() || !isMediaStreamURL(url.getString()))) {

[sof, 2016/06/27 19:28:48]

Can (at least) be reduced to

if (url.protocolIs(mediaSourceBlobProtocol) &&
!isMediaStreamURL(url.getString()))

..but perhaps the RHS of (&&) is redundant?

[whywhat, 2016/06/28 16:13:50]

I believe now it's even simpler.

+        m_mediaSource = HTMLMediaSource::lookup(url.getString());
 
-            if (m_mediaSource) {
-                if (!m_mediaSource->attachToElement(this)) {
-                    // Forget our reference to the MediaSource, so we leave it alone
-                    // while processing remainder of load failure.
-                    m_mediaSource = nullptr;
-                    attemptLoad = false;
-                }
+        if (m_mediaSource) {
+            if (!m_mediaSource->attachToElement(this)) {

[sof, 2016/06/27 19:28:48]

nit: the two ifs could be fused.

[whywhat, 2016/06/28 16:13:50]

Ditto.

[whywhat, 2016/06/28 18:06:34]

Actually fused these diffs :)

+                // Forget our reference to the MediaSource, so we leave it alone
+                // while processing remainder of load failure.
+                m_mediaSource = nullptr;
+                attemptLoad = false;
             }
         }
     }
 
     bool canLoadResource = source.isMediaStream() || canLoadURL(url, contentType);
     if (attemptLoad && canLoadResource) {
         DCHECK(!webMediaPlayer());
 
         // Conditionally defer the load if effective preload is 'none'.
         // Skip this optional deferral for MediaStream sources or any blob URL,
@@ skipping 157 matching lines @@
     }
     DCHECK_EQ(m_deferredLoadState, WaitingForStopDelayingLoadEventTask);
     m_deferredLoadState = WaitingForTrigger;
 }
 
 WebMediaPlayer::LoadType HTMLMediaElement::loadType() const
 {
     if (m_mediaSource)
         return WebMediaPlayer::LoadTypeMediaSource;
 
-    if (m_srcObject || isMediaStreamURL(m_currentSrc.getString()))
+    if (m_srcObject || (!m_currentSrc.isNull() && isMediaStreamURL(m_currentSrc.getString())))

[foolip, 2016/06/28 15:28:51]

Did this assert somewhere before?

[whywhat, 2016/06/28 16:13:50]

isMediaStreamURL is using a hash map that eventually calls into
StringImpl::rawHash() which crashes on a null string. I guess current src could
be null. I observed the crash in one of the Android WebRTC tests (you can find
it in the second patchset CQ results -
WebRtcGetUserMediaBrowserTest.RenderVideoTrackInMultipleTagsAndPause).

         return WebMediaPlayer::LoadTypeMediaStream;
 
     return WebMediaPlayer::LoadTypeURL;
 }
 
 bool HTMLMediaElement::textTracksAreReady() const
 {
     // 4.8.10.12.1 Text track model
     // ...
     // The text tracks of a media element are ready if all the text tracks whose mode was not
@@ skipping 2481 matching lines @@
 void HTMLMediaElement::unlockUserGesture()
 {
     m_lockedPendingUserGesture = false;
 }
 
 bool HTMLMediaElement::isGestureNeededForPlayback() const
 {
     if (!m_lockedPendingUserGesture)
         return false;
 
+    if (loadType() == WebMediaPlayer::LoadTypeMediaStream)
+        return false;
+
     // We want to allow muted video to autoplay if:
     // - the flag is enabled;
     // - Data Saver is not enabled;
     // - Autoplay is enabled in settings;
     if (isHTMLVideoElement()
         && muted()
         && RuntimeEnabledFeatures::autoplayMutedVideosEnabled()
         && !(document().settings() && document().settings()->dataSaverEnabled())
         && isAutoplayAllowedPerSettings()) {
         return false;
@@ skipping 285 matching lines @@
 
 IntRect HTMLMediaElement::AutoplayHelperClientImpl::absoluteBoundingBoxRect() const
 {
     IntRect result;
     if (LayoutObject* object = m_element->layoutObject())
         result = object->absoluteBoundingBoxRect();
     return result;
 }
 
 } // namespace blink