Bug 86409 – Site-specific hack: Disclaim WebM as a supported type on Mac for YouTube.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Site-specific hack: Disclaim WebM as a supported type on Mac for YouTube.

[Jer Noble]

<rdar://problem/11448850>

[Radar WebKit Bug Importer]

<rdar://problem/11450318>

[Jer Noble]

Created attachment 141807 [details]
Patch

[WebKit Review Bot]

Attachment 141807 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/HTMLMediaElement.cpp:3119:  check_again is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrew Scherkus]

Comment on attachment 141807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141807&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:625
> +        if (document()->securityOrigin()->host().endsWith("youtube.com", false)

Is it an issue with the way youtube encodes webm files or are other sites affected?

I noticed the Perian folks just posted saying they're retiring the project :\

Comment on attachment 141807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141807&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:625
>> +        if (document()->securityOrigin()->host().endsWith("youtube.com", false)
> 
> Is it an issue with the way youtube encodes webm files or are other sites affected?
> 
> I noticed the Perian folks just posted saying they're retiring the project :\

Won’t this also match documents from the host www.thisisnotyoutube.com?

[Maciej Stachowiak]

Comment on attachment 141807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141807&action=review

>>> Source/WebCore/html/HTMLMediaElement.cpp:625
>>> +        if (document()->securityOrigin()->host().endsWith("youtube.com", false)
>> 
>> Is it an issue with the way youtube encodes webm files or are other sites affected?
>> 
>> I noticed the Perian folks just posted saying they're retiring the project :\
> 
> Won’t this also match documents from the host www.thisisnotyoutube.com?

r- for reasons that Mitz gave. Right thing to do would be to test for either a specific whitelist of hosts, or else exact match for "youtube.com" or suffix match for ".youtube.com"..

[Maciej Stachowiak]

(In reply to comment #5)
> (From update of attachment 141807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141807&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:625
> > +        if (document()->securityOrigin()->host().endsWith("youtube.com", false)
> 
> Is it an issue with the way youtube encodes webm files or are other sites affected?
> 
> I noticed the Perian folks just posted saying they're retiring the project :\

We only have reports on YouTube, though the issue could affect any other site that serves either H.264 or WebM, and prefers WebM when possible. We're just making the more conservative change for now. Long term, we may need to entirely blacklist Perian, since it appears to have a broken WebM codec and now will likely never be fixed.

[Dale Curtis]

FWIW, by "hangs forever" do you really mean forever, or just a long time? Chrome previously had a local FFmpeg patch which significantly sped up WebM/matroska load times by parsing clusters incrementally. On a slow connection the pre-patch loading could take 20+ seconds.

We recently landed this patch in the upstream FFmpeg repository, so if the next version of Perian updates to FFmpeg head, they should have the patch. If anyone is in contact with them, it might be good information to pass on.

[Jer Noble]

(In reply to comment #6)
> (From update of attachment 141807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141807&action=review
> 
> Won’t this also match documents from the host www.thisisnotyoutube.com?

Whoops! Yes, it will.

(In reply to comment #7)
> (From update of attachment 141807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141807&action=review
> 
> r- for reasons that Mitz gave. Right thing to do would be to test for either a specific whitelist of hosts, or else exact match for "youtube.com" or suffix match for ".youtube.com"..

Right.  Changed to match the latter.

[Jer Noble]

(In reply to comment #9)
> FWIW, by "hangs forever" do you really mean forever, or just a long time? 

In order to allow QuickTime to open a .webm file, Perian must effectively build up a sample table.  There are various ways to do this incrementally, but Perian apparently is downloading and parsing the entire media container before the load completes.  

So I mean forever, in that the media file downloaded completely, but the movie wasn't opened within a few minutes of the download completing.

> We recently landed this patch in the upstream FFmpeg repository, so if the next version of Perian updates to FFmpeg head, they should have the patch. If anyone is in contact with them, it might be good information to pass on.

Unfortunately, it doesn't appear that there will be an new versions of Perian.  See http://perian.org.

[Jer Noble]

Created attachment 141884 [details]
Patch

I tried, and failed, to find way to move this site-specific hack into MediaPlayer or better yet MediaPlayerPrivateQTKit.  I could not find a way to do so that was acceptably non-disurptive.

[WebKit Review Bot]

Attachment 141884 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/HTMLMediaElement.cpp:3121:  check_again is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 141884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141884&action=review

Seems generally fine, but needs a little refactoring. We don’t want the same logic repeated twice.

> Source/WebCore/html/HTMLMediaElement.cpp:629
> +    Settings* settings = document()->settings();
> +    if (settings && settings->needsSiteSpecificQuirks()) {
> +        String host = document()->securityOrigin()->host();
> +        if ((host.endsWith(".youtube.com", false) || equalIgnoringCase("youtube.com", host))
> +            && (mimeType.startsWith("video/webm", false) || mimeType.startsWith("video/x-flv", false)))
> +            return "";
> +    }

This code is repeated twice. Could you please put it into a helper function? That way you can also avoid repeating the comment twice.

The comment does not explain why we are skipping both the video/webm MIME type and the video/x-flv MIME type. Nor why the hack is specifically for YouTube.

[Aaron Colwell]

Is there really no better way to solve this? It seems like we are setting a bad precedent here by fixing a QuickTime issue in HTMLMediaElement. What functionality is missing in the MediaPlayer that prevents this from being hidden down in the QuickTime specific implementation?

[Jer Noble]

(In reply to comment #14)
> (From update of attachment 141884 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141884&action=review
> 
> Seems generally fine, but needs a little refactoring. We don’t want the same logic repeated twice.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:629
> > +    Settings* settings = document()->settings();
> > +    if (settings && settings->needsSiteSpecificQuirks()) {
> > +        String host = document()->securityOrigin()->host();
> > +        if ((host.endsWith(".youtube.com", false) || equalIgnoringCase("youtube.com", host))
> > +            && (mimeType.startsWith("video/webm", false) || mimeType.startsWith("video/x-flv", false)))
> > +            return "";
> > +    }
> 
> This code is repeated twice. Could you please put it into a helper function? That way you can also avoid repeating the comment twice.

Sure thing.  I'm actually going to try to move this down into MediaPlayer::supportsType(), which will be the equivalent of a helper function here.

> The comment does not explain why we are skipping both the video/webm MIME type and the video/x-flv MIME type. Nor why the hack is specifically for YouTube.

I'll update the comment.

[Jer Noble]

(In reply to comment #15)
> Is there really no better way to solve this? It seems like we are setting a bad precedent here by fixing a QuickTime issue in HTMLMediaElement. What functionality is missing in the MediaPlayer that prevents this from being hidden down in the QuickTime specific implementation?

The reason it's not in MediaPlayer (yet) is that MediaPlayer::supportsType() is a static function, and so doesn't have access to the MediaPlayerClient and the information required to support this site-specific hack.  And at the time that HTMLMediaElement::canPlayType() is called, the m_player variable may be null, so it's not as easy as just making it a class function.

However, Sam and Eric pointed out that I could just pass the HTMLMediaElement into MediaPlayer::supportsType() as a MediaPlayerClient*, and then we could move the site-specific hack in there.

So, I'm still working on it.  Patch coming soon. :)

[Aaron Colwell]

(In reply to comment #17)
> (In reply to comment #15)
> > Is there really no better way to solve this? It seems like we are setting a bad precedent here by fixing a QuickTime issue in HTMLMediaElement. What functionality is missing in the MediaPlayer that prevents this from being hidden down in the QuickTime specific implementation?
> 
> The reason it's not in MediaPlayer (yet) is that MediaPlayer::supportsType() is a static function, and so doesn't have access to the MediaPlayerClient and the information required to support this site-specific hack.  And at the time that HTMLMediaElement::canPlayType() is called, the m_player variable may be null, so it's not as easy as just making it a class function.
> 
> However, Sam and Eric pointed out that I could just pass the HTMLMediaElement into MediaPlayer::supportsType() as a MediaPlayerClient*, and then we could move the site-specific hack in there.
> 
> So, I'm still working on it.  Patch coming soon. :)

Ok. Thanks for the explanation. I'm happy to see your're still exploring alternate solutions. :)

[Jer Noble]

Created attachment 142036 [details]
Patch

Moved the site-specific hack into MediaPlayer.  This required a new MediaPlayerClient prototype, exposed both on HTMLMediaElement and from within DOMImplementation.

Conceivably, I could push this further into MediaPlayerPrivateQTKit, but that would require changing the MediaEngineSupportsType signature, which would in turn require changing every MediaPlayerPrivate instance on every platform, which would be very disruptive.

[Darin Adler]

Comment on attachment 142036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142036&action=review

r=me, but I am concerned about the URL vs. securityOrigin issue that Sam raised

> Source/WebCore/dom/DOMImplementation.cpp:72
> +    DOMImplementationSupportsTypeClient(bool needsHacks, String host)

Should be const String&.

> Source/WebCore/dom/DOMImplementation.cpp:74
> +    : m_needsHacks(needsHacks)
> +    , m_host(host)

We normally indent these by four spaces.

> Source/WebCore/dom/DOMImplementation.cpp:79
> +    bool mediaPlayerNeedsSiteSpecificHacks() const OVERRIDE { return m_needsHacks; }
> +    String mediaPlayerDocumentHost() const OVERRIDE { return m_host; }

Should include the keyword virtual as well -- we are normally explicit about that.

These two overrides can and should be private instead of public.

> Source/WebCore/html/HTMLMediaElement.cpp:4396
> +    return document()->securityOrigin()->host();

I thought Sam said this should be based on the document’s URL, not its securityOrigin?

> Source/WebCore/platform/graphics/MediaPlayer.cpp:735
> +        if ((host.endsWith(".youtube.com", false) || equalIgnoringCase("youtube.com", host))
> +            && (contentType.type().startsWith("video/webm", false) || contentType.type().startsWith("video/x-flv", false)))
> +            return IsNotSupported;

Right here is where the “why” comment belongs.

> Source/WebCore/platform/graphics/MediaPlayer.h:203
> +    static MediaPlayer::SupportsType supportsType(const ContentType&, const String& keySystem, const MediaPlayerSupportsTypeClient*);

Could use a reference here instead of a pointer.

[Jer Noble]

(In reply to comment #20)
> (From update of attachment 142036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142036&action=review
> 
> r=me, but I am concerned about the URL vs. securityOrigin issue that Sam raised
> 
> > Source/WebCore/dom/DOMImplementation.cpp:72
> > +    DOMImplementationSupportsTypeClient(bool needsHacks, String host)
> 
> Should be const String&.

Changed.

> > Source/WebCore/dom/DOMImplementation.cpp:74
> > +    : m_needsHacks(needsHacks)
> > +    , m_host(host)
> 
> We normally indent these by four spaces.

Re-indented.

> > Source/WebCore/dom/DOMImplementation.cpp:79
> > +    bool mediaPlayerNeedsSiteSpecificHacks() const OVERRIDE { return m_needsHacks; }
> > +    String mediaPlayerDocumentHost() const OVERRIDE { return m_host; }
> 
> Should include the keyword virtual as well -- we are normally explicit about that.
> 
> These two overrides can and should be private instead of public.

Done and done.

> > Source/WebCore/html/HTMLMediaElement.cpp:4396
> > +    return document()->securityOrigin()->host();
> 
> I thought Sam said this should be based on the document’s URL, not its securityOrigin?

Sorry, I missed Sam's comment earlier.  Changed.

> > Source/WebCore/platform/graphics/MediaPlayer.cpp:735
> > +        if ((host.endsWith(".youtube.com", false) || equalIgnoringCase("youtube.com", host))
> > +            && (contentType.type().startsWith("video/webm", false) || contentType.type().startsWith("video/x-flv", false)))
> > +            return IsNotSupported;
> 
> Right here is where the “why” comment belongs.

I'll pull the comment in here from the ChangeLog.

> > Source/WebCore/platform/graphics/MediaPlayer.h:203
> > +    static MediaPlayer::SupportsType supportsType(const ContentType&, const String& keySystem, const MediaPlayerSupportsTypeClient*);
> 
> Could use a reference here instead of a pointer.

Wouldn't it be weird, then, to have to pass a "*this" in HTMLMediaElement?:

    MediaPlayer::supportsType(ContentType(mimeType), keySystem, *this);

[Jer Noble]

Committed r117147: <http://trac.webkit.org/changeset/117147>