|
Description
Adam Roben (:aroben)
2010-01-22 08:56:35 PST
https://bugs.webkit.org/show_bug.cgi?id=34007 makes the performance much better, though it still isn't great. I am about to attach a number of patches to address this bug. Each are interdependent on the others, and the build will likely fail unless all are submitted at once. Created attachment 54499 [details]
Patch which breaks up QTMovieWin into QTMovieWin and QTMovie
Created attachment 54500 [details]
New classes in QTMovieWin
Created attachment 54501 [details]
Changes to MediaPlayerPrivateQuickTimeWin
Created attachment 54502 [details]
New class MediaPlayerPrivateQuickTimeVisualContext
Created attachment 54504 [details]
New class MediaPlayerPrivateTaskTimer
Created attachment 54505 [details]
Added MediaPlayerPrivateQuickTimeWin support to MediaPlayer
Created attachment 54506 [details]
Added cancelCallOnMainThread
Created attachment 54507 [details]
ChangeLog changes.
Created attachment 54508 [details]
VCProj project file changes
The proposed patches have now all been added. The bug as a whole is ready for review. Created attachment 54512 [details]
Patch which breaks up QTMovieWin into QTMovieWin and QTMovie
Corrected index prefix of patch.
Created attachment 54513 [details]
New classes in QTMovieWin
Corrected index prefix of patch.
Created attachment 54514 [details]
Changes to MediaPlayerPrivateQuickTimeWin
Corrected index prefix of patch.
Created attachment 54515 [details]
New class MediaPlayerPrivateQuickTimeVisualContext
Corrected index prefix of patch.
Created attachment 54516 [details]
New class MediaPlayerPrivateTaskTimer
Corrected index prefix of patch.
Created attachment 54517 [details]
Added MediaPlayerPrivateQuickTimeWin support to MediaPlayer
Corrected index prefix of patch.
Created attachment 54518 [details]
Part 3: JavaScriptCore: Added cancelCallOnMainThread
Corrected index prefix of patch.
Created attachment 54519 [details]
ChangeLog changes.
Corrected index prefix of patch.
Attachment 54517 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/MediaPlayer.cpp:50: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 1 files
If any of these errors are false positives, please file a bug against check-webkit-style.
It's going to be really hard to review this. Is it possible to make a set of incremental patches, each of which can stand on its own? It looks like these patches introduce dependencies on CoreVideo. But CoreVideo isn't included in WebKitSupportLibrary, so this won't work. The calls to CVGetCurrentHostTime and CVGetHostClockFrequency could be moved into QTMovieWin, which links against CVClient.lib, included in the QuickTime SDK. I'll update the patches, removing the dependency on CoreVideo in WebKit. (In reply to comment #25) > The calls to CVGetCurrentHostTime and CVGetHostClockFrequency could be moved > into QTMovieWin, which links against CVClient.lib, included in the QuickTime > SDK. I'll update the patches, removing the dependency on CoreVideo in WebKit. Oh, I didn't realize CoreVideo was part of the QuickTime SDK. I think the only reason we have a separate QTMovieWin.dll is to work around conflicts between QuickTime's CF and the one used by WebKit. If that isn't an issue here, then having WebKit link to CoreVideo seems fine. (But your solution sounds like it will work, too.) (In reply to comment #26) > Oh, I didn't realize CoreVideo was part of the QuickTime SDK. I think the only > reason we have a separate QTMovieWin.dll is to work around conflicts between > QuickTime's CF and the one used by WebKit. If that isn't an issue here, then > having WebKit link to CoreVideo seems fine. Unfortunately, QuickTime doesn't link against the same CoreVideo that we would, so there's a similar situation there to the CF one. The only remaining issue is one of constants defined in CoreVideo headers. Right now, we're doing things like: if (buffer.pixelFormat() == kCVPixelFormat_32ARGB). We could convert those checks into things like: if (buffer.pixelFormatIs32ARGB()) ...and be in the clear there too. Created attachment 54578 [details] Part 1.1: QTMovieWin: The breakup of QTMovieWin In this patch, QTMovie.cpp/h are new files. In the obseleted patch <https://bugs.webkit.org/attachment.cgi?id=54512&action=edit> those files are diffed as modified copies of QTMovieWin.cpp/h. The old patch may be easier to read while reviewing. Created attachment 54580 [details]
Part 2: QTMovieWin: New Classes QTPixelBuffer, QTCFDictionary, and QTMovieVisualContext
Created attachment 54581 [details]
Part 1.2: WebCore: The breakup of MediaPlayerPrivateQuickTimeWin
(In reply to comment #28) > Created an attachment (id=54578) [details] > Part 1.1: QTMovieWin: The breakup of QTMovieWin > > In this patch, QTMovie.cpp/h are new files. In the obseleted patch > <https://bugs.webkit.org/attachment.cgi?id=54512&action=edit> those files are > diffed as modified copies of QTMovieWin.cpp/h. The old patch may be easier to > read while reviewing. Why not do it that way in this patch, too? Not only does it ease reviewing, but it preserves the code's history in Subversion. Created attachment 54582 [details]
Part 4: WebCore: New class MediaPlayerPrivateQuickTimeVisualContext
Created attachment 54583 [details]
Part 5: WebCore: MediaPlayer adds MediaPlayerQuickTimeVisualContext to its list of MediaPlayerPrivates
Created attachment 54584 [details]
Part 6: VCProj project file changes
Created attachment 54594 [details]
Part 1.1: QTMovieWin: The breakup of QTMovieWin
QTMovie.cpp/h are back to being modified copies of QTMovieWin.cpp/h.
Created attachment 54595 [details]
Part 4: WebCore: New class MediaPlayerPrivateQuickTimeVisualContext
MediaPlayerPrivateQuickTimeVisualContext.cpp/h are back to being modified copies of MediaPlayerPrivateQuickTimeWin.cpp/h
Attachment 54580 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/QTMovieVisualContext.cpp:252: Could not find a newline character at the end of the file. [whitespace/ending_newline] [5]
WebCore/platform/graphics/win/QTPixelBuffer.cpp:30: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 54594 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/QTMovie.cpp:32: Alphabetical sorting problem. [build/include_order] [4]
WebCore/platform/graphics/win/QTMovieWin.cpp:32: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 54595 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.h:26: #ifndef header guard has wrong style, please use: MediaPlayerPrivateQuickTimeVisualContext_h [build/header_guard] [5]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:29: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:31: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 3 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 54599 [details]
Part 2: QTMovieWin: New Classes QTPixelBuffer, QTCFDictionary, and QTMovieVisualContext
Fixed style errors.
Created attachment 54600 [details]
Part 1.1: QTMovieWin: The breakup of QTMovieWin
Fixed style errors.
Created attachment 54601 [details]
Part 4: WebCore: New class MediaPlayerPrivateQuickTimeVisualContext
Fixed style errors.
Okay, the patches should finally be back to review-ready. Simon, I've structured the patches so that each "Part" is intra-dependent and only depends on previous parts. You should be able to review them in order. Created attachment 54602 [details]
Part 3: JavaScriptCore: Added cancelCallOnMainThread
Updated the patch so that it will (hopefully) succeed against the latest revision of JavaScriptCore.
Comment on attachment 54602 [details]
Part 3: JavaScriptCore: Added cancelCallOnMainThread
It would be cleaner if callOnMainThread returned some kind of unique ID that you could pass to cancelCallOnMainThread, but perhaps that would be a more intrusive change. This seems like an ok approach though.
r=me (assuming there are appropriate ChangeLog entries with the other chunks; or it's ok to land this standalone w/ its own ChangeLog entry).
Comment on attachment 54581 [details]
Part 1.2: WebCore: The breakup of MediaPlayerPrivateQuickTimeWin
r=me assuming appropriate ChangeLog entry.
Comment on attachment 54519 [details]
ChangeLog changes.
If you plan to land these separately, I suggest including them in the individual patches. Also, you should have the title of the bug above the bugzilla bug URL in our ChangeLog format.
Comment on attachment 54584 [details] Part 6: VCProj project file changes > -<?xml version="1.0" encoding="Windows-1252"?> > +<?xml version="1.0" encoding="windows-1251"?> > Please check with aroben or sfalken to see if the encoding change is significant. rs=me assuming this change is OK. Comment on attachment 54601 [details]
Part 4: WebCore: New class MediaPlayerPrivateQuickTimeVisualContext
It looks like in this patch you have both renamed the file and changed the code? If so it will be much easier to review if you svn mv it in one patch and changed the code in a follow up patch.
(In reply to comment #48) > (From update of attachment 54584 [details]) > > -<?xml version="1.0" encoding="Windows-1252"?> > > +<?xml version="1.0" encoding="windows-1251"?> > > > Please check with aroben or sfalken to see if the encoding change is > significant. > > rs=me assuming this change is OK. Yes, this is fine. Comment on attachment 54599 [details] Part 2: QTMovieWin: New Classes QTPixelBuffer, QTCFDictionary, and QTMovieVisualContext > Index: WebCore/platform/graphics/win/QTCFDictionary.cpp > =================================================================== > --- WebCore/platform/graphics/win/QTCFDictionary.cpp (revision 0) > +++ WebCore/platform/graphics/win/QTCFDictionary.cpp (revision 0) > @@ -0,0 +1,60 @@ > +/* > + * Copyright (C) 2007, 2008, 2009, 2010 Apple, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE COMPUTER, INC. OR > + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY > + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#include "config.h" > + > +#include "QTCFDictionary.h" > + > +#include <CFData.h> > +#include <windows.h> > + > +CFDataRef QTCFPropertyListCreateXMLData(CFAllocatorRef allocator, CFPropertyListRef propertyList) > +{ > + > + typedef CFDataRef (* pfnCFPropertyListCreateXMLData)(CFAllocatorRef allocator, CFPropertyListRef propertyList); > + static pfnCFPropertyListCreateXMLData pCFPropertyListCreateXMLData = 0; > + if (!pCFPropertyListCreateXMLData) { > + HMODULE qtcfDLL = LoadLibraryW(L"QTCF.dll"); > + if (qtcfDLL) > + pCFPropertyListCreateXMLData = reinterpret_cast<pfnCFPropertyListCreateXMLData>(GetProcAddress(qtcfDLL, "QTCF_CFPropertyListCreateXMLData")); > + } > + > + if (pCFPropertyListCreateXMLData) > + return pCFPropertyListCreateXMLData(allocator, propertyList); > + return 0; > +} > + > +CFDictionaryRef QTCFDictionaryCreateCopyWithDataCallback(CFAllocatorRef allocator, CFDictionaryRef dictionary, QTCFDictonaryCreateFromDataCallback callback) > +{ > + ASSERT(dictionary); > + ASSERT(callback); > + > + CFDataRef data = QTCFPropertyListCreateXMLData(kCFAllocatorDefault, dictionary); > + CFDictionaryRef outputDictionary = callback(allocator, CFDataGetBytePtr(data), CFDataGetLength(data)); > + if (data) > + CFRelease(data); > + > + return outputDictionary; > +} > Index: WebCore/platform/graphics/win/QTCFDictionary.h > =================================================================== > --- WebCore/platform/graphics/win/QTCFDictionary.h (revision 0) > +++ WebCore/platform/graphics/win/QTCFDictionary.h (revision 0) > @@ -0,0 +1,44 @@ > +/* > + * Copyright (C) 2007, 2008, 2009, 2010 Apple, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE COMPUTER, INC. OR > + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY > + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#ifndef QTCFDictionary_h > +#define QTCFDictionary_h > + > +#ifdef QTMOVIEWIN_EXPORTS > +#define QTMOVIEWIN_API __declspec(dllexport) > +#else > +#define QTMOVIEWIN_API __declspec(dllimport) > +#endif > + > +#include <CoreFoundation/CFBase.h> > + > +typedef const struct __CFDictionary * CFDictionaryRef; > +typedef const struct __CFAllocator * CFAllocatorRef; > + > +typedef CFDictionaryRef (* QTCFDictonaryCreateFromDataCallback)(CFAllocatorRef, const UInt8*, CFIndex); > + > +QTMOVIEWIN_API CFDictionaryRef QTCFDictionaryCreateCopyWithDataCallback(CFAllocatorRef, CFDictionaryRef, QTCFDictonaryCreateFromDataCallback); > + > +#endif > Index: WebCore/platform/graphics/win/QTMovieVisualContext.cpp > =================================================================== > --- WebCore/platform/graphics/win/QTMovieVisualContext.cpp (revision 0) > +++ WebCore/platform/graphics/win/QTMovieVisualContext.cpp (revision 0) > @@ -0,0 +1,252 @@ > +/* > + * Copyright (C) 2007, 2008, 2009, 2010 Apple, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE COMPUTER, INC. OR > + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY > + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > +#include "config.h" > + > +#include "QTMovieVisualContext.h" > + > +#include "QTMovieTask.h" > +#include <CVBase.h> > +#include <CVHostTime.h> > +#include <ImageCompression.h> > +#include <Movies.h> > +#include <d3d9types.h> > +#include <windows.h> > + > +struct QTCVTimeStamp { > + CVTimeStamp t; > +}; > + > +class QTMovieVisualContextPriv { > +public: > + QTMovieVisualContextPriv(QTMovieVisualContext* parent, QTMovieVisualContextClient* client, CFDictionaryRef options); > + ~QTMovieVisualContextPriv(); > + > + bool isImageAvailableForTime(const QTCVTimeStamp*); > + QTPixelBuffer imageForTime(const QTCVTimeStamp*); > + void task(); > + > + QTVisualContextRef get(); > + > + double getCurrentHostTime(); > + double getHostClockFrequency(); > + > + void setMovie(RefPtr<QTMovie>); > + RefPtr<QTMovie> movie() const; > + > + static void imageAvailableCallback(QTVisualContextRef visualContext, const CVTimeStamp *timeStamp, void *refCon); > + > +private: > + QTMovieVisualContext* m_parent; > + QTMovieVisualContextClient* m_client; > + QTVisualContextRef m_visualContext; > + RefPtr<QTMovie> m_movie; > + > +}; > + > +QTMovieVisualContextPriv::QTMovieVisualContextPriv(QTMovieVisualContext* parent, QTMovieVisualContextClient* client, CFDictionaryRef options) > + : m_parent(parent) > + , m_client(client) > + , m_visualContext(0) > +{ > + HMODULE qtmlDLL = ::LoadLibraryW(L"QTMLClient.dll"); > + if (qtmlDLL) { > + typedef OSStatus ( __cdecl *pfnQTPixelBufferContextCreate)(CFAllocatorRef, CFDictionaryRef, QTVisualContextRef*); > + pfnQTPixelBufferContextCreate pPixelBufferContextCreate = reinterpret_cast<pfnQTPixelBufferContextCreate>(GetProcAddress(qtmlDLL, "QTPixelBufferContextCreate")); > + if (pPixelBufferContextCreate) { > + OSStatus status = pPixelBufferContextCreate(kCFAllocatorDefault, options, &m_visualContext); > + if (status == noErr && m_visualContext) > + QTVisualContextSetImageAvailableCallback(m_visualContext, &QTMovieVisualContextPriv::imageAvailableCallback, static_cast<void*>(this)); > + } > + } > +} > + > +QTMovieVisualContextPriv::~QTMovieVisualContextPriv() > +{ > +} > + > +bool QTMovieVisualContextPriv::isImageAvailableForTime(const QTCVTimeStamp* timeStamp) > +{ > + if (!m_visualContext) > + return false; > + > + return QTVisualContextIsNewImageAvailable(m_visualContext, reinterpret_cast<const CVTimeStamp*>(timeStamp)); > +} > + > +QTPixelBuffer QTMovieVisualContextPriv::imageForTime(const QTCVTimeStamp* timeStamp) > +{ > + QTPixelBuffer pixelBuffer; > + if (m_visualContext) { > + CVImageBufferRef newImage = 0; > + OSStatus status = QTVisualContextCopyImageForTime(m_visualContext, kCFAllocatorDefault, reinterpret_cast<const CVTimeStamp*>(timeStamp), &newImage); > + if (status == noErr) { > + pixelBuffer.set(newImage); > + CVPixelBufferRelease(newImage); > + } > + } > + return pixelBuffer; > +} > + > +void QTMovieVisualContextPriv::task() > +{ > + if (m_visualContext) > + QTVisualContextTask(m_visualContext); > +} > + > +QTVisualContextRef QTMovieVisualContextPriv::get() > +{ > + return m_visualContext; > +} > + > +void QTMovieVisualContextPriv::setMovie(RefPtr<QTMovie> movie) > +{ > + if (m_movie) { > + SetMovieVisualContext(m_movie->getMovieHandle(), 0); > + m_movie = 0; > + } > + > + if (movie) > + OSStatus status = SetMovieVisualContext(movie->getMovieHandle(), m_visualContext); > + > + m_movie = movie; > +} > + > +RefPtr<QTMovie> QTMovieVisualContextPriv::movie() const > +{ > + return m_movie; > +} > + > +void QTMovieVisualContextPriv::imageAvailableCallback(QTVisualContextRef visualContext, const CVTimeStamp *timeStamp, void *refCon) > +{ > + if (!refCon) > + return; > + > + QTMovieVisualContextPriv* vc = static_cast<QTMovieVisualContextPriv*>(refCon); > + if (!vc->m_client) > + return; > + > + vc->m_client->imageAvailableForTime(reinterpret_cast<const QTCVTimeStamp*>(timeStamp)); > +} > + > +static OSStatus SetNumberValue(CFMutableDictionaryRef inDict, CFStringRef inKey, SInt32 inValue) > +{ > + CFNumberRef number; > + > + number = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &inValue); > + if (!number) > + return coreFoundationUnknownErr; > + > + CFDictionarySetValue(inDict, inKey, number); > + > + CFRelease(number); > + > + return noErr; > +} > + > +CFDictionaryRef QTMovieVisualContext::getCGImageOptions() > +{ > + static CFDictionaryRef options = 0; > + > + if (!options) { > + CFMutableDictionaryRef pixelBufferOptions = 0; > + CFMutableDictionaryRef visualContextOptions = 0; > + OSStatus status = noErr; > + > + // Pixel Buffer attributes > + pixelBufferOptions = CFDictionaryCreateMutable(kCFAllocatorDefault, 0, > + &kCFTypeDictionaryKeyCallBacks, > + &kCFTypeDictionaryValueCallBacks); > + > + // Use the k32BGRAPixelFormat, as QuartzCore will be able to use the pixels directly, > + // without needing an aditional copy or rendering pass:s > + SetNumberValue(pixelBufferOptions, kCVPixelBufferPixelFormatTypeKey, k32BGRAPixelFormat); > + > + // alignment > + SetNumberValue(pixelBufferOptions, kCVPixelBufferBytesPerRowAlignmentKey, 16); > + > + // compatability > + SetNumberValue(pixelBufferOptions, kCVPixelBufferCGImageCompatibilityKey, 1); > + > + // QT Visual Context attributes > + visualContextOptions = CFDictionaryCreateMutable(kCFAllocatorDefault, 0, > + &kCFTypeDictionaryKeyCallBacks, > + &kCFTypeDictionaryValueCallBacks); > + > + // set the pixel buffer attributes for the visual context > + CFDictionarySetValue(visualContextOptions, > + kQTVisualContextPixelBufferAttributesKey, > + pixelBufferOptions); > + > + if (pixelBufferOptions) > + CFRelease(pixelBufferOptions); > + > + options = visualContextOptions; > + } > + > + return options; > +} > + > +QTMovieVisualContext::QTMovieVisualContext(QTMovieVisualContextClient* client, CFDictionaryRef options) > + : m_private(new QTMovieVisualContextPriv(this, client, options)) > +{ > +} > + > +QTMovieVisualContext::~QTMovieVisualContext() > +{ > +} > + > +bool QTMovieVisualContext::isImageAvailableForTime(const QTCVTimeStamp* timeStamp) > +{ > + return m_private->isImageAvailableForTime(timeStamp); > +} > + > +QTPixelBuffer QTMovieVisualContext::imageForTime(const QTCVTimeStamp* timeStamp) > +{ > + return m_private->imageForTime(timeStamp); > +} > + > +void QTMovieVisualContext::task() > +{ > + m_private->task(); > +} > + > +QTVisualContextRef QTMovieVisualContext::get() > +{ > + return m_private->get(); > +} > + > +void QTMovieVisualContext::setMovie(RefPtr<QTMovie> movie) > +{ > + m_private->setMovie(movie); > +} > + > +RefPtr<QTMovie> QTMovieVisualContext::movie() const > +{ > + return m_private->movie(); > +} > + > +double QTMovieVisualContext::currentHostTime() > +{ > + return CVGetCurrentHostTime() / CVGetHostClockFrequency(); > +} > Index: WebCore/platform/graphics/win/QTMovieVisualContext.h > =================================================================== > --- WebCore/platform/graphics/win/QTMovieVisualContext.h (revision 0) > +++ WebCore/platform/graphics/win/QTMovieVisualContext.h (revision 0) > @@ -0,0 +1,81 @@ > +/* > + * Copyright (C) 2007, 2008, 2009, 2010 Apple, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE COMPUTER, INC. OR > + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY > + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#ifndef QTMovieVisualContext_h > +#define QTMovieVisualContext_h > + > +#ifdef QTMOVIEWIN_EXPORTS > +#define QTMOVIEWIN_API __declspec(dllexport) > +#else > +#define QTMOVIEWIN_API __declspec(dllimport) > +#endif > + > +#include "QTMovie.h" > +#include "QTMovieTask.h" > +#include "QTPixelBuffer.h" > +#include <WTF/OwnPtr.h> > +#include <WTF/RefCounted.h> > + > +typedef struct OpaqueQTVisualContext* QTVisualContextRef; > + > +// QTCVTimeStamp is a struct containing only a CVTimeStamp. This is to > +// work around the inability of CVTimeStamp to be forward declared, in > +// addition to it being declared in different header files when building > +// the QTMovieWin and WebCore projects. > +struct QTCVTimeStamp; > + > +class QTMovieVisualContextClient { > +public: > + virtual void imageAvailableForTime(const QTCVTimeStamp*) = 0; > +}; > + > +class QTMOVIEWIN_API QTMovieVisualContext : public RefCounted<QTMovieVisualContext> { > +public: > + QTMovieVisualContext(QTMovieVisualContextClient*, CFDictionaryRef options = 0); > + ~QTMovieVisualContext(); > + > + bool isImageAvailableForTime(const QTCVTimeStamp*); > + QTPixelBuffer imageForTime(const QTCVTimeStamp*); > + void task(); > + > + QTVisualContextRef get(); > + > + double getCurrentHostTime(); > + double getHostClockFrequency(); > + > + void setMovie(RefPtr<QTMovie>); > + RefPtr<QTMovie> movie() const; > + > + static CFDictionaryRef getCGImageOptions(); > + static double currentHostTime(); > + > +protected: > + void setupVisualContext(); > + > + friend class QTMovieVisualContextPriv; > + OwnPtr<QTMovieVisualContextPriv> m_private; > +}; > + > +#endif > Index: WebCore/platform/graphics/win/QTPixelBuffer.cpp > =================================================================== > --- WebCore/platform/graphics/win/QTPixelBuffer.cpp (revision 0) > +++ WebCore/platform/graphics/win/QTPixelBuffer.cpp (revision 0) > @@ -0,0 +1,209 @@ > +/* > + * Copyright (C) 2007, 2008, 2009, 2010 Apple, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE COMPUTER, INC. OR > + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY > + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > +#include "config.h" > + > +#include "QTPixelBuffer.h" > + > +#include <CFString.h> > +#include <CGColorSpace.h> > +#include <CGImage.h> > +#include <CVPixelBuffer.h> > +#include <QuickDraw.h> > + > +QTPixelBuffer::QTPixelBuffer() : m_pixelBuffer(0) {} > + > +QTPixelBuffer::QTPixelBuffer(const QTPixelBuffer& p) : m_pixelBuffer(p.m_pixelBuffer) > +{ > + CVPixelBufferRetain(m_pixelBuffer); > +} > + > +QTPixelBuffer::QTPixelBuffer(CVPixelBufferRef ref) : m_pixelBuffer(ref) > +{ > + CVPixelBufferRetain(m_pixelBuffer); > +} > + > +QTPixelBuffer::~QTPixelBuffer() > +{ > + clear(); > +} > + > +QTPixelBuffer& QTPixelBuffer::operator=(const QTPixelBuffer& p) > +{ > + set(p.m_pixelBuffer); > + return *this; > +} > + > +void QTPixelBuffer::set(CVPixelBufferRef ref) > +{ > + CVPixelBufferRetain(ref); > + CVPixelBufferRelease(m_pixelBuffer); > + m_pixelBuffer = ref; > +} > + > +CVPixelBufferRef QTPixelBuffer::get() > +{ > + return m_pixelBuffer; > +} > + > +void QTPixelBuffer::clear() > +{ > + CVPixelBufferRelease(m_pixelBuffer); > + m_pixelBuffer = 0; > +} > + > +CVReturn QTPixelBuffer::lockBaseAddress() > +{ > + return CVPixelBufferLockBaseAddress(m_pixelBuffer, 0); > +} > + > +CVReturn QTPixelBuffer::unlockBaseAddress() > +{ > + return CVPixelBufferUnlockBaseAddress(m_pixelBuffer, 0); > +} > + > +size_t QTPixelBuffer::width() > +{ > + return CVPixelBufferGetWidth(m_pixelBuffer); > +} > + > +size_t QTPixelBuffer::height() > +{ > + return CVPixelBufferGetHeight(m_pixelBuffer); > +} > + > +unsigned long QTPixelBuffer::pixelFormatType() > +{ > + OSType pixelFormatType = CVPixelBufferGetPixelFormatType(m_pixelBuffer); > + return pixelFormatType; > +} > + > +bool QTPixelBuffer::pixelFormatIs32ARGB() > +{ > + return CVPixelBufferGetPixelFormatType(m_pixelBuffer) == k32ARGBPixelFormat; > +} > + > +bool QTPixelBuffer::pixelFormatIs32BGRA() > +{ > + return CVPixelBufferGetPixelFormatType(m_pixelBuffer) == k32BGRAPixelFormat; > +} > + > +void* QTPixelBuffer::baseAddress() > +{ > + return CVPixelBufferGetBaseAddress(m_pixelBuffer); > +} > + > +size_t QTPixelBuffer::bytesPerRow() > +{ > + return CVPixelBufferGetBytesPerRow(m_pixelBuffer); > +} > + > +size_t QTPixelBuffer::dataSize() > +{ > + return CVPixelBufferGetDataSize(m_pixelBuffer); > +} > + > + > +bool QTPixelBuffer::isPlanar() > +{ > + return CVPixelBufferIsPlanar(m_pixelBuffer); > +} > + > +size_t QTPixelBuffer::planeCount() > +{ > + return CVPixelBufferGetPlaneCount(m_pixelBuffer); > +} > + > +size_t QTPixelBuffer::widthOfPlane(size_t plane) > +{ > + return CVPixelBufferGetWidthOfPlane(m_pixelBuffer, plane); > +} > + > +size_t QTPixelBuffer::heightOfPlane(size_t plane) > +{ > + return CVPixelBufferGetHeightOfPlane(m_pixelBuffer, plane); > +} > + > +void* QTPixelBuffer::baseAddressOfPlane(size_t plane) > +{ > + return CVPixelBufferGetBaseAddressOfPlane(m_pixelBuffer, plane); > +} > + > +size_t QTPixelBuffer::bytesPerRowOfPlane(size_t plane) > +{ > + return CVPixelBufferGetBytesPerRowOfPlane(m_pixelBuffer, plane); > +} > + > +void QTPixelBuffer::getExtendedPixels(size_t* left, size_t* right, size_t* top, size_t* bottom) > +{ > + return CVPixelBufferGetExtendedPixels(m_pixelBuffer, left, right, top, bottom); > +} > + > +CFDictionaryRef QTPixelBuffer::attachments() > +{ > + CFDictionaryRef propogated = CVBufferGetAttachments(m_pixelBuffer, kCVAttachmentMode_ShouldPropagate); > + return propogated; > +} > + > +void QTPixelBuffer::retainCallback(void* refcon) > +{ > + CVPixelBufferRetain(static_cast<CVPixelBufferRef>(refcon)); > +} > + > +void QTPixelBuffer::releaseCallback(void* refcon) > +{ > + CVPixelBufferRelease(static_cast<CVPixelBufferRef>(refcon)); > +} > + > +void QTPixelBuffer::imageQueueReleaseCallback(unsigned int type, uint64_t id, void* refcon) > +{ > + CVPixelBufferRelease(static_cast<CVPixelBufferRef>(refcon)); > +} > + > +void QTPixelBuffer::dataProviderReleaseBytePointerCallback(void* refcon, const void* pointer) > +{ > + CVPixelBufferUnlockBaseAddress(static_cast<CVPixelBufferRef>(refcon), 0); > +} > + > +const void* QTPixelBuffer::dataProviderGetBytePointerCallback(void* refcon) > +{ > + CVPixelBufferLockBaseAddress(static_cast<CVPixelBufferRef>(refcon), 0); > + return CVPixelBufferGetBaseAddress(static_cast<CVPixelBufferRef>(refcon)); > +} > + > +size_t QTPixelBuffer::dataProviderGetBytesAtPositionCallback(void* refcon, void* buffer, size_t position, size_t count) > +{ > + char* data = (char*)CVPixelBufferGetBaseAddress(static_cast<CVPixelBufferRef>(refcon)); > + size_t size = CVPixelBufferGetDataSize(static_cast<CVPixelBufferRef>(refcon)); > + if (size - position < count) > + count = size - position; > + > + memcpy(buffer, data+position, count); > + return count; > +} > + > +void QTPixelBuffer::dataProviderReleaseInfoCallback(void* refcon) > +{ > + CVPixelBufferRelease(static_cast<CVPixelBufferRef>(refcon)); > +} > Index: WebCore/platform/graphics/win/QTPixelBuffer.h > =================================================================== > --- WebCore/platform/graphics/win/QTPixelBuffer.h (revision 0) > +++ WebCore/platform/graphics/win/QTPixelBuffer.h (revision 0) > @@ -0,0 +1,97 @@ > +/* > + * Copyright (C) 2007, 2008, 2009, 2010 Apple, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE COMPUTER, INC. OR > + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY > + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#ifndef QTPixelBuffer_h > +#define QTPixelBuffer_h > + > +#ifdef QTMOVIEWIN_EXPORTS > +#define QTMOVIEWIN_API __declspec(dllexport) > +#else > +#define QTMOVIEWIN_API __declspec(dllimport) > +#endif > + > +#include <stdint.h> > +#include <wtf/RefCounted.h> > + > +typedef struct __CVBuffer *CVBufferRef; > +typedef CVBufferRef CVPixelBufferRef; > +typedef struct CGImage* CGImageRef; > +typedef int32_t CVReturn; > +typedef const struct __CFDictionary * CFDictionaryRef; > + > +// QTPixelBuffer wraps QuickTime's implementation of CVPixelBuffer, so its functions are > +// safe to call within WebKit. > +class QTMOVIEWIN_API QTPixelBuffer : RefCounted<QTPixelBuffer> { > +public: > + QTPixelBuffer(); > + QTPixelBuffer(const QTPixelBuffer&); > + QTPixelBuffer(CVPixelBufferRef); > + QTPixelBuffer& operator=(const QTPixelBuffer&); > + ~QTPixelBuffer(); > + > + void set(CVPixelBufferRef); > + CVPixelBufferRef get(); > + void clear(); > + > + CVReturn lockBaseAddress(); > + CVReturn unlockBaseAddress(); > + size_t width(); > + size_t height(); > + > + unsigned long pixelFormatType(); > + bool pixelFormatIs32ARGB(); > + bool pixelFormatIs32BGRA(); > + void* baseAddress(); > + size_t bytesPerRow(); > + size_t dataSize(); > + > + bool isPlanar(); > + size_t planeCount(); > + size_t widthOfPlane(size_t); > + size_t heightOfPlane(size_t); > + void* baseAddressOfPlane(size_t); > + size_t bytesPerRowOfPlane(size_t); > + > + void getExtendedPixels(size_t* left, size_t* right, size_t* top, size_t* bottom); > + CFDictionaryRef attachments(); > + > + // Generic CFRetain/CFRelease callbacks > + static void releaseCallback(void* refcon); > + static void retainCallback(void* refcon); > + > + // CAImageQueue callbacks > + static void imageQueueReleaseCallback(unsigned int type, uint64_t id, void* refcon); > + > + // CGDataProvider callbacks > + static void dataProviderReleaseBytePointerCallback(void* refcon, const void* pointer); > + static const void* dataProviderGetBytePointerCallback(void* refcon); > + static size_t dataProviderGetBytesAtPositionCallback(void* refcon, void* buffer, size_t position, size_t count); > + static void dataProviderReleaseInfoCallback(void* refcon); > + > +private: > + CVPixelBufferRef m_pixelBuffer; > +}; > + > +#endif WebCore/platform/graphics/win/QTMovieVisualContext.cpp:50 + QTVisualContextRef get(); I think this should be renamed to visualContext() or something other than get(). WebCore/platform/graphics/win/QTMovieVisualContext.cpp:54 + We mostly use get for functions that have out parameters. This should be currentHostTime() and hostClockFrequency(). Can these be const? WebCore/platform/graphics/win/QTMovieVisualContext.cpp:56 + RefPtr<QTMovie> movie() const; No need for this to return a RefPtr. WebCore/platform/graphics/win/QTMovieVisualContext.cpp:55 + void setMovie(RefPtr<QTMovie>); This should take a PassRefPtr to reduce ref churn. WebCore/platform/graphics/win/QTMovieVisualContext.cpp:73 + HMODULE qtmlDLL = ::LoadLibraryW(L"QTMLClient.dll"); Should this be a static variable? Is it OK to load the library everytime a QTMovieVisualContextPriv object is created? WebCore/platform/graphics/win/QTMovieVisualContext.cpp:74 + if (qtmlDLL) { An early return here would reduce indentation. WebCore/platform/graphics/win/QTMovieVisualContext.cpp:105 + CVPixelBufferRelease(newImage); Could QTPixelBuffer have an "adopt" member function that would take a reference? If so the CVPixelBufferRelease call could be omitted. WebCore/platform/graphics/win/QTMovieVisualContext.cpp:134 + Yup, looks like this could take a PassRefPtr. WebCore/platform/graphics/win/QTMovieVisualContext.cpp:138 + } And this could return a QTMovie * instead of a RefPtr. WebCore/platform/graphics/win/QTMovieVisualContext.cpp:47 + QTPixelBuffer imageForTime(const QTCVTimeStamp*); Can these be const? WebCore/platform/graphics/win/QTMovieVisualContext.cpp:123 + { You could return early here if (movie == m_movie). (Especially when movie is a PassRefPtr). WebCore/platform/graphics/win/QTMovieVisualContext.h:63 + QTVisualContextRef get(); Again, a better name for this would be visualContext(). WebCore/platform/graphics/win/QTMovieVisualContext.h:66 + double getHostClockFrequency(); Same comments here as in QTMovieVisualContextPrivate. WebCore/platform/graphics/win/QTMovieVisualContext.h:70 + Same comments here as in QTMovieVisualContextPrivate. WebCore/platform/graphics/win/QTPixelBuffer.cpp:35 + QTPixelBuffer::QTPixelBuffer() : m_pixelBuffer(0) {} member initializers should go on a separate row. WebCore/platform/graphics/win/QTPixelBuffer.cpp:37 + QTPixelBuffer::QTPixelBuffer(const QTPixelBuffer& p) : m_pixelBuffer(p.m_pixelBuffer) Ditto. WebCore/platform/graphics/win/QTPixelBuffer.cpp:42 + QTPixelBuffer::QTPixelBuffer(CVPixelBufferRef ref) : m_pixelBuffer(ref) Ditto. WebCore/platform/graphics/win/QTPixelBuffer.cpp:68 + } Should be called "pixelBuffer" or something other than get. WebCore/platform/graphics/win/QTPixelBuffer.cpp:74 + } Does CVPixelBufferRelease work if m_pixelBuffer is 0? WebCore/platform/graphics/win/QTPixelBuffer.h:62 + Can width and height be const? WebCore/platform/graphics/win/QTPixelBuffer.h:69 + Can these be const? (Or at least some of them). WebCore/platform/graphics/win/QTPixelBuffer.h:76 + Const? WebCore/platform/graphics/win/QTPixelBuffer.h:77 + void getExtendedPixels(size_t* left, size_t* right, size_t* top, size_t* bottom); Maybe references would be better than pointers here? WebCore/platform/graphics/win/QTPixelBuffer.cpp:166 + return propogated; Can just merge these into a single line. WebCore/platform/graphics/win/QTPixelBuffer.h:51 + QTPixelBuffer& operator=(const QTPixelBuffer&); Come to think of it, I don't think ref counted objects should be copyable. Are the copy constructor and copy-assignment operators really needed? WebCore/platform/graphics/win/QTPixelBuffer.h:82 + static void retainCallback(void* refcon); What are these used for? Patch looks good overall, but I'd like to see my comments addressed in a new patch. Comment on attachment 54600 [details] Part 1.1: QTMovieWin: The breakup of QTMovieWin WebCore/platform/graphics/win/QTMovie.cpp:266 + m_private->m_clients.remove(indexOfClient); Is this correct? Do you really never want to remove the first client? WebCore/platform/graphics/win/QTMovie.h:60 + class QTMOVIEWIN_API QTMovie : public RefCounted<QTMovie> { It seems like this class should be renamed something like QTMovieView or QTMovieGWorld. If you don't want to rename it in this round, please add a comment. WebCore/platform/graphics/win/QTMovieTask.cpp:46 + static QTMovieTask* s_sharedTask = new QTMovieTask(); This should use the DEFINE_GLOBAL macro. r=me but please consider those changes.WebCore/platform/graphics/win/QTMovieWin.h:75 + RefPtr<QTMovie> movie() const; We don't ever pass or return RefPtr's. Please see http://webkit.org/coding/RefPtr.html for more on that. (In reply to comment #52) > (From update of attachment 54600 [details]) > WebCore/platform/graphics/win/QTMovie.cpp:266 > + m_private->m_clients.remove(indexOfClient); > Is this correct? Do you really never want to remove the first client? You're right, that should be a ">=" > WebCore/platform/graphics/win/QTMovie.h:60 > + class QTMOVIEWIN_API QTMovie : public RefCounted<QTMovie> { > It seems like this class should be renamed something like QTMovieView or > QTMovieGWorld. If you don't want to rename it in this round, please add a > comment. I agree that QTMovieWin isn't the correct name, and that QTMovieGWorld would be better. I don't mind doing the re-naming this pass. > > WebCore/platform/graphics/win/QTMovieTask.cpp:46 > + static QTMovieTask* s_sharedTask = new QTMovieTask(); > This should use the DEFINE_GLOBAL macro. > > r=me but please consider those > changes.WebCore/platform/graphics/win/QTMovieWin.h:75 > + RefPtr<QTMovie> movie() const; > We don't ever pass or return RefPtr's. Please see > http://webkit.org/coding/RefPtr.html for more on that. It seems like the convention is to move the default constructor into protected: and add a static constructor method which returns a PassRefPtr. Should I do that as well to QTMovie, QTMovieGWorld, and QTMovieVisualContext? (In reply to comment #51) > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:50 > + QTVisualContextRef get(); > I think this should be renamed to visualContext() or something other than > get(). Originally, I didn't like the looks of foo->visualContext()->visualContext(). (If the class "Foo" had an accessor which returned a QTVisualContext*.) I considered "platformVisualContext()" or "rawVisualContext()" but didn't like any of those either. "get()" was a cop-out. How about "visualContextRef()"? > > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:54 > + > We mostly use get for functions that have out parameters. This should be > currentHostTime() and hostClockFrequency(). Can these be const? Better, they can be static. Also, I'll just combine those two calls into one which does the division and returns a CFTimeInterval. > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:56 > + RefPtr<QTMovie> movie() const; > No need for this to return a RefPtr. PassRefPtr instead. > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:55 > + void setMovie(RefPtr<QTMovie>); > This should take a PassRefPtr to reduce ref churn. Yup. > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:73 > + HMODULE qtmlDLL = ::LoadLibraryW(L"QTMLClient.dll"); > Should this be a static variable? Is it OK to load the library everytime a > QTMovieVisualContextPriv object is created? It absolutely can be. LoadLibrary won't do any extra work if the DLL is already loaded, but it should be static anyway. > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:74 > + if (qtmlDLL) { > An early return here would reduce indentation. > > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:105 > + CVPixelBufferRelease(newImage); > Could QTPixelBuffer have an "adopt" member function that would take a > reference? If so the CVPixelBufferRelease call could be omitted. Yep. > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:134 > + > Yup, looks like this could take a PassRefPtr. Ok. > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:138 > + } > And this could return a QTMovie * instead of a RefPtr. Why not a return PassRefPtr? > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:47 > + QTPixelBuffer imageForTime(const QTCVTimeStamp*); > Can these be const? Originally, I had thought that if they were, this->m_visualContext would be a "const CVPixelBufferRef", and I'd have to const_cast away the constness before passing it on to QTVisualContextIsImageAvailableForTime. Apparently, (I just tried) this compiles just fine without the const_cast. So, yes. :) However, calling QTVisualContextCopyImageForTime may change the internal state of the QTVisualContext, so it's not necessarily a "const" operation. > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:123 > + { > You could return early here if (movie == m_movie). (Especially when movie is a > PassRefPtr). Yep. > WebCore/platform/graphics/win/QTMovieVisualContext.h:63 > + QTVisualContextRef get(); > Again, a better name for this would be visualContext(). Yep. > WebCore/platform/graphics/win/QTMovieVisualContext.h:66 > + double getHostClockFrequency(); > Same comments here as in QTMovieVisualContextPrivate. Yep. > WebCore/platform/graphics/win/QTMovieVisualContext.h:70 > + > Same comments here as in QTMovieVisualContextPrivate. Yep. > WebCore/platform/graphics/win/QTPixelBuffer.cpp:35 > + QTPixelBuffer::QTPixelBuffer() : m_pixelBuffer(0) {} > member initializers should go on a separate row. Ok. > WebCore/platform/graphics/win/QTPixelBuffer.cpp:37 > + QTPixelBuffer::QTPixelBuffer(const QTPixelBuffer& p) : > m_pixelBuffer(p.m_pixelBuffer) > Ditto. Ok. > WebCore/platform/graphics/win/QTPixelBuffer.cpp:42 > + QTPixelBuffer::QTPixelBuffer(CVPixelBufferRef ref) : m_pixelBuffer(ref) > Ditto. Ok. > WebCore/platform/graphics/win/QTPixelBuffer.cpp:68 > + } > Should be called "pixelBuffer" or something other than get. Similarly, perhaps "pixelBufferRef()"? > WebCore/platform/graphics/win/QTPixelBuffer.cpp:74 > + } > Does CVPixelBufferRelease work if m_pixelBuffer is 0? Yep. > WebCore/platform/graphics/win/QTPixelBuffer.h:62 > + > Can width and height be const? Same as above, yes. > WebCore/platform/graphics/win/QTPixelBuffer.h:69 > + > Can these be const? (Or at least some of them). All of the "consty" ones can, yes. > WebCore/platform/graphics/win/QTPixelBuffer.h:76 > + > Const? Sure. > WebCore/platform/graphics/win/QTPixelBuffer.h:77 > + void getExtendedPixels(size_t* left, size_t* right, size_t* top, > size_t* bottom); > Maybe references would be better than pointers here? The references will just be turned into pointers when we call CVPixelBufferGetExtendedPixels. And if the caller only wanted to get the "left" extended pixel, they would still have to define and pass in all the other parameters anyway. > WebCore/platform/graphics/win/QTPixelBuffer.cpp:166 > + return propogated; > Can just merge these into a single line. Sure thing. (There used to be a CFShow() between those two lines, for debugging purposes.) > WebCore/platform/graphics/win/QTPixelBuffer.h:51 > + QTPixelBuffer& operator=(const QTPixelBuffer&); > Come to think of it, I don't think ref counted objects should be copyable. Are > the copy constructor and copy-assignment operators really needed? Nope. These could be moved into private: and unimplemented. > WebCore/platform/graphics/win/QTPixelBuffer.h:82 > + static void retainCallback(void* refcon); > What are these used for? Funny story. You can't pass a CVPixelBuffer ref created in QTMovieWin.dll to a CFRetain or CFRelease defined in CoreFoundation.dll. So whenever a retain or a release needs to be called on a CVPixelBufferRef, it needs to occur within QTMovieWin.dll. I defined three different types of retain/release callbacks in the QTPixelBuffer class: a straight CF-style retain/release callback that takes a single parameter, a CAImageQueue style release callback that takes a few other extraneous parameters, and a CGDataProvider style release callback. The CF-style retain/release callbacks are so you can put a CVPixelBufferRef in a CFArray (with the correct retain/release callbacks passed in during CFArray creation). The CAImageQueue callbacks are for use with CAImageQueues (which don't take CVPixelBuffers directly, but want a callback they can use to release the underlying pixel data). And the CGDataProvider callbacks are so the QTPixelBuffers can be used to create CGImages, backed by CGDataProviders, referencing the original pixel data in the CVPixelBuffer. > Patch looks good overall, but I'd like to see my comments addressed in a new > patch. Sure thing. I'll post a replacement patch. (In reply to comment #54) > (In reply to comment #51) > > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:50 > > + QTVisualContextRef get(); > > I think this should be renamed to visualContext() or something other than > > get(). > > Originally, I didn't like the looks of foo->visualContext()->visualContext(). > (If the class "Foo" had an accessor which returned a QTVisualContext*.) I > considered "platformVisualContext()" or "rawVisualContext()" but didn't like > any of those either. "get()" was a cop-out. > > How about "visualContextRef()"? I like that! > > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:56 > > + RefPtr<QTMovie> movie() const; > > No need for this to return a RefPtr. > > PassRefPtr instead. You don't want it to return a PassRefPtr, because then if someone does context->movie()->doSomething() that's going to ref and then deref the QTMovie object for no good reason. > > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:138 > > + } > > And this could return a QTMovie * instead of a RefPtr. > > Why not a return PassRefPtr? See my comment above. > > > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:47 > > + QTPixelBuffer imageForTime(const QTCVTimeStamp*); > > Can these be const? > > Originally, I had thought that if they were, this->m_visualContext would be a > "const CVPixelBufferRef", and I'd have to const_cast away the constness before > passing it on to QTVisualContextIsImageAvailableForTime. Apparently, (I just > tried) this compiles just fine without the const_cast. So, yes. :) > > However, calling QTVisualContextCopyImageForTime may change the internal state > of the QTVisualContext, so it's not necessarily a "const" operation. > Makes sense! > > WebCore/platform/graphics/win/QTMovieVisualContext.cpp:123 > > + { > > You could return early here if (movie == m_movie). (Especially when movie is a > > PassRefPtr). > > Yep. > > > WebCore/platform/graphics/win/QTMovieVisualContext.h:63 > > + QTVisualContextRef get(); > > Again, a better name for this would be visualContext(). > > Yep. > Or visualContextRef() :) > > WebCore/platform/graphics/win/QTPixelBuffer.cpp:68 > > + } > > Should be called "pixelBuffer" or something other than get. > > Similarly, perhaps "pixelBufferRef()"? > Sounds good. > > WebCore/platform/graphics/win/QTPixelBuffer.h:77 > > + void getExtendedPixels(size_t* left, size_t* right, size_t* top, > > size_t* bottom); > > Maybe references would be better than pointers here? > > The references will just be turned into pointers when we call > CVPixelBufferGetExtendedPixels. And if the caller only wanted to get the > "left" extended pixel, they would still have to define and pass in all the > other parameters anyway. OK! > > > WebCore/platform/graphics/win/QTPixelBuffer.h:51 > > + QTPixelBuffer& operator=(const QTPixelBuffer&); > > Come to think of it, I don't think ref counted objects should be copyable. Are > > the copy constructor and copy-assignment operators really needed? > > Nope. These could be moved into private: and unimplemented. The way we do this is by inheriting from the "Noncopyable" object. > > > WebCore/platform/graphics/win/QTPixelBuffer.h:82 > > + static void retainCallback(void* refcon); > > What are these used for? > > Funny story. You can't pass a CVPixelBuffer ref created in QTMovieWin.dll to a > CFRetain or CFRelease defined in CoreFoundation.dll. So whenever a retain or a > release needs to be called on a CVPixelBufferRef, it needs to occur within > QTMovieWin.dll. > > I defined three different types of retain/release callbacks in the > QTPixelBuffer class: a straight CF-style retain/release callback that takes a > single parameter, a CAImageQueue style release callback that takes a few other > extraneous parameters, and a CGDataProvider style release callback. > > The CF-style retain/release callbacks are so you can put a CVPixelBufferRef in > a CFArray (with the correct retain/release callbacks passed in during CFArray > creation). The CAImageQueue callbacks are for use with CAImageQueues (which > don't take CVPixelBuffers directly, but want a callback they can use to release > the underlying pixel data). And the CGDataProvider callbacks are so the > QTPixelBuffers can be used to create CGImages, backed by CGDataProviders, > referencing the original pixel data in the CVPixelBuffer. > Ah, I see! > > Patch looks good overall, but I'd like to see my comments addressed in a new > > patch. > > Sure thing. I'll post a replacement patch. Great! Created attachment 55053 [details]
Part 1.1: QTMovieWin: The breakup of QTMovieWin
Sam, I addressed your comments in this patch, except for the DEFINE_STATIC change. That I left out because I could not find a way to declare a static non-const pointer with that macro. Also, it appeared that the macro in question was designed to avoid file-level static initializers, and this static is function level. If you want, I can still use the DEFINE_STATIC macro, and const_cast away the constness of the static variable before returning it.
Created attachment 55054 [details]
Part 4.1: WebCore: New class MediaPlayerPrivateQuickTimeVisualContext, the copy
Created attachment 55056 [details]
Part 4.2: WebCore: New class MediaPlayerPrivateQuickTimeVisualContext, the modification
Created attachment 55057 [details]
Part 2: QTMovieWin: New Classes QTPixelBuffer, QTCFDictionary, and QTMovieVisualContext
Anders, I addressed all of your suggestions in this patch. However, instead of making QTPixelBuffer non-copyable, I made it non-refcounted. It was never used as a refcounted pointer, so there was no need for refcounting in the first place.
Created attachment 55058 [details]
Part 1.1: QTMovieWin: The breakup of QTMovieWin
Attached the wrong patch file previously.
Attachment 55054 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.h:26: #ifndef header guard has wrong style, please use: MediaPlayerPrivateQuickTimeVisualContext_h [build/header_guard] [5]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:29: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:31: Alphabetical sorting problem. [build/include_order] [4]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:57: "FrameView.h" already included at WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:34 [build/include] [4]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:58: "Frame.h" already included at WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:33 [build/include] [4]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:59: Alphabetical sorting problem. [build/include_order] [4]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:391: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:905: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
Total errors found: 8 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 55063 [details]
ChangeLog changes.
Maciej, I added the title of the bug above the bug URL in each of the ChangeLog entries. However, I did not break the ChangeLog diffs into each individual .patch, as there's a very good chance that committing the patches would cause collisions in the ChangeLog. I thought it would be safer to leave the ChangeLog in its own, separate patch.
For Parts 4.1 and 4.2, the style check is failing because the copy and modification portions are in separate patches. if this is going to block submission, let me know and I'll re-unify them. Looks like "Part 2: QTMovieWin: New Classes QTPixelBuffer, QTCFDictionary, and QTMovieVisualContext" and "Part 1.1: QTMovieWin: The breakup of QTMovieWin" are a copy of the same thing. Created attachment 55149 [details]
Part 1.1: QTMovieWin: The breakup of QTMovieWin
Chris, you're right. I must have uploaded the wrong patch as Part 1.1. Replacing that attachment with a new one now.
Comment on attachment 55057 [details]
Part 2: QTMovieWin: New Classes QTPixelBuffer, QTCFDictionary, and QTMovieVisualContext
WebCore/platform/graphics/win/QTMovieVisualContext.cpp:76
+ pPixelBufferContextCreate = reinterpret_cast<pfnQTPixelBufferContextCreate>(GetProcAddress(qtmlDLL, "QTPixelBufferContextCreate"));
Add :: to the GetProcAddress call like you did with LoadLibraryW.
WebCore/platform/graphics/win/QTPixelBuffer.cpp:68
+ m_pixelBuffer = ref;
If you want to, you could return early if ref == m_pixelBuffer.
(I don't feel strongly about it though).
Looks great otherwise, r=me!
Comment on attachment 55063 [details]
ChangeLog changes.
r=me
Comment on attachment 55056 [details] Part 4.2: WebCore: New class MediaPlayerPrivateQuickTimeVisualContext, the modification > +void MediaPlayerPrivateQuickTimeVisualContext::retrieveCurrentImage() > +{ > + if (!m_visualContext) > + return; > + > + if (!m_qtVideoLayer) > + return; > + > + QTPixelBuffer buffer = m_visualContext->imageForTime(0); > + if (!buffer.pixelBufferRef()) > + return; > + I assume imageForTime(0) gets the image for "now"? I realize that it just wraps QTVisualContextCopyImageForTime and that functions defines the magic value, but it would be nice to define a constant instead of passing "0" - "kCurrentMovieTime" maybe ? > + > + CFTimeInterval imageTime = QTMovieVisualContext::currentHostTime(); > + "imageTime" isn't used anywhere, left over from earlier revision? r=me (In reply to comment #68) > I assume imageForTime(0) gets the image for "now"? I realize that it just > wraps QTVisualContextCopyImageForTime and that functions defines the magic > value, but it would be nice > to define a constant instead of passing "0" - "kCurrentMovieTime" maybe ? I think I'd rather add a second function called QTMovieVisualContext::currentImage(), or ::imageForCurrentTime(). The constant is an implementation detail, and could change in the future outside of our own control. > "imageTime" isn't used anywhere, left over from earlier revision? Yup, deleted! Thanks! Comment on attachment 55149 [details] Part 1.1: QTMovieWin: The breakup of QTMovieWin > if (shouldRestorePlaybackState && m_timeToRestore != -1.0f) { > m_movieWin->setCurrentTime(m_timeToRestore); > @@ -281,7 +190,7 @@ void QTMovieWinPrivate::task() > m_rateToRestore = -1.0f; > } I know you didn't add this, but as long as you are cleaning things up you might as well change "-1.0f" to "-1" to match style guidelines. > + size_t indexOfClient = m_private->m_clients.find(client); > + if (indexOfClient >= 0) > + m_private->m_clients.remove(indexOfClient); Is ever this OK for this search to fail? If not, add an ASSERT() so we can track down the source of the problem. Nice refactoring! r=me Comment on attachment 55149 [details] Part 1.1: QTMovieWin: The breakup of QTMovieWin > + size_t indexOfClient = m_private->m_clients.find(client); > + if (indexOfClient >= 0) > + m_private->m_clients.remove(indexOfClient); This test will always be true. size_t is an unsigned value. The correct test is: if (indexOfClient != notFound) (In reply to comment #71) > (From update of attachment 55149 [details]) > > + size_t indexOfClient = m_private->m_clients.find(client); > > + if (indexOfClient >= 0) > > + m_private->m_clients.remove(indexOfClient); > > This test will always be true. size_t is an unsigned value. The correct test > is: > > if (indexOfClient != notFound) Good catch! Changed. Created attachment 55446 [details] Part 7: WebKit: FullscreenVideoController and QTMovieGWorld FullscreenVideoController dangerously casts a QTMovie* to a QTMovieWin*. This patch doesn't fix that, but it does fix the compile error now that QTMovieWin has been renamed QTMovieGWorld. The dangerous cast problem will be tracked in: https://bugs.webkit.org/show_bug.cgi?id=37832 Manually committed r59001: http://trac.webkit.org/changeset/59001 This broke the windows build: http://build.webkit.org/builders/Windows%20Debug%20(Build)/builds/15023/steps/compile-webkit/logs/stdio Manually committed r59003: http://trac.webkit.org/changeset/59003 |