Bug 86409

Summary: Site-specific hack: Disclaim WebM as a supported type on Mac for YouTube.
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: acolwell, dalecurtis, eric.carlson, feature-media-reviews, mitz, mjs, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Jer Noble 2012-05-14 16:20:00 PDT
Site-specific hack: Disclaim WebM as a supported type on Mac for YouTube.
Comment 1 Jer Noble 2012-05-14 16:20:31 PDT
<rdar://problem/11448850>
Comment 2 Radar WebKit Bug Importer 2012-05-14 16:21:08 PDT
<rdar://problem/11450318>
Comment 3 Jer Noble 2012-05-14 16:30:41 PDT
Created attachment 141807 [details]
Patch
Comment 4 WebKit Review Bot 2012-05-14 16:33:26 PDT
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.
Comment 5 Andrew Scherkus 2012-05-14 16:45:18 PDT
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 6 mitz 2012-05-14 16:47:10 PDT
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?
Comment 7 Maciej Stachowiak 2012-05-14 18:01:01 PDT
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"..
Comment 8 Maciej Stachowiak 2012-05-14 18:02:28 PDT
(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.
Comment 9 Dale Curtis 2012-05-14 18:36:26 PDT
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.
Comment 10 Jer Noble 2012-05-15 00:51:56 PDT
(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.
Comment 11 Jer Noble 2012-05-15 00:59:54 PDT
(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.
Comment 12 Jer Noble 2012-05-15 01:09:01 PDT
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.
Comment 13 WebKit Review Bot 2012-05-15 01:12:27 PDT
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.
Comment 14 Darin Adler 2012-05-15 10:06:22 PDT
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.
Comment 15 Aaron Colwell 2012-05-15 10:19:39 PDT
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?
Comment 16 Jer Noble 2012-05-15 10:25:11 PDT
(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.
Comment 17 Jer Noble 2012-05-15 10:28:11 PDT
(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. :)
Comment 18 Aaron Colwell 2012-05-15 10:39:40 PDT
(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. :)
Comment 19 Jer Noble 2012-05-15 12:55:39 PDT
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.
Comment 20 Darin Adler 2012-05-15 13:14:19 PDT
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.
Comment 21 Jer Noble 2012-05-15 13:40:34 PDT
(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);
Comment 22 Jer Noble 2012-05-15 13:53:37 PDT
Committed r117147: <http://trac.webkit.org/changeset/117147>