Bug 101883 – [TexMap][Cairo] Accelerated compositing debug visuals

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Helder Correia]

Add a Cairo implementation to complement the patch from bug 90116.

[Helder Correia]

Created attachment 173539 [details]
Patch

[WebKit Review Bot]

Attachment 173539 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

Comment on attachment 173539 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:365
> +    cairo_set_source_rgb(cr, 1, 0, 0);

Qt has comment about blue color that it will turn red because R+B are not swapped. 
Will it result blue or red here? If it will result in different color from specified, IMHO, it requires comment here.

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:114
> +    return (String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1");

Should we use EFL prefix as Qt does?

[Noam Rosenthal]

> > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:114
> > +    return (String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1");
> 
> Should we use EFL prefix as Qt does?
I don't think that's needed, since Cairo is not EFL specific.

[Martin Robinson]

Comment on attachment 173539 [details]
Patch

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

Looks sane to me.

>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:365
>> +    cairo_set_source_rgb(cr, 1, 0, 0);
> 
> Qt has comment about blue color that it will turn red because R+B are not swapped. 
> Will it result blue or red here? If it will result in different color from specified, IMHO, it requires comment here.

You probably want to do the same thing as Qt above here.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:372
> +    cairo_move_to(cr, 2, pointSize);

Is this 2 pixels just padding?

[Helder Correia]

(In reply to comment #3)
> You probably want to do the same thing as Qt above here.

For Cairo, this patch will paint in red, just like the code above for Qt.

> Is this 2 pixels just padding?

Yes, could be anything.

[Viatcheslav Ostapenko]

(In reply to comment #6)
> (In reply to comment #3)
> > You probably want to do the same thing as Qt above here.
> 
> For Cairo, this patch will paint in red, just like the code above for Qt.

Yes, it seems it will be red, because updateContents always swizzles.

[Helder Correia]

Created attachment 174259 [details]
Fix style

[Kenneth Rohde Christiansen]

Comment on attachment 174259 [details]
Fix style

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:357
> +    // cairo_text_extents() requires a cairo_t, so dimensions need to be guesstimated.

no way of getting hold of that?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:361
> +    cairo_surface_t* surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, width, height);

why not use RefPtr they work for cairo_surface_t! etc

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:362
> +    cairo_t* cr = cairo_create(surface);

Here you have one, a bit too late :)

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:363
> +    cairo_surface_destroy(surface);

Is this safe here? cr stays valid?

[Helder Correia]

(In reply to comment #9)
> (From update of attachment 174259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174259&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:357
> > +    // cairo_text_extents() requires a cairo_t, so dimensions need to be guesstimated.
> 
> no way of getting hold of that?

AFAIK no. We need the width and height to create the surface, which we need to create the context, which we need to use the text extents, which we need to create the surface in the first place. In Qt, QFontMetrics don't need a QPainter. There might be a way in Cairo, but I'm not aware of it. Of course I could create a separate context just for the purpose, but I think there's no point.
 
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:361
> > +    cairo_surface_t* surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, width, height);
> 
> why not use RefPtr they work for cairo_surface_t! etc

I know, I chose to explicitly destroy it locally with no smart pointer allocation because its life cycle is pretty tiny and controlled.

> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:363
> > +    cairo_surface_destroy(surface);
> 
> Is this safe here? cr stays valid?

From http://www.cairographics.org/manual/cairo-cairo-t.html#cairo-create :

"This function references target, so you can immediately call cairo_surface_destroy() on it if you don't need to maintain a separate reference to it."

[WebKit Review Bot]

Comment on attachment 174259 [details]
Fix style

Clearing flags on attachment: 174259

Committed r134676: <http://trac.webkit.org/changeset/134676>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

Can this new code be combined with TextureMapperLayer::drawRepaintCounter somehow?