gpu: Investigate advertised GL readback format logic |
||||
Issue descriptionWe just noticed a few things that might not work as expected: 1) Looks like GL_BGRA is always supported as a readback format in desktop GL 2.1. However, we require the driver to advertise 'GL_EXT_bgra' in feature_info.cc. 2) The notion of this extension which implies that readback is always supported in this format is not supported in GLES. So we translate this extension to GL_EXT_read_format_bgra. But the semantics of this extension are different and require the client to do a IMPLEMENTATION_COLOR_READ_FORMAT query which will then return the preferred format. However, in the decoder on desktop we'd return RGBA even if the internal format is BGRA for the GetInteger() query. 3) Also in the query implementation in the decoder we don't check for 'is_es' but only issue the query if GL_OES_read_format is explicitly advertised although this is in the GLES2 and GLES3 core specs. 4) There is some confusion of how this format to be returned by the driver (and therefore also about how this is to be emulated by the decoder on desktop), because in the GLES2 spec it's referred to as an 'additional' format (as in additional to RGBA which is always supported), while table 6.34 in the ES3 spec calls it the 'preferred' format.
,
Mar 1 2016
Re #4: I think there are two options here: 1) In chrome, all uses of IMPLEMENTATION_COLOR_READ_FORMAT seem to treat it as an "additional format" not a preferred one... so we could always try to return something additional (not RGBA, as that is guaranteed). So an RGBA framebuffer might return BGRA, and a BGRA framebuffer would also return BGRA. 2) If we want to treat this as preferred, we might have cases where a client can't tell what additional, but non-preferred formats are supported... this is especially problematic on desktop where nothing is really too preferred, so we're sort of guessing. In this case we might return RGBA as the preferred format for RGBA, and BGRA as the preferred format for BGRA - this doesn't let the client know that RGBA can also be read back as BGRA. To solve this, we would likely want to add a gl extension GL_EXT_BGRA_READ_CHROMIUM or something, to let clients know that even if BGRA isn't "preferred", it's supported. #1 is definitely easier and is in line with the current uses of IMPLEMENTATION_COLOR_READ_FORMAT... WDYT?
,
Mar 1 2016
Secion 4.3.2 Reading Pixels of the GLEES3 spec also makes it sound more like 'additional'. I'm wondering if the table at the end (6.34) copied the 'preferred' language because from a desktop spec. We *could* invent a CHROMIUM extension that returns an array of supported FORMAT/TYPE pairs and say that the first one is the preferred one or something along those lines. On a desktop 4.5 service-side it'd enumerate the many allowed possibilities defined by that that spec and indicate the values defined by IMPLEMENTATION_COLOR_READ_FORMAT/_TYPE as the preferred pair. Not sure it is worth all that effort, though.
,
Mar 1 2016
Regarding point (3) - we actually do do the right thing - the logic we were looking at was for desktop only - for ES we fall back to querying GL directly.
,
Mar 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3211b4666e5354ddba6c60014837b434dabc7a88 commit 3211b4666e5354ddba6c60014837b434dabc7a88 Author: ericrk <ericrk@chromium.org> Date: Tue Mar 08 19:43:59 2016 Update GL_IMPLEMENTATION_COLOR_READ_FORMAT for BGRA GL_IMPLEMENTATION_COLOR_READ_FORMAT currently always returns RGBA for BGRA or RGBA buffers. To provide a more reasonable emulation of this ES feature on desktop, we should return BGRA readback for framebuffers whose internal format is BGRA. On GLES, this change has no impact, as the GL API is queried directly. BUG= 581311 , 591152 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Review URL: https://codereview.chromium.org/1759433002 Cr-Commit-Position: refs/heads/master@{#379890} [modify] https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88/content/common/gpu/client/gl_helper_readback_support.cc [modify] https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88/gpu/command_buffer/common/gles2_cmd_utils.cc [modify] https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88/gpu/command_buffer/common/gles2_cmd_utils.h [modify] https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88/gpu/command_buffer/service/feature_info.cc [modify] https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88/gpu/command_buffer/service/feature_info.h [modify] https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88/gpu/command_buffer/service/feature_info_unittest.cc [modify] https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88/gpu/command_buffer/service/gles2_cmd_decoder.cc
,
Jul 12 2016
,
Mar 3 2017
Eric, is this fixed by your CL from #5, or is there more to do here?
,
Mar 3 2017
I believe we're at the point we want to be with the CL in #5 - closing this out. |
||||
►
Sign in to add a comment |
||||
Comment 1 by siev...@chromium.org
, Mar 1 2016Isn't this also wrong in GLHelperReadbackSupport? if (format == GL_BGRA_EXT && type == GL_UNSIGNED_BYTE) { const GLubyte* tmp = gl_->GetString(GL_EXTENSIONS); if (tmp) { std::string extensions = " " + std::string(reinterpret_cast<const char*>(tmp)) + " "; if (extensions.find(" GL_EXT_read_format_bgra ") != std::string::npos) { return true; } } } The GL_EXT_read_format_bgra spec says that this is supported by IMPLEMENTATION_COLOR_READ_FORMAT/TYPE queries, not that the format is supported. No?