Bug 74993 – [EFL] Refactor the way using cairo in ewk_tiled_backing_store.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangHyuk]

For now there are cairo related code scattered around the ewk_tiled_backing_store.
From the point of code complexity view, it doesn't look good and I couldn't notice any benefit of it.
So, this patch will let cairo related code be used only for painting operation.

[KwangHyuk]

Created attachment 120570 [details]
Patch.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/ewk/ewk_view_tiled.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


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

[Ryuan Choi]

Comment on attachment 120570 [details]
Patch.

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

> Source/WebKit/efl/ewk/ewk_tiled_model.cpp:249
>      if (colorSpace == EVAS_COLORSPACE_ARGB8888) {
>          bytes = area * 4;
> -        stride = width * 4;
> -        format = CAIRO_FORMAT_ARGB32;
>      } else if (colorSpace == EVAS_COLORSPACE_RGB565_A5P) {
>          bytes = area * 2;
> -        stride = width * 2;
> -        format = CAIRO_FORMAT_RGB16_565;
>      } else {

remove brace for one statement.

> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:31
>  
> +#include <RefPtrCairo.h>

please fix order and remove empty line.

> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:45
> +    Eina_Bool ret = false;
> +
> +    RefPtr<cairo_t> cairo;
> +    RefPtr<cairo_surface_t> surface;
> +    int stride;
> +    cairo_format_t format;
> +    cairo_status_t status;

IMO, C++ can declare variable when it is required.

> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:76
> +    ret = ewk_view_paint_contents(priv, cairo.get(), &rect);
>  
> -    return ewk_view_paint_contents(priv, tile->cairo, &rect);
> +    return ret;

ret is not necessary.

[Tomasz Morawski]

(In reply to comment #1)
> Created an attachment (id=120570) [details]
> Patch.

It seems that this is not good idea to move code from ewk_tile_new function to _ewk_view_tiled_render_cb. Initialization of cairo surface should be done only once per tile. Initialization of new cairo surface on every _ewk_view_tiled_render_cb function call seems to be performance lost. Do you have any test result how performance will be changed?

[KwangHyuk]

Created attachment 120571 [details]
Patch fixed style error.

[KwangHyuk]

Created attachment 120572 [details]
Comment applied.

[KwangHyuk]

(In reply to comment #3)
> (From update of attachment 120570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120570&action=review
> 
> > Source/WebKit/efl/ewk/ewk_tiled_model.cpp:249
> >      if (colorSpace == EVAS_COLORSPACE_ARGB8888) {
> >          bytes = area * 4;
> > -        stride = width * 4;
> > -        format = CAIRO_FORMAT_ARGB32;
> >      } else if (colorSpace == EVAS_COLORSPACE_RGB565_A5P) {
> >          bytes = area * 2;
> > -        stride = width * 2;
> > -        format = CAIRO_FORMAT_RGB16_565;
> >      } else {
> 
> remove brace for one statement.
> 
Ok, thank you. :)
I have just used style check script for the patch file.
I don't know why it couldn't detect this error.

> > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:31
> >  
> > +#include <RefPtrCairo.h>
> 
> please fix order and remove empty line.
> 
> > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:45
> > +    Eina_Bool ret = false;
> > +
> > +    RefPtr<cairo_t> cairo;
> > +    RefPtr<cairo_surface_t> surface;
> > +    int stride;
> > +    cairo_format_t format;
> > +    cairo_status_t status;
> 
> IMO, C++ can declare variable when it is required.
> 
Done.

> > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:76
> > +    ret = ewk_view_paint_contents(priv, cairo.get(), &rect);
> >  
> > -    return ewk_view_paint_contents(priv, tile->cairo, &rect);
> > +    return ret;
> 
> ret is not necessary.
Finally done.

[Ryuan Choi]

(In reply to comment #4)
> (In reply to comment #1)
> > Created an attachment (id=120570) [details] [details]
> > Patch.
> 
> It seems that this is not good idea to move code from ewk_tile_new function to _ewk_view_tiled_render_cb. Initialization of cairo surface should be done only once per tile. Initialization of new cairo surface on every _ewk_view_tiled_render_cb function call seems to be performance lost. Do you have any test result how performance will be changed?

I'm not sure. Does it really have performance issue?

IIRC, It is not expensive and WebKit/Gtk+ and firefox doesn't keep cairo surface.

[KwangHyuk]

(In reply to comment #8)
> (In reply to comment #4)
> > (In reply to comment #1)
> > > Created an attachment (id=120570) [details] [details] [details]
> > > Patch.
> > 
> > It seems that this is not good idea to move code from ewk_tile_new function to _ewk_view_tiled_render_cb. Initialization of cairo surface should be done only once per tile. Initialization of new cairo surface on every _ewk_view_tiled_render_cb function call seems to be performance lost. Do you have any test result how performance will be changed?
> 
> I'm not sure. Does it really have performance issue?
> 
> IIRC, It is not expensive and WebKit/Gtk+ and firefox doesn't keep cairo surface.

I am now agreeing with Ryuan.

According to my measurement, only one big burden occurs when it brings pixel data from image object for the first time when image object is created. :)

I couldn't notice the performance loss when it creates cairo surface and context yet.

[Tomasz Morawski]

> > I'm not sure. Does it really have performance issue?
> > 
> > IIRC, It is not expensive and WebKit/Gtk+ and firefox doesn't keep cairo surface.
> 
> I am now agreeing with Ryuan.
> 
> According to my measurement, only one big burden occurs when it brings pixel data from image object for the first time when image object is created. :)
> 
> I couldn't notice the performance loss when it creates cairo surface and context yet.

Thank you, but for curiosity. According to your measurement, could you tell how much time take cairo_image_surface_create_for_data and cairo_create call in each _ewk_view_tiled_render_cb function call?

[KwangHyuk]

(In reply to comment #10)
> > > I'm not sure. Does it really have performance issue?
> > > 
> > > IIRC, It is not expensive and WebKit/Gtk+ and firefox doesn't keep cairo surface.
> > 
> > I am now agreeing with Ryuan.
> > 
> > According to my measurement, only one big burden occurs when it brings pixel data from image object for the first time when image object is created. :)
> > 
> > I couldn't notice the performance loss when it creates cairo surface and context yet.
> 
> Thank you, but for curiosity. According to your measurement, could you tell how much time take cairo_image_surface_create_for_data 

According to my measurement, it was 0.013 msec.


> and cairo_create call in each _ewk_view_tiled_render_cb function call?
It was 0.0036 msec.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 120572 [details]
Comment applied.

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

> Source/WebKit/efl/ewk/ewk_view_tiled.cpp:28
> +#include <RefPtrCairo.h>

Awesome, I love seeing smart pointers being used :) I don't think you need reference counting in this case, though. Isn't an OwnPtr enough?

[KwangHyuk]

(In reply to comment #12)
> (From update of attachment 120572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120572&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:28
> > +#include <RefPtrCairo.h>
> 
> Awesome, I love seeing smart pointers being used :) I don't think you need reference counting in this case, though. Isn't an OwnPtr enough?

Hi, Kubo.

I am glad that you review my patch. :)
Let me check your idea.

[KwangHyuk]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 120572 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=120572&action=review
> > 
> > > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:28
> > > +#include <RefPtrCairo.h>
> > 
> > Awesome, I love seeing smart pointers being used :) I don't think you need reference counting in this case, though. Isn't an OwnPtr enough?
> 
> Hi, Kubo.
> 
> I am glad that you review my patch. :)
> Let me check your idea.

Kubo,

Unfortunately, OwnPtrCairo doesn't seem to support cairo_t and cairo_surface_t.
Fortunately, One of my colleague (Tomasz) is wanting to add new code into OwnPtrCairo.
After Tomasz's new patch will be applied, I will replace RefPtr with OwnPtr.

[KwangHyuk]

Created attachment 120855 [details]
Patch rebased.

[Tomasz Morawski]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 120572 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=120572&action=review
> > > 
> > > > Source/WebKit/efl/ewk/ewk_view_tiled.cpp:28
> > > > +#include <RefPtrCairo.h>
> > > 
> > > Awesome, I love seeing smart pointers being used :) I don't think you need reference counting in this case, though. Isn't an OwnPtr enough?
> > 
> > Hi, Kubo.
> > 
> > I am glad that you review my patch. :)
> > Let me check your idea.
> 
> Kubo,
> 
> Unfortunately, OwnPtrCairo doesn't seem to support cairo_t and cairo_surface_t.
> Fortunately, One of my colleague (Tomasz) is wanting to add new code into OwnPtrCairo.
> After Tomasz's new patch will be applied, I will replace RefPtr with OwnPtr.
Hi,
Finally. All cairo object should not be wrapped in an OwnPtr because it's reference counted internally. This is way there is no implementation for basic cairo_t object in OwnPtr. We should stay with RefPtr version of Kwang's patch.

[Raphael Kubo da Costa (:rakuco)]

OK then.

[Nikolas Zimmermann]

Comment on attachment 120855 [details]
Patch rebased.

Rubberstamping this. I had worries about the RefPtr<> now being obsolete (that was already commented here, why its ok) as well as the needless recreation of the surfaces, but I was told its no measurable difference, and eg. FF wouldn't cache cairo_t/cairo_surface_t objects as well, so r=me.

[WebKit Review Bot]

Comment on attachment 120855 [details]
Patch rebased.

Clearing flags on attachment: 120855

Committed r104021: <http://trac.webkit.org/changeset/104021>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.