Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
412453002:
GLHelper: Address inconsistent mapping of SkColorType to GL format (Closed)
|
Created:
6 years, 5 months ago by robert.bradford Modified:
6 years, 4 months ago CC:
chromium-reviews, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, epenner, Sami Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionGLHelper: Address inconsistent mapping of SkColorType to GL format
In Chromium on Linux, the native Skia format is BGRA8888, however GLHelper has
been mapping that to a GL_RGBA format, preventing the BGRA, creating a
confusing inconsistency and preventing true BGRA readback.
This change modifies GLHelperReadbackSupport to provide the canonical source of
mapping information from SkColorType to GL format, type and also bytes per
pixel, reducing the duplication through GLHelper.
The enum returned through the ReadbackConfig method provides information on
whether the color type is supported, supported only through an optional
accelerated swizzle the caller must do, or is not supported at all.
BUG=396002
TEST=content_gl_tests enhanced to test BGRA and RGBA readback where supported
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285281
Patch Set 1 #
Total comments: 44
Patch Set 2 : Integrate first feedback from piman@ (incomplete - do not review) #Patch Set 3 : Finish integrating review feedback (including enum move and formatting) #
Total comments: 1
Patch Set 4 : Integrate feedback (comment changes) #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper.cc (left): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:1036: swizzle_, I would prefer if we didn't query ReadbackConfig twice with the same parameters for 2 different reasons (which shader to use, and which format to read back in) and expect them to match. It's very confusing. Instead, you can either: 1- keep this, and DCHECK the format/type/bpp combination in CreateReadbackPipelineYUV (since we can't handle anything but GL_RGBA or GL_BGRA_EXT on this path, with GL_UNSIGNED_BYTE). Then in ReadbackPlane, you can choose the format based on this flag (GL_RGBA or GL_BGRA_EXT), and keep GL_UNSIGNED_BYTE/4 for type/bpp 2- pass the triplet all the way through. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:196: FormatSupport ReadbackConfig(SkColorType color_type, nit: naming... GetReadbackConfig ? https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:197: const bool& can_swizzle, use bool instead of const bool& https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:198: GLint& format, non-const references are forbidden by style guide. Use pointers. GLenum instead of GLint? https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:199: GLint& type, GLenum https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:200: int32& bytes_per_pixel); nit: size_t instead of int32. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:373: GLenum format = GL_RGBA , type = GL_UNSIGNED_BYTE; nit: no space before , https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:377: // independent of any desired readback layout I don't understand this comment. Does it only apply to both RGBA_8888/BGRA_8888? Can we move it to l.380? https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:417: int32 bytes_per_pixel, size_t https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:473: (supported == kReadbackSwizzle ? true : false), ? true : false unneeded. (supported == kReadbackSwizzle) is already a bool. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:575: kRGBA_8888_SkColorType, // GL_RGBA nit: 2 spaces before // https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:901: // Pass true in for swizzle to match earlier call What does this comment mean? https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper_readback_support.cc (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.cc:126: GLint new_format = 0, new_type = 0; nit: move these into the scope where they are needed (l.139) https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.cc:140: GetAdditionalFormat(format, type, &new_format, &new_type); Why do we need this path, given that we can always readback as RGBA/UB which matches kRGBA_8888_SkColorType? https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper_readback_support.h (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:14: enum FormatSupport { kReadbackSupported, nit: leave the enum in GLHelperReadbackSupport for scoping nit2: enum values should be MACRO_STYLE per style guide. e.g. READBACK_SUPPORTED nit3: formatting looks odd. Run 'git cl format' ? https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:33: // out values irrespective of the return value nit: end comment with a period. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:35: const bool& can_swizzle, nit: bool instead of const bool &. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:36: GLint& format, non-const references are forbidden by style guide. Use pointers. GLenum https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:37: GLint& type, GLenum https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:38: int32& bytes_per_pixel); size_t
Hi piman@ please you could take another look at this. I'm aware that there is possibly code relying on the old behaviour. If you could trigger a full set of tryjobs that would be appreciated. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper.cc (left): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:1036: swizzle_, On 2014/07/22 19:57:54, piman (slow to review) wrote: > I would prefer if we didn't query ReadbackConfig twice with the same parameters > for 2 different reasons (which shader to use, and which format to read back in) > and expect them to match. It's very confusing. > > Instead, you can either: > 1- keep this, and DCHECK the format/type/bpp combination in > CreateReadbackPipelineYUV (since we can't handle anything but GL_RGBA or > GL_BGRA_EXT on this path, with GL_UNSIGNED_BYTE). Then in ReadbackPlane, you can > choose the format based on this flag (GL_RGBA or GL_BGRA_EXT), and keep > GL_UNSIGNED_BYTE/4 for type/bpp I implemented option 1. here. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:196: FormatSupport ReadbackConfig(SkColorType color_type, On 2014/07/22 19:57:55, piman (slow to review) wrote: > nit: naming... GetReadbackConfig ? Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:197: const bool& can_swizzle, On 2014/07/22 19:57:54, piman (slow to review) wrote: > use bool instead of const bool& Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:198: GLint& format, On 2014/07/22 19:57:55, piman (slow to review) wrote: > non-const references are forbidden by style guide. Use pointers. > GLenum instead of GLint? Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:199: GLint& type, On 2014/07/22 19:57:54, piman (slow to review) wrote: > GLenum Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:200: int32& bytes_per_pixel); On 2014/07/22 19:57:55, piman (slow to review) wrote: > nit: size_t instead of int32. Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:373: GLenum format = GL_RGBA , type = GL_UNSIGNED_BYTE; On 2014/07/22 19:57:54, piman (slow to review) wrote: > nit: no space before , Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:377: // independent of any desired readback layout On 2014/07/22 19:57:55, piman (slow to review) wrote: > I don't understand this comment. Does it only apply to both RGBA_8888/BGRA_8888? > Can we move it to l.380? This is the initialisation of the texture for the creation of the pure copy and scale codepath as well as the scale and readback code path. The color_type that comes in has a somewhat overloaded meaning, both the texture format of the source as well as the destination readback format. However for the texture source/temporary texture all we really care about is if we're doing GL_RGB 16-bit or GL_RGBA 32-bit. I've modified the code to make this clearer along with a comment. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:417: int32 bytes_per_pixel, On 2014/07/22 19:57:54, piman (slow to review) wrote: > size_t Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:473: (supported == kReadbackSwizzle ? true : false), On 2014/07/22 19:57:55, piman (slow to review) wrote: > ? true : false unneeded. (supported == kReadbackSwizzle) is already a bool. Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:575: kRGBA_8888_SkColorType, // GL_RGBA On 2014/07/22 19:57:54, piman (slow to review) wrote: > nit: 2 spaces before // Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper.cc:901: // Pass true in for swizzle to match earlier call On 2014/07/22 19:57:55, piman (slow to review) wrote: > What does this comment mean? By adopting your proposed solution in the YUVReadbackImpl constructor this comment and call has now gone. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper_readback_support.cc (left): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.cc:88: bool GLHelperReadbackSupport::SupportsFormat(GLint format, GLint type) { I also needed to change this to GLenum too. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper_readback_support.cc (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.cc:126: GLint new_format = 0, new_type = 0; On 2014/07/22 19:57:55, piman (slow to review) wrote: > nit: move these into the scope where they are needed (l.139) Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.cc:140: GetAdditionalFormat(format, type, &new_format, &new_type); On 2014/07/22 19:57:55, piman (slow to review) wrote: > Why do we need this path, given that we can always readback as RGBA/UB which > matches kRGBA_8888_SkColorType? This is to handle the case (Intel) where the preferred format for readback is BGRA and if swizzling is possible then pre-swizzle. (This is the generalisation of the code that was previously in the YUV codepath only.) https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper_readback_support.h (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:14: enum FormatSupport { kReadbackSupported, On 2014/07/22 19:57:55, piman (slow to review) wrote: > nit: leave the enum in GLHelperReadbackSupport for scoping > nit2: enum values should be MACRO_STYLE per style guide. e.g. READBACK_SUPPORTED > nit3: formatting looks odd. Run 'git cl format' ? Enum moved in, and moved to MACRO_STYLE. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:33: // out values irrespective of the return value On 2014/07/22 19:57:55, piman (slow to review) wrote: > nit: end comment with a period. Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:35: const bool& can_swizzle, On 2014/07/22 19:57:55, piman (slow to review) wrote: > nit: bool instead of const bool &. Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:36: GLint& format, On 2014/07/22 19:57:56, piman (slow to review) wrote: > non-const references are forbidden by style guide. Use pointers. > GLenum Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:37: GLint& type, On 2014/07/22 19:57:56, piman (slow to review) wrote: > GLenum Done. https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.h:38: int32& bytes_per_pixel); On 2014/07/22 19:57:55, piman (slow to review) wrote: > size_t Done.
lgtm https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper_readback_support.cc (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.cc:140: GetAdditionalFormat(format, type, &new_format, &new_type); On 2014/07/23 16:58:45, robert.bradford wrote: > On 2014/07/22 19:57:55, piman (slow to review) wrote: > > Why do we need this path, given that we can always readback as RGBA/UB which > > matches kRGBA_8888_SkColorType? > > This is to handle the case (Intel) where the preferred format for readback is > BGRA and if swizzling is possible then pre-swizzle. (This is the generalisation > of the code that was previously in the YUV codepath only.) Ok, can you add a comment to that effect, that we take the presence of an additional format as a hint that we prefer it for performance? https://codereview.chromium.org/412453002/diff/40001/content/common/gpu/clien... File content/common/gpu/client/gl_helper_readback_support.cc (right): https://codereview.chromium.org/412453002/diff/40001/content/common/gpu/clien... content/common/gpu/client/gl_helper_readback_support.cc:25: // supported formats yet. nit: coalesce with previous line.
The CQ bit was checked by robert.bradford@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robert.bradford@intel.com/412453002/60001
Message was sent while issue was closed.
Change committed as 285281
Message was sent while issue was closed.
https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper_readback_support.cc (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.cc:140: GetAdditionalFormat(format, type, &new_format, &new_type); On 2014/07/23 18:07:38, piman (OOO) wrote: > On 2014/07/23 16:58:45, robert.bradford wrote: > > On 2014/07/22 19:57:55, piman (slow to review) wrote: > > > Why do we need this path, given that we can always readback as RGBA/UB which > > > matches kRGBA_8888_SkColorType? > > > > This is to handle the case (Intel) where the preferred format for readback is > > BGRA and if swizzling is possible then pre-swizzle. (This is the > generalisation > > of the code that was previously in the YUV codepath only.) > > Ok, can you add a comment to that effect, that we take the presence of an > additional format as a hint that we prefer it for performance? Just wondering.... why do you assume that the additional format is faster? The GLES2 spec seems to request that one other format is supported. But what if it's actually slower? On Android we use RGBA as the native skia byte ordering, and I assume the framebuffer internal byte order is usually also RGBA. Then reading back BGRA would always force the driver to swizzle somehow. Wouldn't that be more likely to be slower?
Message was sent while issue was closed.
https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... File content/common/gpu/client/gl_helper_readback_support.cc (right): https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... content/common/gpu/client/gl_helper_readback_support.cc:140: GetAdditionalFormat(format, type, &new_format, &new_type); On 2014/08/21 19:41:08, sievers wrote: > On 2014/07/23 18:07:38, piman (OOO) wrote: > > On 2014/07/23 16:58:45, robert.bradford wrote: > > > On 2014/07/22 19:57:55, piman (slow to review) wrote: > > > > Why do we need this path, given that we can always readback as RGBA/UB > which > > > > matches kRGBA_8888_SkColorType? > > > > > > This is to handle the case (Intel) where the preferred format for readback > is > > > BGRA and if swizzling is possible then pre-swizzle. (This is the > > generalisation > > > of the code that was previously in the YUV codepath only.) > > > > Ok, can you add a comment to that effect, that we take the presence of an > > additional format as a hint that we prefer it for performance? > > Just wondering.... why do you assume that the additional format is faster? The > GLES2 spec seems to request that one other format is supported. But what if it's > actually slower? > On Android we use RGBA as the native skia byte ordering, and I assume the > framebuffer internal byte order is usually also RGBA. Then reading back BGRA > would always force the driver to swizzle somehow. Wouldn't that be more likely > to be slower? Well, the spec doesn't say anything, but the intent of the GL_IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} is to return the internal native format for the buffer, or at least the faster one. If the buffer is actually GL_RGBA/GL_UNSIGNED_BYTE, that's what GL_IMPLEMENTATION_COLOR_READ_* should return. On Intel and ANGLE, the native format is GL_BGRA_EXT/GL_UNSIGNED_BYTE, and there's no fast path to readback + swizzle in the driver. If we find that there are drivers that return GL_BGRA_EXT/GL_UNSIGNED_BYTE for a RGBA buffer, then we'll need an extra piece of data / workaround entry to indicate whether or not we expect the GL_IMPLEMENTATION_COLOR_READ_* format to be slower.
Message was sent while issue was closed.
On 2014/08/21 20:00:02, piman (OOO) wrote: > https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... > File content/common/gpu/client/gl_helper_readback_support.cc (right): > > https://codereview.chromium.org/412453002/diff/1/content/common/gpu/client/gl... > content/common/gpu/client/gl_helper_readback_support.cc:140: > GetAdditionalFormat(format, type, &new_format, &new_type); > On 2014/08/21 19:41:08, sievers wrote: > > On 2014/07/23 18:07:38, piman (OOO) wrote: > > > On 2014/07/23 16:58:45, robert.bradford wrote: > > > > On 2014/07/22 19:57:55, piman (slow to review) wrote: > > > > > Why do we need this path, given that we can always readback as RGBA/UB > > which > > > > > matches kRGBA_8888_SkColorType? > > > > > > > > This is to handle the case (Intel) where the preferred format for readback > > is > > > > BGRA and if swizzling is possible then pre-swizzle. (This is the > > > generalisation > > > > of the code that was previously in the YUV codepath only.) > > > > > > Ok, can you add a comment to that effect, that we take the presence of an > > > additional format as a hint that we prefer it for performance? > > > > Just wondering.... why do you assume that the additional format is faster? The > > GLES2 spec seems to request that one other format is supported. But what if > it's > > actually slower? > > On Android we use RGBA as the native skia byte ordering, and I assume the > > framebuffer internal byte order is usually also RGBA. Then reading back BGRA > > would always force the driver to swizzle somehow. Wouldn't that be more likely > > to be slower? > > Well, the spec doesn't say anything, but the intent of the > GL_IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} is to return the internal native > format for the buffer, or at least the faster one. If the buffer is actually > GL_RGBA/GL_UNSIGNED_BYTE, that's what GL_IMPLEMENTATION_COLOR_READ_* should > return. > > On Intel and ANGLE, the native format is GL_BGRA_EXT/GL_UNSIGNED_BYTE, and > there's no fast path to readback + swizzle in the driver. > > If we find that there are drivers that return GL_BGRA_EXT/GL_UNSIGNED_BYTE for a > RGBA buffer, then we'll need an extra piece of data / workaround entry to > indicate whether or not we expect the GL_IMPLEMENTATION_COLOR_READ_* format to > be slower. Ok, yea, I haven't really looked at what drivers actually report but am just a bit worried because in the GLES2 spec in 4.3.1 it says that drivers must support RGBA plus one other format which may be queried with GL_IMPLEMENTATION_COLOR_READ_FORMAT. So it seems that the additional format would always be slower to read back (unless the hw internally actually uses BGRA, but then all our texture uploads would have to end up swizzling internally, meh?).
On Thu, Aug 21, 2014 at 1:19 PM, <sievers@chromium.org> wrote: > On 2014/08/21 20:00:02, piman (OOO) wrote: > > https://codereview.chromium.org/412453002/diff/1/content/ > common/gpu/client/gl_helper_readback_support.cc > >> File content/common/gpu/client/gl_helper_readback_support.cc (right): >> > > > https://codereview.chromium.org/412453002/diff/1/content/ > common/gpu/client/gl_helper_readback_support.cc#newcode140 > >> content/common/gpu/client/gl_helper_readback_support.cc:140: >> GetAdditionalFormat(format, type, &new_format, &new_type); >> On 2014/08/21 19:41:08, sievers wrote: >> > On 2014/07/23 18:07:38, piman (OOO) wrote: >> > > On 2014/07/23 16:58:45, robert.bradford wrote: >> > > > On 2014/07/22 19:57:55, piman (slow to review) wrote: >> > > > > Why do we need this path, given that we can always readback as >> RGBA/UB >> > which >> > > > > matches kRGBA_8888_SkColorType? >> > > > >> > > > This is to handle the case (Intel) where the preferred format for >> > readback > >> > is >> > > > BGRA and if swizzling is possible then pre-swizzle. (This is the >> > > generalisation >> > > > of the code that was previously in the YUV codepath only.) >> > > >> > > Ok, can you add a comment to that effect, that we take the presence >> of an >> > > additional format as a hint that we prefer it for performance? >> > >> > Just wondering.... why do you assume that the additional format is >> faster? >> > The > >> > GLES2 spec seems to request that one other format is supported. But >> what if >> it's >> > actually slower? >> > On Android we use RGBA as the native skia byte ordering, and I assume >> the >> > framebuffer internal byte order is usually also RGBA. Then reading back >> BGRA >> > would always force the driver to swizzle somehow. Wouldn't that be more >> > likely > >> > to be slower? >> > > Well, the spec doesn't say anything, but the intent of the >> GL_IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} is to return the internal >> native >> format for the buffer, or at least the faster one. If the buffer is >> actually >> GL_RGBA/GL_UNSIGNED_BYTE, that's what GL_IMPLEMENTATION_COLOR_READ_* >> should >> return. >> > > On Intel and ANGLE, the native format is GL_BGRA_EXT/GL_UNSIGNED_BYTE, and >> there's no fast path to readback + swizzle in the driver. >> > > If we find that there are drivers that return >> GL_BGRA_EXT/GL_UNSIGNED_BYTE for >> > a > >> RGBA buffer, then we'll need an extra piece of data / workaround entry to >> indicate whether or not we expect the GL_IMPLEMENTATION_COLOR_READ_* >> format to >> be slower. >> > > Ok, yea, I haven't really looked at what drivers actually report but am > just a > bit worried because in the GLES2 spec in 4.3.1 it says that drivers must > support > RGBA plus one other format which may be queried with > GL_IMPLEMENTATION_COLOR_READ_FORMAT. > So it seems that the additional format would always be slower to read back > (unless the hw internally actually uses BGRA, but then all our texture > uploads > would have to end up swizzling internally, meh?). > The *whole* point of the additional format is to allow to be faster (at the cost of more code / less portability, since you need to figure out if you can handle the extra format). Antoine > > https://codereview.chromium.org/412453002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/08/21 20:32:17, piman (OOO) wrote: > On Thu, Aug 21, 2014 at 1:19 PM, <mailto:sievers@chromium.org> wrote: > > > On 2014/08/21 20:00:02, piman (OOO) wrote: > > > > https://codereview.chromium.org/412453002/diff/1/content/ > > common/gpu/client/gl_helper_readback_support.cc > > > >> File content/common/gpu/client/gl_helper_readback_support.cc (right): > >> > > > > > > https://codereview.chromium.org/412453002/diff/1/content/ > > common/gpu/client/gl_helper_readback_support.cc#newcode140 > > > >> content/common/gpu/client/gl_helper_readback_support.cc:140: > >> GetAdditionalFormat(format, type, &new_format, &new_type); > >> On 2014/08/21 19:41:08, sievers wrote: > >> > On 2014/07/23 18:07:38, piman (OOO) wrote: > >> > > On 2014/07/23 16:58:45, robert.bradford wrote: > >> > > > On 2014/07/22 19:57:55, piman (slow to review) wrote: > >> > > > > Why do we need this path, given that we can always readback as > >> RGBA/UB > >> > which > >> > > > > matches kRGBA_8888_SkColorType? > >> > > > > >> > > > This is to handle the case (Intel) where the preferred format for > >> > > readback > > > >> > is > >> > > > BGRA and if swizzling is possible then pre-swizzle. (This is the > >> > > generalisation > >> > > > of the code that was previously in the YUV codepath only.) > >> > > > >> > > Ok, can you add a comment to that effect, that we take the presence > >> of an > >> > > additional format as a hint that we prefer it for performance? > >> > > >> > Just wondering.... why do you assume that the additional format is > >> faster? > >> > > The > > > >> > GLES2 spec seems to request that one other format is supported. But > >> what if > >> it's > >> > actually slower? > >> > On Android we use RGBA as the native skia byte ordering, and I assume > >> the > >> > framebuffer internal byte order is usually also RGBA. Then reading back > >> BGRA > >> > would always force the driver to swizzle somehow. Wouldn't that be more > >> > > likely > > > >> > to be slower? > >> > > > > Well, the spec doesn't say anything, but the intent of the > >> GL_IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} is to return the internal > >> native > >> format for the buffer, or at least the faster one. If the buffer is > >> actually > >> GL_RGBA/GL_UNSIGNED_BYTE, that's what GL_IMPLEMENTATION_COLOR_READ_* > >> should > >> return. > >> > > > > On Intel and ANGLE, the native format is GL_BGRA_EXT/GL_UNSIGNED_BYTE, and > >> there's no fast path to readback + swizzle in the driver. > >> > > > > If we find that there are drivers that return > >> GL_BGRA_EXT/GL_UNSIGNED_BYTE for > >> > > a > > > >> RGBA buffer, then we'll need an extra piece of data / workaround entry to > >> indicate whether or not we expect the GL_IMPLEMENTATION_COLOR_READ_* > >> format to > >> be slower. > >> > > > > Ok, yea, I haven't really looked at what drivers actually report but am > > just a > > bit worried because in the GLES2 spec in 4.3.1 it says that drivers must > > support > > RGBA plus one other format which may be queried with > > GL_IMPLEMENTATION_COLOR_READ_FORMAT. > > So it seems that the additional format would always be slower to read back > > (unless the hw internally actually uses BGRA, but then all our texture > > uploads > > would have to end up swizzling internally, meh?). > > > > The *whole* point of the additional format is to allow to be faster (at the > cost of more code / less portability, since you need to figure out if you > can handle the extra format). > > Antoine > I guess the main source of confusion for me is then the simultaneous hard requirement for supporting the extra, different format. > > > > > https://codereview.chromium.org/412453002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/08/21 20:34:42, sievers wrote: > On 2014/08/21 20:32:17, piman (OOO) wrote: > > On Thu, Aug 21, 2014 at 1:19 PM, <mailto:sievers@chromium.org> wrote: > > > > > On 2014/08/21 20:00:02, piman (OOO) wrote: > > > > > > https://codereview.chromium.org/412453002/diff/1/content/ > > > common/gpu/client/gl_helper_readback_support.cc > > > > > >> File content/common/gpu/client/gl_helper_readback_support.cc (right): > > >> > > > > > > > > > https://codereview.chromium.org/412453002/diff/1/content/ > > > common/gpu/client/gl_helper_readback_support.cc#newcode140 > > > > > >> content/common/gpu/client/gl_helper_readback_support.cc:140: > > >> GetAdditionalFormat(format, type, &new_format, &new_type); > > >> On 2014/08/21 19:41:08, sievers wrote: > > >> > On 2014/07/23 18:07:38, piman (OOO) wrote: > > >> > > On 2014/07/23 16:58:45, robert.bradford wrote: > > >> > > > On 2014/07/22 19:57:55, piman (slow to review) wrote: > > >> > > > > Why do we need this path, given that we can always readback as > > >> RGBA/UB > > >> > which > > >> > > > > matches kRGBA_8888_SkColorType? > > >> > > > > > >> > > > This is to handle the case (Intel) where the preferred format for > > >> > > > readback > > > > > >> > is > > >> > > > BGRA and if swizzling is possible then pre-swizzle. (This is the > > >> > > generalisation > > >> > > > of the code that was previously in the YUV codepath only.) > > >> > > > > >> > > Ok, can you add a comment to that effect, that we take the presence > > >> of an > > >> > > additional format as a hint that we prefer it for performance? > > >> > > > >> > Just wondering.... why do you assume that the additional format is > > >> faster? > > >> > > > The > > > > > >> > GLES2 spec seems to request that one other format is supported. But > > >> what if > > >> it's > > >> > actually slower? > > >> > On Android we use RGBA as the native skia byte ordering, and I assume > > >> the > > >> > framebuffer internal byte order is usually also RGBA. Then reading back > > >> BGRA > > >> > would always force the driver to swizzle somehow. Wouldn't that be more > > >> > > > likely > > > > > >> > to be slower? > > >> > > > > > > Well, the spec doesn't say anything, but the intent of the > > >> GL_IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} is to return the internal > > >> native > > >> format for the buffer, or at least the faster one. If the buffer is > > >> actually > > >> GL_RGBA/GL_UNSIGNED_BYTE, that's what GL_IMPLEMENTATION_COLOR_READ_* > > >> should > > >> return. > > >> > > > > > > On Intel and ANGLE, the native format is GL_BGRA_EXT/GL_UNSIGNED_BYTE, and > > >> there's no fast path to readback + swizzle in the driver. > > >> > > > > > > If we find that there are drivers that return > > >> GL_BGRA_EXT/GL_UNSIGNED_BYTE for > > >> > > > a > > > > > >> RGBA buffer, then we'll need an extra piece of data / workaround entry to > > >> indicate whether or not we expect the GL_IMPLEMENTATION_COLOR_READ_* > > >> format to > > >> be slower. > > >> > > > > > > Ok, yea, I haven't really looked at what drivers actually report but am > > > just a > > > bit worried because in the GLES2 spec in 4.3.1 it says that drivers must > > > support > > > RGBA plus one other format which may be queried with > > > GL_IMPLEMENTATION_COLOR_READ_FORMAT. > > > So it seems that the additional format would always be slower to read back > > > (unless the hw internally actually uses BGRA, but then all our texture > > > uploads > > > would have to end up swizzling internally, meh?). > > > > > > > The *whole* point of the additional format is to allow to be faster (at the > > cost of more code / less portability, since you need to figure out if you > > can handle the extra format). > > > > Antoine > > > > I guess the main source of confusion for me is then the simultaneous hard > requirement for supporting the extra, different format. > > Ok I think I see now... the additional format does not have to be different at all in GLES2. And it has to be from table 3.4 (but not luminance or luminance_alpha). And that table does not have BGRA, but has RGBA (and RGB24, 4444, 565 and alpha). So we should not hit this codepath on GLES2 even? > > > > > > > > https://codereview.chromium.org/412453002/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Thu, Aug 21, 2014 at 1:43 PM, <sievers@chromium.org> wrote: > On 2014/08/21 20:34:42, sievers wrote: > >> On 2014/08/21 20:32:17, piman (OOO) wrote: >> > On Thu, Aug 21, 2014 at 1:19 PM, <mailto:sievers@chromium.org> wrote: >> > >> > > On 2014/08/21 20:00:02, piman (OOO) wrote: >> > > >> > > https://codereview.chromium.org/412453002/diff/1/content/ >> > > common/gpu/client/gl_helper_readback_support.cc >> > > >> > >> File content/common/gpu/client/gl_helper_readback_support.cc >> (right): >> > >> >> > > >> > > >> > > https://codereview.chromium.org/412453002/diff/1/content/ >> > > common/gpu/client/gl_helper_readback_support.cc#newcode140 >> > > >> > >> content/common/gpu/client/gl_helper_readback_support.cc:140: >> > >> GetAdditionalFormat(format, type, &new_format, &new_type); >> > >> On 2014/08/21 19:41:08, sievers wrote: >> > >> > On 2014/07/23 18:07:38, piman (OOO) wrote: >> > >> > > On 2014/07/23 16:58:45, robert.bradford wrote: >> > >> > > > On 2014/07/22 19:57:55, piman (slow to review) wrote: >> > >> > > > > Why do we need this path, given that we can always readback >> as >> > >> RGBA/UB >> > >> > which >> > >> > > > > matches kRGBA_8888_SkColorType? >> > >> > > > >> > >> > > > This is to handle the case (Intel) where the preferred format >> for >> > >> >> > > readback >> > > >> > >> > is >> > >> > > > BGRA and if swizzling is possible then pre-swizzle. (This is >> the >> > >> > > generalisation >> > >> > > > of the code that was previously in the YUV codepath only.) >> > >> > > >> > >> > > Ok, can you add a comment to that effect, that we take the >> presence >> > >> of an >> > >> > > additional format as a hint that we prefer it for performance? >> > >> > >> > >> > Just wondering.... why do you assume that the additional format is >> > >> faster? >> > >> >> > > The >> > > >> > >> > GLES2 spec seems to request that one other format is supported. But >> > >> what if >> > >> it's >> > >> > actually slower? >> > >> > On Android we use RGBA as the native skia byte ordering, and I >> assume >> > >> the >> > >> > framebuffer internal byte order is usually also RGBA. Then reading >> back >> > >> BGRA >> > >> > would always force the driver to swizzle somehow. Wouldn't that be >> more >> > >> >> > > likely >> > > >> > >> > to be slower? >> > >> >> > > >> > > Well, the spec doesn't say anything, but the intent of the >> > >> GL_IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} is to return the internal >> > >> native >> > >> format for the buffer, or at least the faster one. If the buffer is >> > >> actually >> > >> GL_RGBA/GL_UNSIGNED_BYTE, that's what GL_IMPLEMENTATION_COLOR_READ_* >> > >> should >> > >> return. >> > >> >> > > >> > > On Intel and ANGLE, the native format is >> GL_BGRA_EXT/GL_UNSIGNED_BYTE, >> > and > >> > >> there's no fast path to readback + swizzle in the driver. >> > >> >> > > >> > > If we find that there are drivers that return >> > >> GL_BGRA_EXT/GL_UNSIGNED_BYTE for >> > >> >> > > a >> > > >> > >> RGBA buffer, then we'll need an extra piece of data / workaround >> entry to >> > >> indicate whether or not we expect the GL_IMPLEMENTATION_COLOR_READ_* >> > >> format to >> > >> be slower. >> > >> >> > > >> > > Ok, yea, I haven't really looked at what drivers actually report but >> am >> > > just a >> > > bit worried because in the GLES2 spec in 4.3.1 it says that drivers >> must >> > > support >> > > RGBA plus one other format which may be queried with >> > > GL_IMPLEMENTATION_COLOR_READ_FORMAT. >> > > So it seems that the additional format would always be slower to read >> back >> > > (unless the hw internally actually uses BGRA, but then all our texture >> > > uploads >> > > would have to end up swizzling internally, meh?). >> > > >> > >> > The *whole* point of the additional format is to allow to be faster (at >> the >> > cost of more code / less portability, since you need to figure out if >> you >> > can handle the extra format). >> > >> > Antoine >> > >> > > I guess the main source of confusion for me is then the simultaneous hard >> requirement for supporting the extra, different format. >> > > > > Ok I think I see now... the additional format does not have to be > different at > all in GLES2. > Right. > And it has to be from table 3.4 (but not luminance or luminance_alpha). > And that table does not have BGRA, but has RGBA (and RGB24, 4444, 565 and > alpha). > Right, GL_BGRA_EXT is defined in an extension, not stock GLES2. > So we should not hit this codepath on GLES2 even? If EXT_read_format_bgra is supported, it's added to the table. Antoine > > > > >> > > >> > > https://codereview.chromium.org/412453002/ >> > > >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/412453002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
