Bug 39577 - Improve HTML5 <video> tag performance
Summary: Improve HTML5 <video> tag performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Windows 7
: P2 Normal
Assignee: Nobody
URL: http://vimeo.com/7352118
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-05-23 18:08 PDT by Jer Noble
Modified: 2010-05-24 18:40 PDT (History)
1 user (show)

See Also:


Attachments
Patch (400.94 KB, patch)
2010-05-24 00:22 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (395.22 KB, patch)
2010-05-24 00:27 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (395.51 KB, patch)
2010-05-24 01:08 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (395.07 KB, patch)
2010-05-24 01:11 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (388.02 KB, patch)
2010-05-24 18:22 PDT, Jer Noble
sfalken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2010-05-23 18:08:14 PDT
HTML5 <video> tag performance worse than Flash
Comment 1 Jer Noble 2010-05-23 18:09:25 PDT
rdar://problem/7982458
Comment 2 Jer Noble 2010-05-24 00:22:59 PDT
Created attachment 56851 [details]
Patch
Comment 3 WebKit Review Bot 2010-05-24 00:27:19 PDT
Attachment 56851 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/WKCACFLayer.h:158:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jer Noble 2010-05-24 00:27:47 PDT
Created attachment 56852 [details]
Patch
Comment 5 Jer Noble 2010-05-24 00:28:12 PDT
This time, with less extraneous vcproj file churn.
Comment 6 WebKit Review Bot 2010-05-24 00:30:33 PDT
Attachment 56852 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/WKCACFLayer.h:158:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Jer Noble 2010-05-24 00:39:38 PDT
This time, with less extraneous vcproj file churn.
Comment 8 Jer Noble 2010-05-24 01:08:01 PDT
Created attachment 56859 [details]
Patch
Comment 9 WebKit Review Bot 2010-05-24 01:09:04 PDT
Attachment 56859 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/WKCACFLayer.h:158:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Jer Noble 2010-05-24 01:11:24 PDT
Created attachment 56860 [details]
Patch
Comment 11 WebKit Review Bot 2010-05-24 01:14:59 PDT
Attachment 56860 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/WKCACFLayer.h:158:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Chris Marrin 2010-05-24 07:32:13 PDT
Comment on attachment 56860 [details]
Patch

WebCore/WebCore.vcproj/WebCore.vcproj:44112
 +  		<Filter
This looks like a spurious change to the vcproj file. It's best to avoid checking them in. I've had success reverting the vcproj and then hand editing it to add the lines needed.

WebCore/WebCore.vcproj/WebCore.vcproj:44111
 +  		</Filter>
This looks like a spurious change to the vcproj file. It's best to avoid checking them in. I've had success reverting the vcproj and then hand editing it to add the lines needed.

WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:683
 +      retrieveCurrentImage();
Why the change in indenting?

WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:766
 +          if (!buffer.lockBaseAddress()) {
More indenting changes?

WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:775
 +                  // Debug QuickTime links against a non-Debug version of CoreFoundation, so the CFDictionary attached to the CVPixelBuffer cannot be directly passed on into the CAImageQueue without being converted to a non-Debug CFDictionary.  Additionally, old versions of QuickTime used a non-AAS CoreFoundation, so the types are not interchangable even in the release case.
This comment should be split onto multiple lines

WebCore/platform/graphics/win/WKCACFLayer.cpp:507
 +          }
It would be nice if we could print something no matter what the type. Do we know all the types this could possibly return, or is there a way to go from CFTypeID to a string describing the type? Even if it just said ImageQueue without any additional info, it would be useful

WebCore/platform/graphics/win/WKCACFLayer.h:209
 +      CFTimeInterval speed() const { return CACFLayerGetSpeed(layer()); }
Weird. How did this ever compile? Oh, I know how. We never use this function so it never did compile! Thanks for finding it.

WebCore/platform/graphics/win/WKCAImageQueue.cpp:51
 +  }
What are these for? I don't see them being used anywhere

WebCore/platform/graphics/win/WKCAImageQueue.h:63
 +      };
I think the WebKit style is to not prefix enumerants like this.

WebCore/platform/graphics/win/WKCAImageQueue.h:74
 +      uint32_t height();
These can be const

WebCore/platform/graphics/win/WKCAImageQueue.h:77
 +      uint32_t capacity();
This can be const

WebCore/platform/graphics/win/WKCAImageQueue.h:85
 +      uint32_t flags();
This can be const

WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:123
 +  };
No prefixes needed here?
Comment 13 Eric Carlson 2010-05-24 08:17:01 PDT
Comment on attachment 56852 [details]
Patch

> -const CFStringRef kMinQuartzCoreVersion = CFSTR("1.0.43.0");
> -const CFStringRef kMinCoreVideoVersion = CFSTR("1.0.0.2");
> +const CFStringRef kMinQuartzCoreVersion = CFSTR("1.0.42.0");
> +const CFStringRef kMinCoreVideoVersion = CFSTR("1.0.1.0");
>  
These are only used inside of arePrerequisitesSatisfied. You might as well define them there so the code doesn't have to run at startup.


>  void MediaPlayerPrivateQuickTimeVisualContext::visualContextTimerFired(Timer<MediaPlayerPrivateQuickTimeVisualContext>*)
>  {
>      if (m_visualContext && m_visualContext->isImageAvailableForTime(0))
> -        retrieveCurrentImage();
> +    retrieveCurrentImage();
>
Is there a reason to remove the indentation?


> +            // Debug QuickTime links against a non-Debug version of CoreFoundation, so the CFDictionary attached to the CVPixelBuffer cannot be directly passed on into the CAImageQueue without being converted to a non-Debug CFDictionary.  Additionally, old versions of QuickTime used a non-AAS CoreFoundation, so the types are not interchangable even in the release case.
> +            RetainPtr<CFDictionaryRef> attachments(AdoptCF, QTCFDictionaryCreateCopyWithDataCallback(kCFAllocatorDefault, buffer.attachments(), &QTCFDictionaryCreateWithDataCallback));
> +            CFTimeInterval imageTime = QTMovieVisualContext::currentHostTime();
>  
This code is always executed so the comment about Debug QuickTime is incorrect. It would be nice if the comment was broken onto multiple lines for readability.


> +            size_t slots = m_imageQueue->collect();
>  
This local variable isn't used.


> +static bool arePrerequisitesSatisfied() 
>
This name is fairly generic, maybe "requiredDllsAvailable", or "minimumDllsAvailable"?


> +    CFShow(quartzCoreString);
> +    CFShow(coreVideoString);
> +
Oops, you definitely don't want to leave these in!


> -    void setContents(CGImageRef contents) { CACFLayerSetContents(layer(), contents); setNeedsCommit(); }
> -    CGImageRef contents() const { return static_cast<CGImageRef>(const_cast<void*>(CACFLayerGetContents(layer()))); }
> +    void setContents(CFTypeRef contents) { CACFLayerSetContents(layer(), contents); setNeedsCommit(); }
>
Not sure, but using a comma instead of a semi-colon between commands might quiet the style checker complaint.


> +WKCAImageQueue::WKCAImageQueue(uint32_t width, uint32_t height, uint32_t capacity)
> +    : m_private(new WKCAImageQueuePrivate())
> +{
> +    m_private->m_imageQueue = wkCAImageQueueCreate(width, height, capacity);
> +}
> +
> +WKCAImageQueue::WKCAImageQueue(const WKCAImageQueue& o)
> +    : m_private(new WKCAImageQueuePrivate())
> +{
> +    m_private->m_imageQueue = WKCAImageQueueRetain(o.m_private->m_imageQueue);
> +}

You only use the first form of the constructor, can this one be made private?

> +WKCAImageQueue& WKCAImageQueue::operator=(const WKCAImageQueue& o)
> +{
> +    WKCAImageQueueRetain(o.m_private->m_imageQueue);
> +    WKCAImageQueueRelease(m_private->m_imageQueue);
> +    m_private->m_imageQueue = o.m_private->m_imageQueue;
> +    return *this;
> +}
> +

Does this need to be public?


> +uint32_t WKCAImageQueue::width()
> +uint32_t WKCAImageQueue::height()
> +void WKCAImageQueue::setSize(uint32_t width, uint32_t height)
> +uint32_t WKCAImageQueue::capacity()
> +void WKCAImageQueue::flush()
> +uint32_t WKCAImageQueue::flags()

None of these are used yet, I would leave them out of the class. This will also allow you to remove the WebKitSystemInterface functions.

r=me with these changes
Comment 14 Eric Carlson 2010-05-24 08:18:06 PDT
Comment on attachment 56852 [details]
Patch

Remove flags set on the wrong patch
Comment 15 Jer Noble 2010-05-24 14:55:10 PDT
(In reply to comment #13)
> (From update of attachment 56852 [details])
> > -const CFStringRef kMinQuartzCoreVersion = CFSTR("1.0.43.0");
> > -const CFStringRef kMinCoreVideoVersion = CFSTR("1.0.0.2");
> > +const CFStringRef kMinQuartzCoreVersion = CFSTR("1.0.42.0");
> > +const CFStringRef kMinCoreVideoVersion = CFSTR("1.0.1.0");
> >  
> These are only used inside of arePrerequisitesSatisfied. You might as well define them there so the code doesn't have to run at startup.

Moved these into their relevant function (which is renamed below).

> > -        retrieveCurrentImage();
> > +    retrieveCurrentImage();
> >
> Is there a reason to remove the indentation?

Entirely accidental.  Reverted.

> > +            // Debug QuickTime links against a non-Debug version of CoreFoundation, so the CFDictionary attached to the CVPixelBuffer cannot be directly passed on into the CAImageQueue without being converted to a non-Debug CFDictionary.  Additionally, old versions of QuickTime used a non-AAS CoreFoundation, so the types are not interchangable even in the release case.

> This code is always executed so the comment about Debug QuickTime is incorrect. It would be nice if the comment was broken onto multiple lines for readability.

It's been broken up.  And I made sure to add that "Additionally," sentence to specify there's problems in the release case as well.

> > +            size_t slots = m_imageQueue->collect();
> >  
> This local variable isn't used.

Undeclared!

> > +static bool arePrerequisitesSatisfied() 
> >
> This name is fairly generic, maybe "requiredDllsAvailable", or "minimumDllsAvailable"?

Renamed it to requiredDllsAvailable().

> > +    CFShow(quartzCoreString);
> > +    CFShow(coreVideoString);
> > +
> Oops, you definitely don't want to leave these in!

Deleted!

> > -    void setContents(CGImageRef contents) { CACFLayerSetContents(layer(), contents); setNeedsCommit(); }
> > -    CGImageRef contents() const { return static_cast<CGImageRef>(const_cast<void*>(CACFLayerGetContents(layer()))); }
> > +    void setContents(CFTypeRef contents) { CACFLayerSetContents(layer(), contents); setNeedsCommit(); }
> >
> Not sure, but using a comma instead of a semi-colon between commands might quiet the style checker complaint.

I could, but my inner English teacher was yelling at me for using comma-splices.  I chose to ignore the style warnings instead. (And then that function will match all its siblings, and won't be so lonely.)

> > +WKCAImageQueue::WKCAImageQueue(uint32_t width, uint32_t height, uint32_t capacity)
> > +    : m_private(new WKCAImageQueuePrivate())
> > +{
> > +    m_private->m_imageQueue = wkCAImageQueueCreate(width, height, capacity);
> > +}
> > +
> > +WKCAImageQueue::WKCAImageQueue(const WKCAImageQueue& o)
> > +    : m_private(new WKCAImageQueuePrivate())
> > +{
> > +    m_private->m_imageQueue = WKCAImageQueueRetain(o.m_private->m_imageQueue);
> > +}
> 
> You only use the first form of the constructor, can this one be made private?

Yes!

> > +WKCAImageQueue& WKCAImageQueue::operator=(const WKCAImageQueue& o)
> > +{
> > +    WKCAImageQueueRetain(o.m_private->m_imageQueue);
> > +    WKCAImageQueueRelease(m_private->m_imageQueue);
> > +    m_private->m_imageQueue = o.m_private->m_imageQueue;
> > +    return *this;
> > +}
> > +
> 
> Does this need to be public?

No!

> > +uint32_t WKCAImageQueue::width()
> > +uint32_t WKCAImageQueue::height()
> > +void WKCAImageQueue::setSize(uint32_t width, uint32_t height)
> > +uint32_t WKCAImageQueue::capacity()
> > +void WKCAImageQueue::flush()
> > +uint32_t WKCAImageQueue::flags()
> 
> None of these are used yet, I would leave them out of the class. This will also allow you to remove the WebKitSystemInterface functions.

Ok!
Comment 16 Jer Noble 2010-05-24 16:00:51 PDT
Committed r60094: <http://trac.webkit.org/changeset/60094>
Comment 17 Jer Noble 2010-05-24 17:02:01 PDT
Roll-out changes; build broke.
Comment 18 Jer Noble 2010-05-24 18:22:51 PDT
Created attachment 56955 [details]
Patch
Comment 19 Jer Noble 2010-05-24 18:40:34 PDT
Committed r60110: <http://trac.webkit.org/changeset/60110>