Bug 68501 – [skia] Optimize ImageBuffer constructor when accelerated

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Ben Wells]

The ImageBuffer constructor creates a SkDevice for a non-accelerated canvas first. Then, if an accelerated buffer is requested it will try to create an accelerated SkDevice, if successful overwriting the non-accelerated device.

This can be optimized to first try, if requested, to create an accelerated SkDevice and only create the non-accelerated device if that fails.

[Stephen White]

Created attachment 108153 [details]
Patch

[James Robinson]

Comment on attachment 108153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108153&action=review

R=me, left some comments

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:69
> +    GraphicsContext3D* context3D = SharedGraphicsContext3D::create(0);

we really should remove the parameter from SharedGraphicsContext3D::create(), it's not useful

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:76
> +    SkCanvas* canvas = new SkCanvas();

nit: if you move this down below line 83 I think you can avoid having to manually delete this if the texture thingy fails

also, if you make this function return a PassOwnPtr<> then you can make this an OwnPtr<SkCanvas>, have the return value be canvas.release() and not have to worry about saying 'delete' at all

[Alok Priyadarshi]

Comment on attachment 108153 [details]
Patch

lgtm. I like James' suggestion about moving the creation of canvas after creating the texture.

[Stephen White]

Created attachment 108198 [details]
Updated per comments

[Stephen White]

Thanks for your review.

(In reply to comment #2)
> (From update of attachment 108153 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108153&action=review
> 
> R=me, left some comments
> 
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:69
> > +    GraphicsContext3D* context3D = SharedGraphicsContext3D::create(0);
> 
> we really should remove the parameter from SharedGraphicsContext3D::create(), it's not useful

Done.  Also renamed SharedGraphicsContext3D::create() to get(), since it's only a create the first time.

> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:76
> > +    SkCanvas* canvas = new SkCanvas();
> 
> nit: if you move this down below line 83 I think you can avoid having to manually delete this if the texture thingy fails

Done.

> also, if you make this function return a PassOwnPtr<> then you can make this an OwnPtr<SkCanvas>, have the return value be canvas.release() and not have to worry about saying 'delete' at all

I tried that, but it meant all the early-returns needed to return a PassRefPtr<SkCanvas*>(0), which looked ugly.

[James Robinson]

Comment on attachment 108198 [details]
Updated per comments

View in context: https://bugs.webkit.org/attachment.cgi?id=108198&action=review

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:37
> +    static GraphicsContext3D* get();

this header file is kind of hilariously tiny now.  I actually think you can remove the three #include lines and just leave a forward declaration of GraphicsContext3D.

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:85
> +    SkCanvas* canvas = new SkCanvas();

I think I'd still mildly prefer having this function be:
PassOwnPtr<SkCanvas> createAcceleratedCanvas(...)

and this line being OwnPtr<SkCanvas> canvas = adoptPtr(new SkCanvas())

just to try to keep the raw pointers down.  it's not a very practical concern

[James Robinson]

(In reply to comment #5)
> > also, if you make this function return a PassOwnPtr<> then you can make this an OwnPtr<SkCanvas>, have the return value be canvas.release() and not have to worry about saying 'delete' at all
> 
> I tried that, but it meant all the early-returns needed to return a PassRefPtr<SkCanvas*>(0), which looked ugly.

Aha!  You want nullptr

[Stephen White]

Committed r95658: <http://trac.webkit.org/changeset/95658>