Bug 104672 - [GTK] Add accelerated 2D canvas support using cairo-gl
Summary: [GTK] Add accelerated 2D canvas support using cairo-gl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk
: 108743 (view as bug list)
Depends on: 104640 114490
Blocks: 112150
  Show dependency treegraph
 
Reported: 2012-12-11 06:55 PST by Alejandro G. Castro
Modified: 2013-04-12 08:20 PDT (History)
24 users (show)

See Also:


Attachments
WIP patch (19.43 KB, patch)
2012-12-11 07:51 PST, Alejandro G. Castro
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
WIP patch (19.04 KB, patch)
2012-12-24 02:30 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (26.92 KB, patch)
2013-04-10 13:00 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch rebased on master (26.90 KB, patch)
2013-04-10 13:26 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2012-12-11 06:55:10 PST
With this bug we will track patches to implement accelerated 2D canvas support using the cairo-gl backend.
Comment 1 Alejandro G. Castro 2012-12-11 07:51:20 PST
Created attachment 178805 [details]
WIP patch

This patch is a work in progress, I've updated and fixed some problems with the patch mrobinson implemented some months ago. Tests and comments are welcome, we will be working on it. If you want to test it you need to apply the patch in the bug blocking this one, compile cairo with gl support, enable accelerated2dCanvasEnabled in Settings.in and build with option --enable-accelerated-2d-canvas.
Comment 2 WebKit Review Bot 2012-12-11 10:53:35 PST
Attachment 178805 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.am', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:107:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 EFL EWS Bot 2012-12-11 11:56:44 PST
Comment on attachment 178805 [details]
WIP patch

Attachment 178805 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15280182
Comment 4 Dominik Röttsches (drott) 2012-12-13 01:27:44 PST
(In reply to comment #0)
> With this bug we will track patches to implement accelerated 2D canvas support using the cairo-gl backend.

Do you have any comparisons before and after? That'd be interesting to hear.
Comment 5 Martin Robinson 2012-12-13 01:32:48 PST
(In reply to comment #4)
> (In reply to comment #0)
> > With this bug we will track patches to implement accelerated 2D canvas support using the cairo-gl backend.
> 
> Do you have any comparisons before and after? That'd be interesting to hear.

It's very much dependent on your hardware. For instance, current mesa-based drivers do not support multisampling, so they will go down the slow cairo path when anti-aliasing is turned on.
Comment 6 arno. 2012-12-21 18:07:38 PST
Hi,
I tried this patch which looks really interesting.
But there seems to be a problem with putByteArray: the first row is correct. Then all other rows are the same as first row

see this example:
http://renevier.net/misc/webkit_104672.html
and the result:
http://renevier.net/misc/webkit_104672_bug.jpg

Anyway, why not call glReadPixels and glTexSubImage2D directly? That would save many data copies between the gl-surface and the mapped image. And even a glReadPixels which is performed at the start of putByteArray while in theory we don't need to read previous pixel data when we just upload new data).
Comment 7 Martin Robinson 2012-12-23 09:32:45 PST
(In reply to comment #6)

> Anyway, why not call glReadPixels and glTexSubImage2D directly? That would save many data copies between the gl-surface and the mapped image. And even a glReadPixels which is performed at the start of putByteArray while in theory we don't need to read previous pixel data when we just upload new data).

Cairo's map-to-image and unmap-image effectively just do a glReadPixels and glTexSubImage2D. In the future the may be optimized further with PBOs as well. There shouldn't be any extra data copies, as far as I know.
Comment 8 arno. 2012-12-23 11:10:38 PST
(In reply to comment #7)


> Cairo's map-to-image and unmap-image effectively just do a glReadPixels and glTexSubImage2D. In the future the may be optimized further with PBOs as well.

In the patch, there is a map at the start of getImageData and an unmap at its end. Same thing for putByteArray.

But getImageData only needs to read pixels from graphic card, and putByteArray only needs to puts pixels to graphic card. So, there may be an extra glTexSubImage2D in getImageData and an extra glReadPixels in putByteArray. 


> There shouldn't be any extra data copies, as far as I know.

ok, sorry about that.
Comment 9 Martin Robinson 2012-12-23 12:08:12 PST
(In reply to comment #8)

> In the patch, there is a map at the start of getImageData and an unmap at its end. Same thing for putByteArray.
> 
> But getImageData only needs to read pixels from graphic card, and putByteArray only needs to puts pixels to graphic card. So, there may be an extra glTexSubImage2D in getImageData and an extra glReadPixels in putByteArray. 

Ah, I see what you mean now. Yeah, there's definitely a chance that we can avoid an extra read here. Doing pixel upload is already pretty slow, so we likely won't be getting great performance here anyway, but your suggestion makes a lot of sense.
Comment 10 Alejandro G. Castro 2012-12-24 02:30:42 PST
Created attachment 180659 [details]
WIP patch

Some minor fix in the sizes and style cleaning.
Comment 11 Alejandro G. Castro 2012-12-24 02:32:36 PST
(In reply to comment #4)
> (In reply to comment #0)
> > With this bug we will track patches to implement accelerated 2D canvas support using the cairo-gl backend.
> 
> Do you have any comparisons before and after? That'd be interesting to hear.

The idea about uploading the work in progress patch is trying to get those figures and proposals from everyone interested in these solution. We still need to make sure in which situations and how to integrate it the best way.
Comment 12 EFL EWS Bot 2012-12-24 02:45:04 PST
Comment on attachment 180659 [details]
WIP patch

Attachment 180659 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15491480
Comment 13 kov's GTK+ EWS bot 2012-12-24 02:45:34 PST
Comment on attachment 180659 [details]
WIP patch

Attachment 180659 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15491481
Comment 14 Kyungjin Kim 2013-02-07 17:09:03 PST
*** Bug 108743 has been marked as a duplicate of this bug. ***
Comment 15 Martin Robinson 2013-04-10 13:00:00 PDT
Created attachment 197390 [details]
Patch
Comment 16 Martin Robinson 2013-04-10 13:26:02 PDT
Created attachment 197396 [details]
Patch rebased on master
Comment 17 Alejandro G. Castro 2013-04-11 16:22:32 PDT
Comment on attachment 197396 [details]
Patch rebased on master

LGTM. Great, after this time testing let's add it to the repository and work from that.
Comment 18 Martin Robinson 2013-04-11 16:38:17 PDT
Comment on attachment 197396 [details]
Patch rebased on master

Clearing flags on attachment: 197396

Committed r148247: <http://trac.webkit.org/changeset/148247>
Comment 19 Martin Robinson 2013-04-11 16:38:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Commit Bot 2013-04-11 22:59:44 PDT
Re-opened since this is blocked by bug 114490
Comment 21 Zan Dobersek 2013-04-11 23:06:29 PDT
(In reply to comment #20)
> Re-opened since this is blocked by bug 114490

Rolled it out as Cairo does not build on the builders due to missing EGL headers. I don't have the permissions to install those dependencies myself so I'll get someone to do that for me. After that I'll get the patch landed again.
Comment 22 Zan Dobersek 2013-04-12 00:38:11 PDT
... and rolled back in in r148264. Cairo now compiles. Closing as fixed.
http://trac.webkit.org/changeset/148264
Comment 23 Zan Dobersek 2013-04-12 00:39:46 PDT
Oh, and sorry for the noise, the rollout was done with too much haste as it didn't take long to get someone to install those headers.
Comment 24 Sergio Correia (qrwteyrutiyoup) 2013-04-12 07:51:31 PDT
Out of curiosity, is there any improvements performance-wise, when using cairo-gl? Maybe some traces to veify with cairo-perf-trace? :)
Comment 25 Martin Robinson 2013-04-12 08:20:22 PDT
(In reply to comment #24)
> Out of curiosity, is there any improvements performance-wise, when using cairo-gl? Maybe some traces to veify with cairo-perf-trace? :)

It depends quite a bit on the system, but sometimes this does yield a performance boost. Part of making sure that it's consistently faster though, is being able to easily test CairoGL. :)