[Webkit-unassigned] [Bug 39577] Improve HTML5 <video> tag performance

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39577


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56852|                            |review+, commit-queue-
               Flag|                            |




--- Comment #13 from Eric Carlson <eric.carlson at apple.com>  2010-05-24 08:17:01 PST ---
(From update of attachment 56852)
> -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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.