Bug 148307 - Clear cairo-gl surface for initialization
Summary: Clear cairo-gl surface for initialization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jinyoung Hur
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-21 08:10 PDT by Jinyoung Hur
Modified: 2015-08-24 18:57 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.89 KB, patch)
2015-08-21 08:34 PDT, Jinyoung Hur
no flags Details | Formatted Diff | Diff
Patch (2.89 KB, patch)
2015-08-21 08:53 PDT, Jinyoung Hur
no flags Details | Formatted Diff | Diff
Patch (2.70 KB, patch)
2015-08-24 17:41 PDT, Jinyoung Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jinyoung Hur 2015-08-21 08:10:48 PDT
Clear cairo-gl surface for initialization
Comment 1 Jinyoung Hur 2015-08-21 08:34:00 PDT
Created attachment 259615 [details]
Patch
Comment 2 Jinyoung Hur 2015-08-21 08:50:04 PDT
The accelerated canvas backed by cairo-gl has a possible memory initialize problem.

A texture memory that is created by calling glTexImage2D with no data, is in undefined status[1] so it can have a garbage data.

And also, according to the comments[2] in cairo.h, cairo seems not cleaning the passed texture when it create a surface.

So it would be good to clean the just created cairo-gl surface by hand.


[1] https://www.opengl.org/sdk/docs/man/html/glTexImage2D.xhtml  
    "The image is undefined if the user tries..."
[2] "if the user passes in a reference to some backing storage and asks cairo to wrap that in a #cairo_surface_t, then the contents are not modified.
Comment 3 Jinyoung Hur 2015-08-21 08:53:45 PDT
Created attachment 259616 [details]
Patch
Comment 4 Gyuyoung Kim 2015-08-24 00:12:32 PDT
CC'ing Martin.
Comment 5 Martin Robinson 2015-08-24 09:43:19 PDT
Comment on attachment 259616 [details]
Patch

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

Looks okay to me, but the code has a few extra steps which I think are unnecessary.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:80
> +    cairo_rectangle(cr.get(), 0, 0, size.width(), size.height());
> +    cairo_set_operator(cr.get(), CAIRO_OPERATOR_CLEAR);
> +    cairo_fill(cr.get());

You don't need to use cairo_rectangle + cairo_fill, you can simply do cairo_paint.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:81
> +    cairo_restore(cr.get());

No need to save and restore the context state, you are just throwing it away afterward.
Comment 6 Jinyoung Hur 2015-08-24 17:41:43 PDT
Created attachment 259798 [details]
Patch
Comment 7 Jinyoung Hur 2015-08-24 17:43:59 PDT
(In reply to comment #5)
> Comment on attachment 259616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259616&action=review
> 
> Looks okay to me, but the code has a few extra steps which I think are
> unnecessary.
> 
> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:80
> > +    cairo_rectangle(cr.get(), 0, 0, size.width(), size.height());
> > +    cairo_set_operator(cr.get(), CAIRO_OPERATOR_CLEAR);
> > +    cairo_fill(cr.get());
> 
> You don't need to use cairo_rectangle + cairo_fill, you can simply do
> cairo_paint.
> 
> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:81
> > +    cairo_restore(cr.get());
> 
> No need to save and restore the context state, you are just throwing it away
> afterward.

Thank you for a helpful review!
I've modified the patch as you said.
Comment 8 WebKit Commit Bot 2015-08-24 18:57:01 PDT
Comment on attachment 259798 [details]
Patch

Clearing flags on attachment: 259798

Committed r188902: <http://trac.webkit.org/changeset/188902>
Comment 9 WebKit Commit Bot 2015-08-24 18:57:06 PDT
All reviewed patches have been landed.  Closing bug.