Summary: | [Mac] 10X slower than Chrome when drawing a video into a canvas | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||
Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aloisio.almeida, commit-queue, dino, eric.carlson, esprehn+autocc, ggaren, glenn, gyuyoung.kim, philn, thiago.lacerda, xan.lopez | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jer Noble
2013-11-19 12:42:07 PST
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. |