Bug 124599 – [Mac] 10X slower than Chrome when drawing a video into a canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[Mac] 10X slower than Chrome when drawing a video into a canvas

[Jer Noble]

Created attachment 217320 [details]
Patch

[Jer Noble]

Created attachment 217322 [details]
Patch

Fix the GTK bot.

[Dean Jackson]

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.

[Jer Noble]

Created attachment 217325 [details]
Patch

[Jer Noble]

Committed r159518: <http://trac.webkit.org/changeset/159518>

[Darin Adler]

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 ?

[Jer Noble]

(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.

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 217339 [details]
Follow up patch to address Darin's comments

[Mark Rowe (bdash)]

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.

[Darin Adler]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

<rdar://problem/15382272>