[Mac] 10X slower than Chrome when drawing a video into a canvas
Created attachment 217320 [details] Patch
Created attachment 217322 [details] Patch Fix the GTK bot.
Comment on attachment 217322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217322&action=review > Source/WebCore/html/HTMLVideoElement.h:74 > > + > + > bool shouldDisplayPosterImage() const { return displayMode() == Poster || displayMode() == PosterWaitingForVideo; } Why so spacey? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1351 > + // pixelBuffer will be of type kCVPixelFormatType_32BGRA Nit: Ending . > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1360 > + CFRetain(pixelBuffer); // Balanced by CVPixelBufferReleaseInfoCallback in providerCallbacks Ditto. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1412 > + // Wait for 1 second Ditto.
Created attachment 217325 [details] Patch
Committed r159518: <http://trac.webkit.org/changeset/159518>
Comment on attachment 217325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217325&action=review > Source/WebCore/html/HTMLVideoElement.cpp:274 > + return 0; nullptr > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1454 > + if (PassNativeImagePtr image = video->nativeImageForCurrentTime()) { Shouldn’t this be NativeImagePtr, not PassnNativeImagePtr? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1457 > + if (rectContainsCanvas(dstRect)) > + didDrawEntireCanvas(); Why is this needed? Shouldn’t didDraw do this check? > Source/WebCore/platform/graphics/MediaPlayerPrivate.h:124 > + virtual PassNativeImagePtr nativeImageForCurrentTime() { return 0; } nullptr > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:200 > + MediaPlayerPrivateAVFoundationObjC* m_callback; > + dispatch_semaphore_t m_semaphore; Missing space before “*” here. Strange to use the m_member syntax in an Objective-C class. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:202 > +- (id)initWithCallback:(MediaPlayerPrivateAVFoundationObjC*)callback; Missing space before “*” here. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:270 > + , m_videoOutputDelegate(adoptNS([[WebCoreAVFPullDelegate alloc] initWithCallback:this])) I believe we need to clear out the callback pointer when this is deleted, in case someone has retained the delegate. Maybe in dealloc. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:271 > + , m_videoOutputSemaphore(0) nullptr > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1332 > + CVPixelBufferRef pixelBuffer = (CVPixelBufferRef)info; static_cast > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1337 > +static void CVPixelBufferReleaseBytePointerCallback(void *info, const void *) Misplaced “*” here. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1339 > + CVPixelBufferRef pixelBuffer = (CVPixelBufferRef)info; static_cast > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1345 > + CVPixelBufferRef pixelBuffer = (CVPixelBufferRef)info; static_cast > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1378 > +void MediaPlayerPrivateAVFoundationObjC::paintWithVideoOutput(GraphicsContext* context, const IntRect& rect) Should be GraphicsContext& on any new function. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1408 > + m_videoOutputSemaphore = dispatch_semaphore_create(0); nullptr ?
(In reply to comment #6) > (From update of attachment 217325 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217325&action=review Darin, I didn't see your review before landing this patch. I'll address these in a follow up. > > Source/WebCore/html/HTMLVideoElement.cpp:274 > > + return 0; > > nullptr Changed. > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1454 > > + if (PassNativeImagePtr image = video->nativeImageForCurrentTime()) { > > Shouldn’t this be NativeImagePtr, not PassnNativeImagePtr? For CG, these are identical, but I'll change the local variable to a NativeImagePtr. > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1457 > > + if (rectContainsCanvas(dstRect)) > > + didDrawEntireCanvas(); > > Why is this needed? didDrawEntireCanvas() clips the sourceRect to the canvas's, then calls didDraw(). > Shouldn’t didDraw do this check? Perhaps, but from the other places where didDrawEntireCanvas() is called, there are some optimizations which would be lost. > > Source/WebCore/platform/graphics/MediaPlayerPrivate.h:124 > > + virtual PassNativeImagePtr nativeImageForCurrentTime() { return 0; } > > nullptr Changed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:200 > > + MediaPlayerPrivateAVFoundationObjC* m_callback; > > + dispatch_semaphore_t m_semaphore; > > Missing space before “*” here. Added. > Strange to use the m_member syntax in an Objective-C class. Agreed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:202 > > +- (id)initWithCallback:(MediaPlayerPrivateAVFoundationObjC*)callback; > > Missing space before “*” here. Added. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:270 > > + , m_videoOutputDelegate(adoptNS([[WebCoreAVFPullDelegate alloc] initWithCallback:this])) > > I believe we need to clear out the callback pointer when this is deleted, in case someone has retained the delegate. Maybe in dealloc. Added. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:271 > > + , m_videoOutputSemaphore(0) > > nullptr Changed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1332 > > + CVPixelBufferRef pixelBuffer = (CVPixelBufferRef)info; > > static_cast Changed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1337 > > +static void CVPixelBufferReleaseBytePointerCallback(void *info, const void *) > > Misplaced “*” here. Moved. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1339 > > + CVPixelBufferRef pixelBuffer = (CVPixelBufferRef)info; > > static_cast Changed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1345 > > + CVPixelBufferRef pixelBuffer = (CVPixelBufferRef)info; > > static_cast Changed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1378 > > +void MediaPlayerPrivateAVFoundationObjC::paintWithVideoOutput(GraphicsContext* context, const IntRect& rect) > > Should be GraphicsContext& on any new function. This is an old function; the diff just got confused. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1408 > > + m_videoOutputSemaphore = dispatch_semaphore_create(0); > > nullptr ? No, this is a count.
Reopening to attach new patch.
Created attachment 217339 [details] Follow up patch to address Darin's comments
Comment on attachment 217325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217325&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:247 > + globalQueue = dispatch_queue_create("WebCoreAVFPullDelegate queue", DISPATCH_QUEUE_SERIAL); Dispatch queue names are conventionally reverse-DNS. This should be something like com.apple.WebCore.AVFPullDelegate.
Comment on attachment 217339 [details] Follow up patch to address Darin's comments View in context: https://bugs.webkit.org/attachment.cgi?id=217339&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:203 > +- (void)setCallback:(MediaPlayerPrivateAVFoundationObjC*)callback; Funny that you added this without a "*". I also think we probably could just have a clearCallback method instead of one that takes a pointer. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:302 > + [m_videoOutputDelegate setCallback:0]; nullptr > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:303 > + [m_videoOutput setDelegate:nil queue:0]; nullptr
Comment on attachment 217339 [details] Follow up patch to address Darin's comments Clearing flags on attachment: 217339 Committed r164160: <http://trac.webkit.org/changeset/164160>
All reviewed patches have been landed. Closing bug.
<rdar://problem/15382272>