Bug 68501 - [skia] Optimize ImageBuffer constructor when accelerated
Summary: [skia] Optimize ImageBuffer constructor when accelerated
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-20 21:13 PDT by Ben Wells
Modified: 2011-11-02 02:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.47 KB, patch)
2011-09-21 07:20 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Updated per comments (6.36 KB, patch)
2011-09-21 11:53 PDT, Stephen White
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Wells 2011-09-20 21:13:46 PDT
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.
Comment 1 Stephen White 2011-09-21 07:20:04 PDT
Created attachment 108153 [details]
Patch
Comment 2 James Robinson 2011-09-21 11:10:47 PDT
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
Comment 3 Alok Priyadarshi 2011-09-21 11:23:27 PDT
Comment on attachment 108153 [details]
Patch

lgtm. I like James' suggestion about moving the creation of canvas after creating the texture.
Comment 4 Stephen White 2011-09-21 11:53:26 PDT
Created attachment 108198 [details]
Updated per comments
Comment 5 Stephen White 2011-09-21 11:56:59 PDT
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.
Comment 6 James Robinson 2011-09-21 11:57:56 PDT
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
Comment 7 James Robinson 2011-09-21 11:58:15 PDT
(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
Comment 8 Stephen White 2011-09-21 12:10:44 PDT
Committed r95658: <http://trac.webkit.org/changeset/95658>