HTML5 <video> tag performance worse than Flash
rdar://problem/7982458
Created attachment 56851 [details] Patch
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.
Created attachment 56852 [details] Patch
This time, with less extraneous vcproj file churn.
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.
Created attachment 56859 [details] Patch
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.
Created attachment 56860 [details] Patch
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 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 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 on attachment 56852 [details] Patch Remove flags set on the wrong patch
(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!
Committed r60094: <http://trac.webkit.org/changeset/60094>
Roll-out changes; build broke.
Created attachment 56955 [details] Patch
Committed r60110: <http://trac.webkit.org/changeset/60110>