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

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from Jer Noble <jer.noble at apple.com>  2010-05-24 14:55:10 PST ---
(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!

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