Bug 158335 – HTMLMediaElement.prototype.canPlayType accounting for 250-750ms first loading theverge.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Joseph Pecoraro]

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

[Radar WebKit Bug Importer]

<rdar://problem/26615416>

[Eric Carlson]

Created attachment 280829 [details]
Proposed patch

[Brent Fulgham]

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?

[WebKit Commit Bot]

Comment on attachment 280829 [details]
Proposed patch

Clearing flags on attachment: 280829

Committed r201831: <http://trac.webkit.org/changeset/201831>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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.

[Darin Adler]

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.

[Csaba Osztrogonác]

(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

[Eric Carlson]

Created attachment 281043 [details]
Address post-review comments.

[Eric Carlson]

(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!

[Eric Carlson]

(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.

[Eric Carlson]

(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.

[Joseph Pecoraro]

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?