Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1759433002:
Update GL_IMPLEMENTATION_COLOR_READ_FORMAT for BGRA (Closed)
DescriptionUpdate 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
Committed: https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88
Cr-Commit-Position: refs/heads/master@{#379890}
Patch Set 1 #Patch Set 2 : Remove no longer needed check from GLHelperReadbackSupport::SupportsFormat #
Total comments: 3
Patch Set 3 : feedback #
Total comments: 3
Patch Set 4 : feedback #
Total comments: 3
Messages
Total messages: 27 (10 generated)
Description was changed from ========== Update GL_IMPLEMENTATION_COLOR_READ_FORMAT for BGRA GL_IMPLEMENTATION_COLOR_READ_FORMAT currently always returns RGBA for BGRA or RGBA buffers. From looking at the spec, this value should return a second format (in addition to GL_RGBA), for which read is supported. Given this, it's relatively clear that GL_BGRA buffers should return GL_BGRA as a supported read format. Looking at the spec, it's less clear what should be returned for GL_RGBA buffers. GL_RGBA readback is guaranteed, and the spec indicates that this function should return an "second" format. The spec also indicates that this "second" format is "preferred", which makes things a bit less clear. That said, looking at this function from a utility standpoint, on a platform like desktop where both BGRA and RGBA are efficiently supported, this function presents our only opportunity for communicating to the client that they can read their RGBA framebuffer as either BGRA or RGBA. With this in mind, this change also returns BGRA for RGBA buffers. On GLES, this change has no impact, as the GL API is queried directly. BUG=581311,591152 ========== to ========== Update GL_IMPLEMENTATION_COLOR_READ_FORMAT for BGRA GL_IMPLEMENTATION_COLOR_READ_FORMAT currently always returns RGBA for BGRA or RGBA buffers. From looking at the spec, this value should return a second format (in addition to GL_RGBA), for which read is supported. Given this, it's relatively clear that GL_BGRA buffers should return GL_BGRA as a supported read format. Looking at the spec, it's less clear what should be returned for GL_RGBA buffers. GL_RGBA readback is guaranteed, and the spec indicates that this function should return an "second" format. The spec also indicates that this "second" format is "preferred", which makes things a bit less clear. That said, looking at this function from a utility standpoint, on a platform like desktop where both BGRA and RGBA are efficiently supported, this function presents our only opportunity for communicating to the client that they can read their RGBA framebuffer as either BGRA or RGBA. With this in mind, this change also returns BGRA for RGBA buffers. 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 ==========
ericrk@chromium.org changed reviewers: + bsalomon@google.com, piman@chromium.org, sievers@chromium.org
https://codereview.chromium.org/1759433002/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gl_helper_readback_support.cc (left): https://codereview.chromium.org/1759433002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gl_helper_readback_support.cc:107: if (format == GL_BGRA_EXT && type == GL_UNSIGNED_BYTE) { The presence of GL_EXT_read_format_bgra doesn't guarantee that BGRA readback is supported, it just indicates that it may be returned from GL_IMPLEMENTATION_COLOR_READ_FORMAT. We should always be calling that function in all cases. We used to avoid doing so to work around an issue where the driver would never report BGRA from GL_IMPLEMENTATION_COLOR_READ_FORMAT, but that issue is resolved with this CL.
The code in GLHelperReadbackSupport::GetReadbackConfig will try to use the additional format, assuming it is prefered. This CL would return BGRA for RGBA buffers, so I think this could regress performance (extra swizzling in the driver). https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:984: // should return a second, implementaiton-chosen format. For consumers of typo: implementaiton->implementation https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:1034: return GL_BGRA_EXT; We're only allowed to return this if we expose GL_EXT_read_format_bgra to the client - otherwise, the client would try to use it, but glReadPixels would fail. See the code around enable_read_format_bgra in feature_info.cc
On 2016/03/04 20:55:37, piman - OOO back 2016-03-04 wrote: > The code in GLHelperReadbackSupport::GetReadbackConfig will try to use the > additional format, assuming it is prefered. This CL would return BGRA for RGBA > buffers, so I think this could regress performance (extra swizzling in the > driver). > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > File gpu/command_buffer/common/gles2_cmd_utils.cc (right): > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > gpu/command_buffer/common/gles2_cmd_utils.cc:984: // should return a second, > implementaiton-chosen format. For consumers of > typo: implementaiton->implementation > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > gpu/command_buffer/common/gles2_cmd_utils.cc:1034: return GL_BGRA_EXT; > We're only allowed to return this if we expose GL_EXT_read_format_bgra to the > client - otherwise, the client would try to use it, but glReadPixels would fail. > See the code around enable_read_format_bgra in feature_info.cc Ah, missed that part of GLHelperReadbackSupport - it seems we have a few options to deal with this: 1) Tweak GLHelperReadbackSupport (although that seems like it may remove a necessary optimization). 2) Tweak the code here to only return BGRA if the framebuffer's internal_format is BGRA (should solve the immediate Skia issue... but doesn't give Skia the desired ability to know all viable readback formats) If we go with option 2, we should file a follow up to expose an additional GL extension which will indicate to Skia that BGRA is available for readback. Note that GL_EXT_read_format_bgra is not sufficient, as the extension indicates that the format *may* be supported, but you still must call GL_IMPLEMENTATION_COLOR_READ_FORMAT to find out if BGRA is usable on the current framebuffer. Do you have any preferences here?
On 2016/03/04 21:59:43, ericrk wrote: > On 2016/03/04 20:55:37, piman - OOO back 2016-03-04 wrote: > > The code in GLHelperReadbackSupport::GetReadbackConfig will try to use the > > additional format, assuming it is prefered. This CL would return BGRA for RGBA > > buffers, so I think this could regress performance (extra swizzling in the > > driver). > > > > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > > File gpu/command_buffer/common/gles2_cmd_utils.cc (right): > > > > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > > gpu/command_buffer/common/gles2_cmd_utils.cc:984: // should return a second, > > implementaiton-chosen format. For consumers of > > typo: implementaiton->implementation > > > > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > > gpu/command_buffer/common/gles2_cmd_utils.cc:1034: return GL_BGRA_EXT; > > We're only allowed to return this if we expose GL_EXT_read_format_bgra to the > > client - otherwise, the client would try to use it, but glReadPixels would > fail. > > See the code around enable_read_format_bgra in feature_info.cc > > Ah, missed that part of GLHelperReadbackSupport - it seems we have a few options > to deal with this: > 1) Tweak GLHelperReadbackSupport (although that seems like it may remove a > necessary optimization). > 2) Tweak the code here to only return BGRA if the framebuffer's internal_format > is BGRA (should solve the immediate Skia issue... but doesn't give Skia the > desired ability to know all viable readback formats) > > If we go with option 2, we should file a follow up to expose an additional GL > extension which will indicate to Skia that BGRA is available for readback. Note > that GL_EXT_read_format_bgra is not sufficient, as the extension > indicates that the format *may* be supported, but you still must call > GL_IMPLEMENTATION_COLOR_READ_FORMAT to find out if BGRA is usable on the current > framebuffer. > > Do you have any preferences here? Although... in practice GL_EXT_read_format_bgra may be enough for Skia to assume it can read back BGRA, even though the spec doesn't seem to guarantee it in all cases.... not sure?
On Fri, Mar 4, 2016 at 1:59 PM, <ericrk@chromium.org> wrote: > On 2016/03/04 20:55:37, piman - OOO back 2016-03-04 wrote: > > The code in GLHelperReadbackSupport::GetReadbackConfig will try to use > the > > additional format, assuming it is prefered. This CL would return BGRA > for RGBA > > buffers, so I think this could regress performance (extra swizzling in > the > > driver). > > > > > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > > File gpu/command_buffer/common/gles2_cmd_utils.cc (right): > > > > > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > > gpu/command_buffer/common/gles2_cmd_utils.cc:984: // should return a > second, > > implementaiton-chosen format. For consumers of > > typo: implementaiton->implementation > > > > > > https://codereview.chromium.org/1759433002/diff/20001/gpu/command_buffer/comm... > > gpu/command_buffer/common/gles2_cmd_utils.cc:1034: return GL_BGRA_EXT; > > We're only allowed to return this if we expose GL_EXT_read_format_bgra > to the > > client - otherwise, the client would try to use it, but glReadPixels > would > fail. > > See the code around enable_read_format_bgra in feature_info.cc > > Ah, missed that part of GLHelperReadbackSupport - it seems we have a few > options > to deal with this: > 1) Tweak GLHelperReadbackSupport (although that seems like it may remove a > necessary optimization). > 2) Tweak the code here to only return BGRA if the framebuffer's > internal_format > is BGRA (should solve the immediate Skia issue... but doesn't give Skia the > desired ability to know all viable readback formats) > > If we go with option 2, we should file a follow up to expose an additional > GL > extension which will indicate to Skia that BGRA is available for readback. > Note > that GL_EXT_read_format_bgra is not sufficient, as the extension > indicates that the format *may* be supported, but you still must call > GL_IMPLEMENTATION_COLOR_READ_FORMAT to find out if BGRA is usable on the > current > framebuffer. > > Do you have any preferences here? > I think my preference would be: 1- Have GL_IMPLEMENTATION_COLOR_READ_FORMAT return the preferred format: - if on ES, return the driver's GL_IMPLEMENTATION_COLOR_READ_FORMAT (if we understand what it is, and we allow it, otherwise RGBA) - if on desktop GL, return our best guess (if internal format is BGRA and we expose GL_EXT_read_format_bgra, return BGRA, otherwise RGBA) - possibly on desktop >= 4.3 we could query GL_IMPLEMENTATION_COLOR_READ_FORMAT 2- GLHelperReadbackSupport always uses GL_IMPLEMENTATION_COLOR_READ_FORMAT It means we may return RGBA for GL_IMPLEMENTATION_COLOR_READ_FORMAT, but I don't think it's a problem. > > https://codereview.chromium.org/1759433002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Update GL_IMPLEMENTATION_COLOR_READ_FORMAT for BGRA GL_IMPLEMENTATION_COLOR_READ_FORMAT currently always returns RGBA for BGRA or RGBA buffers. From looking at the spec, this value should return a second format (in addition to GL_RGBA), for which read is supported. Given this, it's relatively clear that GL_BGRA buffers should return GL_BGRA as a supported read format. Looking at the spec, it's less clear what should be returned for GL_RGBA buffers. GL_RGBA readback is guaranteed, and the spec indicates that this function should return an "second" format. The spec also indicates that this "second" format is "preferred", which makes things a bit less clear. That said, looking at this function from a utility standpoint, on a platform like desktop where both BGRA and RGBA are efficiently supported, this function presents our only opportunity for communicating to the client that they can read their RGBA framebuffer as either BGRA or RGBA. With this in mind, this change also returns BGRA for RGBA buffers. 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 ========== to ========== 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 ==========
re-worked this based on feedback - let me know how this looks. https://codereview.chromium.org/1759433002/diff/80001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1759433002/diff/80001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:984: bool supports_bgra) { could pass the context in as well (and have the extension querying internal to this fn), but that requires dependency changes.
https://codereview.chromium.org/1759433002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1759433002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:5119: context_->HasExtension("GL_EXT_read_format_bgra")); Actually, here you're only asking the underlying GL context, not what we expose to the client. That is, you'd pass false on desktop GL, when you'd want to pass true. My suggestion is to add a ext_read_format_bgra field in FeatureInfo::FeatureFlags, that would be set in feature_info.cc where we enable the extension (similar to, e.g. ext_texture_format_bgra8888) and use that here.
https://codereview.chromium.org/1759433002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1759433002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:5119: context_->HasExtension("GL_EXT_read_format_bgra")); On 2016/03/07 21:04:14, piman wrote: > Actually, here you're only asking the underlying GL context, not what we expose > to the client. That is, you'd pass false on desktop GL, when you'd want to pass > true. > > My suggestion is to add a ext_read_format_bgra field in > FeatureInfo::FeatureFlags, that would be set in feature_info.cc where we enable > the extension (similar to, e.g. ext_texture_format_bgra8888) and use that here. ah, yeah, missed that - thanks! Added this to a feature flag and updated.
lgtm
The CQ bit was checked by ericrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1759433002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1759433002/100001
https://codereview.chromium.org/1759433002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1759433002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:5119: feature_info_->feature_flags().ext_read_format_bgra); Don't we strictly speaking also have to check whether we support GL_EXT_bgra? Sure we don't want to be noncompliant wrt the client by returning BGRA here without having advertised GL_EXT_read_format_bgra, but isn't it a bit fragile and relying on this being implied here (because we are in a desktop-only path here and because of the logic in feature_info.cc).
https://codereview.chromium.org/1759433002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1759433002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:5119: feature_info_->feature_flags().ext_read_format_bgra); On 2016/03/08 00:26:28, sievers wrote: > Don't we strictly speaking also have to check whether we support GL_EXT_bgra? > > Sure we don't want to be noncompliant wrt the client by returning BGRA here > without having advertised GL_EXT_read_format_bgra, but isn't it a bit fragile > and relying on this being implied here (because we are in a desktop-only path > here and because of the logic in feature_info.cc). And also note that checking the internal format is not good enough because you can have GL_EXT_texture_format_BGRA8888 but not support the same host memory format.
The CQ bit was unchecked by ericrk@chromium.org
https://codereview.chromium.org/1759433002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1759433002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:5119: feature_info_->feature_flags().ext_read_format_bgra); On 2016/03/08 00:28:44, sievers wrote: > On 2016/03/08 00:26:28, sievers wrote: > > Don't we strictly speaking also have to check whether we support GL_EXT_bgra? > > > > Sure we don't want to be noncompliant wrt the client by returning BGRA here > > without having advertised GL_EXT_read_format_bgra, but isn't it a bit fragile > > and relying on this being implied here (because we are in a desktop-only path > > here and because of the logic in feature_info.cc). > > And also note that checking the internal format is not good enough because you > can have GL_EXT_texture_format_BGRA8888 but not support the same host memory > format. On desktop GL, the format/type is orthogonal to the internal format in the sense that you can mix & match BGRA vs RGBA, and the driver is responsible for the translation (for both read and write). We only get here if we support full GL_EXT_bgra, I think?
On 2016/03/08 00:48:07, piman wrote: > We only get here if we support full GL_EXT_bgra, I think? I think so too, but it's not completely obvious. Then again, I just realized that BGRA as a readback format is part of core GL 2.1, so we can probably just assume that we have it here in the is-not-gles path. (Though we might also want to skip checking for GL_EXT_BGRA in general in feature_info.cc on desktop.) lgtm
The CQ bit was checked by ericrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1759433002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1759433002/100001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88 Cr-Commit-Position: refs/heads/master@{#379890} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3211b4666e5354ddba6c60014837b434dabc7a88 Cr-Commit-Position: refs/heads/master@{#379890} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
