GNOME Bugzilla – Bug 779234
Avoid pixel conversions when storing textures from cairo image format
Last modified: 2018-03-24 00:04:12 UTC
On intel/mesa/wayland, running gtk+/tests/scrolling-performance testcase (either gtk3 or master with GSK_RENDERER=cairo) can easily get gnome-shell at 45-50% CPU here. This testcase tries to scroll a big scrolledwindow at 60fps. Some perf runs later (and some brainstorming with Christian Kellner), it seems pretty clear the hottest path is in mesa, in the convert_ubyte() function, this is the deepest function in the code involved in transforming pixel buffers between formats. Some more investigation later, it seems most notably due to storing BGRA data into textures with internal RGBA format. And it turns out, we do it a lot. As BGRA is the practical storage format of cairo in little endian platforms, anything that's ultimately backed by cairo requires format conversions for storing those in textures: this means glyphs, assets, shm-buffer wayland clients, ... In weston they seem to avoid this by using GLES and GL_EXT_texture_format_BGRA8888, so they can just use BGRA internal format in textures, and no conversions are required. But AFAICS that extension is not exposed on GL, setting a BGRA internal format just fails there even though it's announced. There's however the GL_*_texture_swizzle extension, which allows shuffling the pixel channels at later point, when the texture is needed in the pipeline. This let us pretend the texture is stored as RGBA so no conversion is performed by mesa, and let the pipeline access it correctly as BGRA when rendering, so it actually displays the right colors. I'm attaching a patch to use it this way. This notably reduces CPU usage with that gtk+ testcase to 25-30% CPU, which is still higher than I'd wish, but much lower.
Created attachment 346725 [details] [review] cogl: Prefer swizzling to convert BGRA buffers If the GL implementation/hw supports the GL_*_texture_swizzle extension, pretend that BGRA textures shall contain ARGB data, and let the flipping happen when the texture will be used in the rendering pipeline. This avoids rather expensive format conversions when forcing BGRA buffers into ARGB textures, which happens rather often with WL_SHM_FORMAT_ARGB8888 buffers (like gtk+ uses) in little-endian machines. In intel/mesa/wayland, the performance improvement is rather noticeable, CPU% as seen by top decreases from 45-50% to 25-30% when running gtk+/tests/scrolling-performance with a cairo renderer.
After positive review yesterday on IRC from ebassi and today from rtcm, I pushed this patch to master. Attachment 346725 [details] pushed as 1705a26 - cogl: Prefer swizzling to convert BGRA buffers
Created attachment 346986 [details] [review] cogl: Read pixels in the correct 32bit format as per the given bitmap Fixes the gnome-shell screenshot tool from getting colors with the wrong byte order. --- I apparently broke screenshots with this change, this new patch forces the right pixel format when reading pixels.
Review of attachment 346986 [details] [review]: oops, makes sense
Thanks :). Hopefully this is all the fallout. Attachment 346986 [details] pushed as 95e9fa1 - cogl: Read pixels in the correct 32bit format as per the given bitmap
Something else is wrong. After upgrading mutter from 3.23.90 to 3.23.91, I noticed that gnome-shell appeared to be displaying all its fonts (e.g. in the top panel) with BGR subpixel rendering instead of RGB subpixel rendering. I bisected the problem to 1705a26. I tried 95e9fa1 but the problem remained. My laptop is a Thinkpad Yoga 14 20FY with Intel HD 520 graphics running Ubuntu 17.04. Should I file a new bug report?
Great that you noticed, and thanks for the heads up! I don't think there's need to file a new bug, as this is a regression in these commits. There's indeed something amiss with font rendering, actually it's not just AA what is wrong but all font color, I think just hidden because gnome-shell fonts tend to be very close to the gray scale. I traced this down to CoglAtlasTexture, used beneath font rendering, which uses a RGBA texture for storage, the bitmap makes now cogl think that swizzling may apply, but the texture has the unexpected format and sets no GL_TEXTURE_SWIZZLE_RGBA parameter. This results in BGRA buffers being copied as-is into an RGBA texture for fonts. The following change makes the problem entirely evident: --- a/cogl/cogl-pango/cogl-pango-render.c +++ b/cogl/cogl-pango/cogl-pango-render.c @@ -609,7 +609,7 @@ cogl_pango_renderer_set_dirty_glyph (PangoFont *font, scaled_font = pango_cairo_font_get_scaled_font (PANGO_CAIRO_FONT (font)); cairo_set_scaled_font (cr, scaled_font); - cairo_set_source_rgba (cr, 1.0, 1.0, 1.0, 1.0); + cairo_set_source_rgba (cr, 1.0, 0.0, 0.0, 1.0); With this gnome-shell will render fonts in full red, however they're shown blue with master. I'm attaching a couple of patches to cater for this situation of uploading bitmaps to textures with a predefined/non-matching internal format.
Created attachment 347156 [details] [review] cogl: Add pixel_format_to_gl_with_target driver vfunc This is used by the GL driver in order to determine whether swizzling actually applies given the bitmap and target texture internal format. If both agree that they store BGRA, then swizzling may apply.
Created attachment 347157 [details] [review] cogl: Use pixel_format_to_gl_with_target on bitmap uploading paths We already do have a texture with an internal format in these paths, so we should check the required format according to it. This fixes CoglAtlasTexture (and CoglPangoRenderer indirectly), as it forces a RGBA format on its texture, but pixel_format_to_gl() anyway assumed swizzling is performed on the texture, while it is not the case.
Thanks; with all these patches applied, the rendering seems fixed.
*** Bug 779661 has been marked as a duplicate of this bug. ***
(In reply to Anders Kaseorg from comment #10) > Thanks; with all these patches applied, the rendering seems fixed. Thanks for confirming. I also see the patches fix https://bugzilla.redhat.com/show_bug.cgi?id=1428559, where Xwayland SHM buffers with RGB components also get the wrong swizzling treatment.
Review of attachment 347156 [details] [review]: Looks good except for what seems to be a missing argument ::: cogl/cogl/driver/gl/gl/cogl-driver-gl.c @@ +302,3 @@ +static CoglPixelFormat +_cogl_driver_pixel_format_to_gl (CoglContext *context, + CoglPixelFormat format, nit: an opportunity to clean up a stray whitespace ::: cogl/cogl/driver/gl/gles/cogl-driver-gles.c @@ +229,3 @@ +{ + return _cogl_driver_pixel_format_to_gl_with_target (context, + format, this is missing a "format"
Review of attachment 347157 [details] [review]: Looks good.
(In reply to Jonas Ådahl from comment #13) > Review of attachment 347156 [details] [review] [review]: > +{ > + return _cogl_driver_pixel_format_to_gl_with_target (context, > + format, > > this is missing a "format" Oops, I guess I tried without --enable-gles2. I fixed that and am pushing now, thanks!
Attachment 347156 [details] pushed as aa5738c - cogl: Add pixel_format_to_gl_with_target driver vfunc Attachment 347157 [details] pushed as 35388fb - cogl: Use pixel_format_to_gl_with_target on bitmap uploading paths
This feature seem to introduce another regression. When a clone of a window actor of a surface with a DRM_FORMAT_XRGB8888 OpenGL texture, the clone will be incorrectly displayed. Steps to reproduce: 1. Start playing a video file with mpv 2. Press f, to go fullscreen Result: During the fullscreen transition animation, the window content will have wrong colors
Created attachment 349031 [details] [review] cogl: Fix pixel_format_to_gl return value when swizzling The function is expected to return the cogl format that comes closest to the returned GL format, however since commit 1705a2 that's no longer the case if swizzling is used. So adjust the returned format to fix the few cases where the value is used (most notably find_best_gl_get_data_format).
Also when I make a screenshot of Gedit window with Solarized Light it's look like http://storage2.static.itmages.ru/i/17/0429/h_1493459064_8383947_35cfc84622.png Wrong colors http://storage8.static.itmages.ru/i/17/0429/h_1493459265_6740358_9592a183a8.png
(In reply to Florian Müllner from comment #18) > Created attachment 349031 [details] [review] [review] > cogl: Fix pixel_format_to_gl return value when swizzling > > The function is expected to return the cogl format that comes > closest to the returned GL format, however since commit 1705a2 > that's no longer the case if swizzling is used. So adjust the > returned format to fix the few cases where the value is used > (most notably find_best_gl_get_data_format). With this patch, screenshots of windows give the correct colors. But screenshots of areas using gnome-screenshot still show orange instead of blue.
Videos recorded using ctrl + alt +shit +r also have inverted colors.
(In reply to Hussam Al-Tayeb from comment #21) > Videos recorded using ctrl + alt +shit +r also have inverted colors. Screenshots and recording already work fine with non-nvidia drivers, the patch fixes a completely different case.
*** Bug 782588 has been marked as a duplicate of this bug. ***
*** Bug 782712 has been marked as a duplicate of this bug. ***
Created attachment 352118 [details] [review] cogl: Use pixel_format_to_gl_with_target in find_best_gl_get_data_format Fixes cogl_texture_get_data() resorting to the wrong conversions when extracting the texture data. This notably resulted in RGB/RGBA buffers copied as-is into BGRA buffers, for instance for the fullscreen animation, or single-window screenshots of such buffers.
I've tested this patch with the full range of surfaces we handle at MetaWaylandBuffer on little endian (BGRA for shm surfaces, plus RGB/RGBA for egl surfaces) with: - Fullscreen screenshot - Window screenshot - Area screenshot - Screencast - Fullscreen/unfullscreen animations It all seems to work correctly with it.
Review of attachment 352118 [details] [review]: OK
Pushed. Hopefully this is the last time we ever heard of it. Attachment 352118 [details] pushed as 374bb63 - cogl: Use pixel_format_to_gl_with_target in find_best_gl_get_data_format
*** Bug 780744 has been marked as a duplicate of this bug. ***
*** Bug 783127 has been marked as a duplicate of this bug. ***
*** Bug 784127 has been marked as a duplicate of this bug. ***
*** Bug 787026 has been marked as a duplicate of this bug. ***
Created attachment 360496 [details] Swapped color channels in window-only screenshot In mutter 3.26.0 under Wayland, when I use Alt+PrtSc to capture a GNOME Control Center window, the red and blue color channels are swapped. This does not happen if I capture the entire screen using only PrtSc, or if I'm running in Xorg. I don't have have swizzle according to GL_EXTENSIONS.
Reopening given the above comment. Another thing to mention is that this was when using a radeon card.
I always thought it was an issue taking a pic of a browser Window, but just repro'd it with one of the system-monitor using alt-printsc. My video card is Nvidia, using the commercial drivers.
*** Bug 792423 has been marked as a duplicate of this bug. ***
I am using intel + wayland + GNOME 3.26 and have the color problem too.
For people seeing this with Intel, this issue only appeared on recent mesa, https://gitlab.gnome.org/GNOME/mutter/issues/72 was reported around it and the fix is in 3.28/master. For people seeing this with other GPUs/drivers, the fix there will also work for you. The working theory is that mutter was compensating for an Intel DRI bug (mea culpa for not having more hw to test with) and GL drivers are now following the same page. Anyhow, this bug gets closed again. If this is ever seen again, please file a separate issue on gitlab, specifying chipset and mesa/GL driver.
*** Bug 793473 has been marked as a duplicate of this bug. ***