Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308313008:
Fix ReadPixels implementation specific read format/type on desktop GL. (Closed)
|
Created:
5 years, 4 months ago by Zhenyao Mo Modified:
5 years, 3 months ago CC:
chromium-reviews, piman+watch_chromium.org, no sievers, vmiura Base URL:
https://chromium.googlesource.com/chromium/src.git@clear Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ReadPixels implementation specific read format/type on desktop GL.
BUG=429053
TEST=conformance2/reading/read-pixels-from-fbo-test.html
R=kbr@chromium.org,bajones@chromium.org
Committed: https://crrev.com/b3ccf244d9dab03ae656e3c8a825ad0caffa3981
Cr-Commit-Position: refs/heads/master@{#345805}
Patch Set 1 #
Total comments: 15
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 41 (15 generated)
kbr, bajones: PTAL Others: FYI
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308313008/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308313008/1
Very very nice. LGTM overall. Couple of questions. https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... File gpu/command_buffer/build_gles2_cmd_buffer.py (left): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:1513: 'deprecated_es3': [ What's the consequence of un-deprecating these? https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:1615: 'GL_UNSIGNED_SHORT_5_5_5_1', Similarly, what's the consequence of un-deprecating these? https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... File gpu/command_buffer/build_gles2_cmd_buffer.py (right): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:1518: 'GL_LUMINANCE_ALPHA', It looks to me like GL_LUMINANCE and GL_LUMINANCE_ALPHA are defined in Table 3.3 rather than 3.2 in the spec, and that the implementation color read format and type are defined as coming from Table 3.2. Can you verify? https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_utils.cc:841: case GL_RGBA8: Is the GL_RGBA8 intended? Looks like an accident. https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_utils.cc:890: // TODO(zmo): Return GL_HALF_FLOAT on ES3. Additionally -- I think that EXT_color_buffer_float on ES3 mandates that half-float color buffers actually be read back as type FLOAT, rather than HALF_FLOAT. Not sure whether the implementation color read type could be HALF_FLOAT for these.
kbr@chromium.org changed reviewers: + sievers@chromium.org
+sievers
https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... File gpu/command_buffer/build_gles2_cmd_buffer.py (left): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:1513: 'deprecated_es3': [ On 2015/08/26 01:00:44, Ken Russell wrote: > What's the consequence of un-deprecating these? It's a mistake to deprecate them in the first place. https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:1615: 'GL_UNSIGNED_SHORT_5_5_5_1', On 2015/08/26 01:00:43, Ken Russell wrote: > Similarly, what's the consequence of un-deprecating these? The same, table 3.3 (valid unsized ones) should also be included https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... File gpu/command_buffer/build_gles2_cmd_buffer.py (right): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:1518: 'GL_LUMINANCE_ALPHA', On 2015/08/26 01:00:43, Ken Russell wrote: > It looks to me like GL_LUMINANCE and GL_LUMINANCE_ALPHA are defined in Table 3.3 > rather than 3.2 in the spec, and that the implementation color read format and > type are defined as coming from Table 3.2. Can you verify? 3.2 defines the sized ones, and 3.3 defines the unsized ones. Both should be valid. https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_utils.cc:841: case GL_RGBA8: On 2015/08/26 01:00:44, Ken Russell wrote: > Is the GL_RGBA8 intended? Looks like an accident. It is intended to allow RGB readback for RGBA8 as an alternative. https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_utils.cc:890: // TODO(zmo): Return GL_HALF_FLOAT on ES3. On 2015/08/26 01:00:44, Ken Russell wrote: > Additionally -- I think that EXT_color_buffer_float on ES3 mandates that > half-float color buffers actually be read back as type FLOAT, rather than > HALF_FLOAT. Not sure whether the implementation color read type could be > HALF_FLOAT for these. Then the original code is a bug?
https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... File gpu/command_buffer/build_gles2_cmd_buffer.py (right): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:1518: 'GL_LUMINANCE_ALPHA', On 2015/08/26 01:11:04, Zhenyao Mo wrote: > On 2015/08/26 01:00:43, Ken Russell wrote: > > It looks to me like GL_LUMINANCE and GL_LUMINANCE_ALPHA are defined in Table > 3.3 > > rather than 3.2 in the spec, and that the implementation color read format and > > type are defined as coming from Table 3.2. Can you verify? > > 3.2 defines the sized ones, and 3.3 defines the unsized ones. Both should be > valid. Not sure about that. ES 3.0.4 section 4.4.4 "Framebuffer Completeness": "An internal format is color-renderable if it is one of the formats from table 3.13 noted as color-renderable or if it is unsized format RGBA or RGB. No other formats, including compressed internal formats, are color-renderable." I think this excludes LUMINANCE and LUMINANCE_ALPHA type textures as being valid framebuffer object color attachments, so it's not possible to call ReadPixels when a framebuffer is using that format and type. https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_utils.cc:841: case GL_RGBA8: On 2015/08/26 01:11:05, Zhenyao Mo wrote: > On 2015/08/26 01:00:44, Ken Russell wrote: > > Is the GL_RGBA8 intended? Looks like an accident. > > It is intended to allow RGB readback for RGBA8 as an alternative. OK. In that case "GetPreferredGLReadPixelsFormat" is a little confusing since it's really the preferred *implementation* read format. https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_utils.cc:890: // TODO(zmo): Return GL_HALF_FLOAT on ES3. On 2015/08/26 01:11:05, Zhenyao Mo wrote: > On 2015/08/26 01:00:44, Ken Russell wrote: > > Additionally -- I think that EXT_color_buffer_float on ES3 mandates that > > half-float color buffers actually be read back as type FLOAT, rather than > > HALF_FLOAT. Not sure whether the implementation color read type could be > > HALF_FLOAT for these. > > Then the original code is a bug? For ES 3.0, yes -- I think this is something that will need to be fixed when we expose EXT_color_buffer_float to WebGL 2.0 contexts. Worth expanding the TODO?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Revised. bajones: can you also take a look? https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_utils.cc:841: case GL_RGBA8: On 2015/08/26 01:53:32, Ken Russell wrote: > On 2015/08/26 01:11:05, Zhenyao Mo wrote: > > On 2015/08/26 01:00:44, Ken Russell wrote: > > > Is the GL_RGBA8 intended? Looks like an accident. > > > > It is intended to allow RGB readback for RGBA8 as an alternative. > > OK. In that case "GetPreferredGLReadPixelsFormat" is a little confusing since > it's really the preferred *implementation* read format. Switched to a different name. https://codereview.chromium.org/1308313008/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gles2_cmd_utils.cc:890: // TODO(zmo): Return GL_HALF_FLOAT on ES3. On 2015/08/26 01:53:33, Ken Russell wrote: > On 2015/08/26 01:11:05, Zhenyao Mo wrote: > > On 2015/08/26 01:00:44, Ken Russell wrote: > > > Additionally -- I think that EXT_color_buffer_float on ES3 mandates that > > > half-float color buffers actually be read back as type FLOAT, rather than > > > HALF_FLOAT. Not sure whether the implementation color read type could be > > > HALF_FLOAT for these. > > > > Then the original code is a bug? > > For ES 3.0, yes -- I think this is something that will need to be fixed when we > expose EXT_color_buffer_float to WebGL 2.0 contexts. Worth expanding the TODO? OK, I think I will return GL_FLOAT so the renderbuffer case is consistent with the texture case in WebGL 1 below. Add a TODO to consider return HALF_FLOAT in WebGL 2. I checked the spec and it is a possibility.
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308313008/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308313008/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1308313008/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1308313008/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:8783: accepted_types.push_back(GL_UNSIGNED_INT_2_10_10_10_REV); Don't we need a break right here?
https://codereview.chromium.org/1308313008/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1308313008/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:8783: accepted_types.push_back(GL_UNSIGNED_INT_2_10_10_10_REV); On 2015/08/26 23:39:48, bajones wrote: > Don't we need a break right here? No, RGB10_A2 is special. It will supports three format/type. So I put it here on purpose without a break.
On 2015/08/26 23:43:20, Zhenyao Mo wrote: > https://codereview.chromium.org/1308313008/diff/40001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/1308313008/diff/40001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8783: > accepted_types.push_back(GL_UNSIGNED_INT_2_10_10_10_REV); > On 2015/08/26 23:39:48, bajones wrote: > > Don't we need a break right here? > > No, RGB10_A2 is special. It will supports three format/type. So I put it here > on purpose without a break. LGTM, then, I'd prefer there be a comment explaining this, though. Otherwise I think someone may inadvertently add more case statements underneath it and really screw things up.
https://codereview.chromium.org/1308313008/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1308313008/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:8783: accepted_types.push_back(GL_UNSIGNED_INT_2_10_10_10_REV); On 2015/08/26 23:43:20, Zhenyao Mo wrote: > On 2015/08/26 23:39:48, bajones wrote: > > Don't we need a break right here? > > No, RGB10_A2 is special. It will supports three format/type. So I put it here > on purpose without a break. Good point. I will explicitly push the RGBA/UNSIGNED_BYTE here and break to avoid any future maintenance cost.
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308313008/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308313008/60001
One more concern. https://codereview.chromium.org/1308313008/diff/60001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1308313008/diff/60001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:891: return GL_FLOAT; This change may break existing WebGL 1.0 code which is using OES_texture_half_float and expecting to be able to do ReadPixels operations with format RGBA, type GL_HALF_FLOAT_OES. Let's please leave this code path unchanged, and have different behavior for WebGL 1.0 and 2.0 when we implement EXT_color_buffer_float for ES 3.0.
https://codereview.chromium.org/1308313008/diff/60001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/1308313008/diff/60001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:891: return GL_FLOAT; On 2015/08/26 23:58:36, Ken Russell wrote: > This change may break existing WebGL 1.0 code which is using > OES_texture_half_float and expecting to be able to do ReadPixels operations with > format RGBA, type GL_HALF_FLOAT_OES. Let's please leave this code path > unchanged, and have different behavior for WebGL 1.0 and 2.0 when we implement > EXT_color_buffer_float for ES 3.0. Another note: it looks like I was lazy and didn't check the implementation color read format and type readback path in sdk/tests/conformance/extensions/oes-texture-half-float.html in the WebGL conformance tests. oes-texture-float.html in the same directory does test RGBA/FLOAT readbacks. But since it's not guaranteed that these texture types are renderable, the conformance tests can't easily catch regressions in this area.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:70001) has been deleted
kbr: separate the ES2 and ES3 behaviors. Please take another look.
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308313008/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308313008/90001
As long as this still passes the oes-texture-float.html test and runs the
floating-point readback portions at the end of the test ("Checking readback of
floating-point values", "readPixels from floating-point renderbuffer should
succeed"), LGTM. Thanks for persisting with this.
https://codereview.chromium.org/1308313008/diff/90001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
https://codereview.chromium.org/1308313008/diff/90001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8806: }
I'm having difficulty understanding how the earlier code worked. It doesn't look
to me like it would advertise support for reading back RGBA/UNSIGNED_BYTE if a
floating-point framebuffer were bound -- but that would have broken the
conformance/extensions/oes-texture-float.html test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:110001) has been deleted
https://codereview.chromium.org/1308313008/diff/90001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1308313008/diff/90001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:8806: } On 2015/08/27 01:45:45, Ken Russell wrote: > I'm having difficulty understanding how the earlier code worked. It doesn't look > to me like it would advertise support for reading back RGBA/UNSIGNED_BYTE if a > floating-point framebuffer were bound -- but that would have broken the > conformance/extensions/oes-texture-float.html test. It turned out we never tested RGBA/UNSIGNED_BYTE read back with float/half_float textures in WebGL 1. That's why the bug is never caught.
The CQ bit was checked by zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/1308313008/#ps130001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308313008/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308313008/130001
Message was sent while issue was closed.
Committed patchset #5 (id:130001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b3ccf244d9dab03ae656e3c8a825ad0caffa3981 Cr-Commit-Position: refs/heads/master@{#345805} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
