From 008a12a6373559cbc0a85ef5ac5dcfb5358faaff Mon Sep 17 00:00:00 2001 From: Pekka Paalanen Date: Tue, 6 Nov 2018 16:27:52 +0200 Subject: [PATCH] cogl: pick glReadPixels format by target, not source Presumably glReadPixels itself can be more performant with pixel format conversions than doing a fix-up conversion on the CPU afterwards. Hence, pick required_format based on the destination rather than the source, so that it has a better chance to avoid the fix-up conversion. With CoglOnscreen objects, CoglFramebuffer::internal_format (the source format) is also wrong. It is left to a default value and never set to reflect the reality. In other words, read-pixels had an arbitrary intermediate pixel format that was used in glReadPixels and then fix-up conversion made it work for the destination. The render buffers (GBM surface) are allocated as DRM_FORMAT_XRGB8888. If the destination buffer is allocated as the same format, the Cogl read-pixels first converts with glReadPixels XRGB -> ABGR because of the above default format, and then the fix-up conversion does ABGR -> XRGB. This case was observed with DisplayLink outputs, where the native renderer must use the CPU copy path to fill the "secondary GPU" framebuffers. This patch stops using internal_format and uses the desired destination format instead. _cogl_framebuffer_gl_read_pixels_into_bitmap() will still use internal_format to determine alpha premultiplication state and multiply or un-multiply as needed. Luckily all the formats involved in the DisplayLink use case are always _PRE and so is the default internal_format too, so things work in practise. Furthermore, the GL texture_swizzle extension can never apply to glReadPixels. Not even with FBOs, as found in this discussion: https://gitlab.gnome.org/GNOME/mutter/issues/72 Therefore the target_format argument is hardcoded to something that can never match anything, which will prevent the swizzle from being assumed. --- cogl/cogl/driver/gl/cogl-framebuffer-gl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cogl/cogl/driver/gl/cogl-framebuffer-gl.c b/cogl/cogl/driver/gl/cogl-framebuffer-gl.c index 70cd4304b..e6e31a515 100644 --- a/cogl/cogl/driver/gl/cogl-framebuffer-gl.c +++ b/cogl/cogl/driver/gl/cogl-framebuffer-gl.c @@ -4,6 +4,7 @@ * A Low Level GPU Graphics and Utilities API * * Copyright (C) 2007,2008,2009,2012 Intel Corporation. + * Copyright (C) 2018 DisplayLink (UK) Ltd. * * Permission is hereby granted, free of charge, to any person * obtaining a copy of this software and associated documentation @@ -1273,9 +1274,12 @@ _cogl_framebuffer_gl_read_pixels_into_bitmap (CoglFramebuffer *framebuffer, if (!cogl_is_offscreen (framebuffer)) y = framebuffer_height - y - height; + /* Use target format ANY, because GL texture_swizzle extension cannot + * ever apply for glReadPixels. + */ required_format = ctx->driver_vtable->pixel_format_to_gl_with_target (ctx, - framebuffer->internal_format, format, + COGL_PIXEL_FORMAT_ANY, &gl_intformat, &gl_format, &gl_type);