[webkit-reviews] review granted: [Bug 39577] Improve HTML5 <video> tag performance : [Attachment 56852] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted  review:
Bug 39577: Improve HTML5 <video> tag performance
https://bugs.webkit.org/show_bug.cgi?id=39577

Attachment 56852: Patch
https://bugs.webkit.org/attachment.cgi?id=56852&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> -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<MediaPl
ayerPrivateQuickTimeVisualContext>*)
>  {
>      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