Bug 39577 – Improve HTML5 <video> tag performance

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

HTML5 <video> tag performance worse than Flash

[Jer Noble]

rdar://problem/7982458

[Jer Noble]

Created attachment 56851 [details]
Patch

[WebKit Review Bot]

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.

[Jer Noble]

Created attachment 56852 [details]
Patch

[Jer Noble]

This time, with less extraneous vcproj file churn.

[WebKit Review Bot]

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.

[Jer Noble]

This time, with less extraneous vcproj file churn.

[Jer Noble]

Created attachment 56859 [details]
Patch

[WebKit Review Bot]

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.

[Jer Noble]

Created attachment 56860 [details]
Patch

[WebKit Review Bot]

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.

[Chris Marrin]

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?

[Eric Carlson]

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

[Eric Carlson]

Comment on attachment 56852 [details]
Patch

Remove flags set on the wrong patch

[Jer Noble]

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

[Jer Noble]

Committed r60094: <http://trac.webkit.org/changeset/60094>

[Jer Noble]

Roll-out changes; build broke.

[Jer Noble]

Created attachment 56955 [details]
Patch

[Jer Noble]

Committed r60110: <http://trac.webkit.org/changeset/60110>