Bug 158335 - HTMLMediaElement.prototype.canPlayType accounting for 250-750ms first loading theverge.com
Summary: HTMLMediaElement.prototype.canPlayType accounting for 250-750ms first loading...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-02 19:11 PDT by Joseph Pecoraro
Modified: 2016-06-10 21:37 PDT (History)
8 users (show)

See Also:


Attachments
[IMAGE] Web Inspector Profile - 750ms in canPlayType (248.09 KB, image/png)
2016-06-02 19:11 PDT, Joseph Pecoraro
no flags Details
Proposed patch (18.45 KB, patch)
2016-06-08 13:19 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Address post-review comments. (3.79 KB, patch)
2016-06-10 13:57 PDT, Eric Carlson
jer.noble: review+
eric.carlson: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-06-02 19:11:39 PDT
Created attachment 280402 [details]
[IMAGE] Web Inspector Profile - 750ms in canPlayType

* SUMMARY
HTMLMediaElement.prototype.canPlayType accounting for 250-750ms first loading theverge.com.

See attached screenshot. With the steps to reproduce below I saw 250ms and 750ms spent in canPlayType for the first page load in a WebContentProcess.

* STEPS TO REPRODUCE
1. Open about:blank
2. Open Web Inspector
3. Show Timeline Tab
4. Enable Script & Events Timeline
5. Navigate the page to <http://www.theverge.com>
6. Let timeline auto-record and stop recording
7. Show Script & Events Timeline, Call Trees, Bottom Up View
  => Many milliseconds were spent inside of HTMLMediaElement.prototype.canPlayType
Comment 1 Radar WebKit Bug Importer 2016-06-02 19:11:59 PDT
<rdar://problem/26615416>
Comment 2 Eric Carlson 2016-06-08 13:19:54 PDT
Created attachment 280829 [details]
Proposed patch
Comment 3 Brent Fulgham 2016-06-08 13:34:45 PDT
Comment on attachment 280829 [details]
Proposed patch

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

r=me

> Source/WebCore/ChangeLog:18
> +        * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:

Do we need similar changes made to the MediaPlayerPrivateAVFoundationCF code?
Comment 4 WebKit Commit Bot 2016-06-08 14:06:14 PDT
Comment on attachment 280829 [details]
Proposed patch

Clearing flags on attachment: 280829

Committed r201831: <http://trac.webkit.org/changeset/201831>
Comment 5 WebKit Commit Bot 2016-06-08 14:06:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Joseph Pecoraro 2016-06-08 15:14:42 PDT
Comment on attachment 280829 [details]
Proposed patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.mm:69
> +        m_lock.lock();
> +        m_status = Loaded;
> +        m_lock.unlock();

Maybe we can dispatch_release m_loaderQueue some time after we've loaded? Since we won't need it anymore. Its just abandoned memory.
Comment 7 Darin Adler 2016-06-09 20:06:58 PDT
Comment on attachment 280829 [details]
Proposed patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.h:27
> +#ifndef AVFoundationMIMETypeCache_h
> +#define AVFoundationMIMETypeCache_h

New files should use #pragma once, not #ifndef/define.

> Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.mm:36
> +#import <AVFoundation/AVAsset.h>
> +#import <wtf/HashMap.h>
> +#import <wtf/Locker.h>
> +#import <wtf/NeverDestroyed.h>
> +
> +#import "CoreMediaSoftLink.h"

This should be a single paragraph; not two paragraphs.

> Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.mm:90
> +    return cache.get();

I don’t think we need the .get() here; maybe I am mistaken, though.
Comment 8 Csaba Osztrogonác 2016-06-10 01:23:56 PDT
(In reply to comment #4)
> Comment on attachment 280829 [details]
> Proposed patch
> 
> Clearing flags on attachment: 280829
> 
> Committed r201831: <http://trac.webkit.org/changeset/201831>

just to document, cmake buildfix landed in https://trac.webkit.org/changeset/201916
Comment 9 Eric Carlson 2016-06-10 13:57:04 PDT
Created attachment 281043 [details]
Address post-review comments.
Comment 10 Eric Carlson 2016-06-10 13:59:28 PDT
(In reply to comment #8)
> (In reply to comment #4)
> > Comment on attachment 280829 [details]
> > Proposed patch
> > 
> > Clearing flags on attachment: 280829
> > 
> > Committed r201831: <http://trac.webkit.org/changeset/201831>
> 
> just to document, cmake buildfix landed in
> https://trac.webkit.org/changeset/201916

Sorry about that, but thanks for the fix Ossy!
Comment 11 Eric Carlson 2016-06-10 13:59:46 PDT
(In reply to comment #6)
> Comment on attachment 280829 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280829&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.mm:69
> > +        m_lock.lock();
> > +        m_status = Loaded;
> > +        m_lock.unlock();
> 
> Maybe we can dispatch_release m_loaderQueue some time after we've loaded?
> Since we won't need it anymore. Its just abandoned memory.

Good idea, will do.
Comment 12 Eric Carlson 2016-06-10 14:00:37 PDT
(In reply to comment #7)
> Comment on attachment 280829 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280829&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.h:27
> > +#ifndef AVFoundationMIMETypeCache_h
> > +#define AVFoundationMIMETypeCache_h
> 
> New files should use #pragma once, not #ifndef/define.
> 

Fixed.

> > Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.mm:36
> > +#import <AVFoundation/AVAsset.h>
> > +#import <wtf/HashMap.h>
> > +#import <wtf/Locker.h>
> > +#import <wtf/NeverDestroyed.h>
> > +
> > +#import "CoreMediaSoftLink.h"
> 
> This should be a single paragraph; not two paragraphs.
> 

Fixed.

> > Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.mm:90
> > +    return cache.get();
> 
> I don’t think we need the .get() here; maybe I am mistaken, though.

Fixed.
Comment 13 Joseph Pecoraro 2016-06-10 21:37:46 PDT
Comment on attachment 281043 [details]
Address post-review comments.

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

> Source/WebCore/platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.mm:89
>      if (status != Loaded) {
> +        ASSERT(m_loaderQueue);
>          loadTypes();
>          dispatch_sync(m_loaderQueue, ^{ });
>      }

I think there is a race condition here between checking the status after unlocking and doing the dispatch_sync.

Between the m_lock.unlock() and the dispatch_sync here, the status could have been Loading, and m_loaderQueue could be released before it is used here.

Maybe not an issue in practice?