Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1320093002:
Command Buffer: read pixels into pixel pack buffer (Closed)
DescriptionCommand Buffer: read pixels into pixel pack buffer
TEST=conformance2/reading/read-pixels-into-pixel-pack-buffer.html, gpu_unittests
BUG=429053
Committed: https://crrev.com/0b1fc5d170a52839cd7cdba069c3b79bc0a4aa21
Cr-Commit-Position: refs/heads/master@{#365989}
Patch Set 1 #Patch Set 2 : run the existed validation for format and type #Patch Set 3 : add unittests #
Total comments: 2
Patch Set 4 : addressed the feedback from zmo@ and piman@, rebased the code #Patch Set 5 : more unittests #
Total comments: 6
Patch Set 6 : addressed zmo@'s feedback #Patch Set 7 : get correct pixel size #
Total comments: 12
Patch Set 8 : addressed zmo@'s feedback #
Total comments: 4
Patch Set 9 : addressed zmo@'s feedback: should use different functions to get pixels size #
Messages
Total messages: 44 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
yunchao.he@intel.com changed reviewers: + piman@chromium.org
Description was changed from ========== Command Buffer: read pixels into pixel pack buffer BUG=429053 ========== to ========== Command Buffer: read pixels into pixel pack buffer TEST=conformance2/reading/read-pixels-into-pixel-pack-buffer.html, gpu_unittests BUG=429053 ==========
Patchset #3 (id:80001) has been deleted
PTAL, Thanks a lot!
https://chromiumcodereview.appspot.com/1320093002/diff/100001/gpu/command_buf... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://chromiumcodereview.appspot.com/1320093002/diff/100001/gpu/command_buf... gpu/command_buffer/service/gles2_cmd_decoder.cc:8870: pixels = reinterpret_cast<void *>(c.pixels_shm_offset); We probably want to perform a bunch of extra validations here. 1) the bound buffer data store is not currently mapped 2) the bound buffer data store is large enough to write to Although the underlying driver also does the validation, but this is potentially out-of-bounds access, so probably we want to be on the safe side and perform this validation ourselves. piman: do you have an opinion on this?
On 2015/12/02 00:25:52, Zhenyao Mo wrote: > https://chromiumcodereview.appspot.com/1320093002/diff/100001/gpu/command_buf... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://chromiumcodereview.appspot.com/1320093002/diff/100001/gpu/command_buf... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8870: pixels = > reinterpret_cast<void *>(c.pixels_shm_offset); > We probably want to perform a bunch of extra validations here. > > 1) the bound buffer data store is not currently mapped > 2) the bound buffer data store is large enough to write to > > Although the underlying driver also does the validation, but this is potentially > out-of-bounds access, so probably we want to be on the safe side and perform > this validation ourselves. > > piman: do you have an opinion on this? Yes, we absolutely need to validate the bounds. The driver is not required to do so, and is allowed to crash or cause bad things.
Yunchao, note that I am landing this CL: https://codereview.chromium.org/1474513003/ You probably want to rebase your CL on top of mine.
On 2015/12/02 01:18:56, Zhenyao Mo wrote: > Yunchao, note that I am landing this CL: > https://codereview.chromium.org/1474513003/?_ga=1.64454447.10542079.1448347689 > > You probably want to rebase your CL on top of mine. Got it. Thank you, Zhenyao.
Thanks Zhenyao and piman. Your feedback are addressed. PTAL when you have time. Thanks a lot! https://codereview.chromium.org/1320093002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1320093002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8870: pixels = reinterpret_cast<void *>(c.pixels_shm_offset); On 2015/12/02 00:25:52, Zhenyao Mo wrote: > We probably want to perform a bunch of extra validations here. > > 1) the bound buffer data store is not currently mapped > 2) the bound buffer data store is large enough to write to > > Although the underlying driver also does the validation, but this is potentially > out-of-bounds access, so probably we want to be on the safe side and perform > this validation ourselves. > > piman: do you have an opinion on this? Done.
Mostly look good, but I will ask this CL to wait as I explained. piman: for the case we ReadPixels into a bound buffer but with x,y < 0 or width/height bigger than the image, do we need to worry about it? I am a bit nervous to just hand it down to the driver. https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... gpu/command_buffer/service/gles2_cmd_decoder.cc:8914: if (buffer->size() < pixels_size) { Right now the pixels_size we compute could be smaller than it actually requests, because in ComputeImageDataSizes we don't consider ES3 unpack parameters. This exposes a security risk that the buffer is not big enough but it passes the check here. I will have to ask this CL to wait until I land the CL I am working on to fix this size issue. https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... gpu/command_buffer/service/gles2_cmd_decoder.cc:8918: } It's worth checking the result_shm_id and result_shm_offset is both 0. Otherwise return error::kInvalidArgs. https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... gpu/command_buffer/service/gles2_cmd_decoder.cc:9155: if (c.pixels_shm_id != 0) { I think we should also avoid glGetError() if we read into a bound buffer.
On 2015/12/02 19:36:41, Zhenyao Mo wrote: > Mostly look good, but I will ask this CL to wait as I explained. > > piman: for the case we ReadPixels into a bound buffer but with x,y < 0 or > width/height bigger than the image, do we need to worry about it? I am a bit > nervous to just hand it down to the driver. Per offline discussion with piman, Chrome command buffer is designed not to leak undefined pixels, which if just passing the call to the underlying driver, the pixels outside the image are undefined. So here is the plan: we could clamp the x, y, width, height and call glReadPixels. In order to align with the memory, we will probably need to set PACK_ROW_LENGTH (and set it back afterwards). Still, we don't know what glReadPixels may do with extra pixels per row, etc. So it's safest if we then map the buffer, and write 0 to all pixels outside the image. This is quite some work, so I am OK you do it in another CL. > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8914: if (buffer->size() < > pixels_size) { > Right now the pixels_size we compute could be smaller than it actually requests, > because in ComputeImageDataSizes we don't consider ES3 unpack parameters. This > exposes a security risk that the buffer is not big enough but it passes the > check here. > > I will have to ask this CL to wait until I land the CL I am working on to fix > this size issue. > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8918: } > It's worth checking the result_shm_id and result_shm_offset is both 0. Otherwise > return error::kInvalidArgs. > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > gpu/command_buffer/service/gles2_cmd_decoder.cc:9155: if (c.pixels_shm_id != 0) > { > I think we should also avoid glGetError() if we read into a bound buffer.
On 2015/12/02 21:19:30, Zhenyao Mo wrote: > On 2015/12/02 19:36:41, Zhenyao Mo wrote: > > Mostly look good, but I will ask this CL to wait as I explained. > > > > piman: for the case we ReadPixels into a bound buffer but with x,y < 0 or > > width/height bigger than the image, do we need to worry about it? I am a bit > > nervous to just hand it down to the driver. > > Per offline discussion with piman, Chrome command buffer is designed not to leak > undefined pixels, which if just passing the call to the underlying driver, the > pixels outside the image are undefined. > > So here is the plan: we could clamp the x, y, width, height and call > glReadPixels. In order to align with the memory, we will probably need to set > PACK_ROW_LENGTH (and set it back afterwards). Also setting SKIP_PIXELS and SKIP_ROWS may be necessary. > > Still, we don't know what glReadPixels may do with extra pixels per row, etc. > So it's safest if we then map the buffer, and write 0 to all pixels outside the > image. > > This is quite some work, so I am OK you do it in another CL. > > > > > > > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:8914: if (buffer->size() < > > pixels_size) { > > Right now the pixels_size we compute could be smaller than it actually > requests, > > because in ComputeImageDataSizes we don't consider ES3 unpack parameters. This > > exposes a security risk that the buffer is not big enough but it passes the > > check here. > > > > I will have to ask this CL to wait until I land the CL I am working on to fix > > this size issue. > > > > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:8918: } > > It's worth checking the result_shm_id and result_shm_offset is both 0. > Otherwise > > return error::kInvalidArgs. > > > > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:9155: if (c.pixels_shm_id != > 0) > > { > > I think we should also avoid glGetError() if we read into a bound buffer.
Thank you, Zhenayao and piman. Code has been updated accordingly. PTAL if necessary... When you land your CL to resolve the pixel pack buffer size issue here. Thanks a lot! https://codereview.chromium.org/1320093002/diff/140001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1320093002/diff/140001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8914: if (buffer->size() < pixels_size) { On 2015/12/02 19:36:41, Zhenyao Mo wrote: > Right now the pixels_size we compute could be smaller than it actually requests, > because in ComputeImageDataSizes we don't consider ES3 unpack parameters. This > exposes a security risk that the buffer is not big enough but it passes the > check here. > > I will have to ask this CL to wait until I land the CL I am working on to fix > this size issue. Acknowledged. https://codereview.chromium.org/1320093002/diff/140001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8918: } On 2015/12/02 19:36:41, Zhenyao Mo wrote: > It's worth checking the result_shm_id and result_shm_offset is both 0. Otherwise > return error::kInvalidArgs. Done. https://codereview.chromium.org/1320093002/diff/140001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9155: if (c.pixels_shm_id != 0) { On 2015/12/02 19:36:41, Zhenyao Mo wrote: > I think we should also avoid glGetError() if we read into a bound buffer. Done.
On 2015/12/02 21:20:50, Zhenyao Mo wrote: > On 2015/12/02 21:19:30, Zhenyao Mo wrote: > > On 2015/12/02 19:36:41, Zhenyao Mo wrote: > > > Mostly look good, but I will ask this CL to wait as I explained. > > > > > > piman: for the case we ReadPixels into a bound buffer but with x,y < 0 or > > > width/height bigger than the image, do we need to worry about it? I am a bit > > > nervous to just hand it down to the driver. > > > > Per offline discussion with piman, Chrome command buffer is designed not to > leak > > undefined pixels, which if just passing the call to the underlying driver, the > > pixels outside the image are undefined. > > > > So here is the plan: we could clamp the x, y, width, height and call > > glReadPixels. In order to align with the memory, we will probably need to set > > PACK_ROW_LENGTH (and set it back afterwards). > > Also setting SKIP_PIXELS and SKIP_ROWS may be necessary. > > > > > Still, we don't know what glReadPixels may do with extra pixels per row, etc. > > So it's safest if we then map the buffer, and write 0 to all pixels outside > the > > image. > > > > This is quite some work, so I am OK you do it in another CL. > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:8914: if (buffer->size() < > > > pixels_size) { > > > Right now the pixels_size we compute could be smaller than it actually > > requests, > > > because in ComputeImageDataSizes we don't consider ES3 unpack parameters. > This > > > exposes a security risk that the buffer is not big enough but it passes the > > > check here. > > > > > > I will have to ask this CL to wait until I land the CL I am working on to > fix > > > this size issue. > > > > > > > > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:8918: } > > > It's worth checking the result_shm_id and result_shm_offset is both 0. > > Otherwise > > > return error::kInvalidArgs. > > > > > > > > > https://chromiumcodereview.appspot.com/1320093002/diff/140001/gpu/command_buf... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:9155: if (c.pixels_shm_id != > > 0) > > > { > > > I think we should also avoid glGetError() if we read into a bound buffer. OK, I will submit another CL to resolve the problem when reading pixels outside of the framebuffer. Thanks a lot!
Yunchao, now https://codereview.chromium.org/1508953002/ landed and you can finish this CL. You can do GLES2Util::ComputeImageDataSizeES3(..., state_.GetPackParams(), ...) to get the sizes you will need.
On 2015/12/09 17:38:42, Zhenyao Mo wrote: > Yunchao, now https://codereview.chromium.org/1508953002/ landed and you can > finish this CL. > > You can do GLES2Util::ComputeImageDataSizeES3(..., state_.GetPackParams(), ...) > to get the sizes you will need. Thanks for your reminder and your suggestions, Zhenyao. I have being aware of your patch. I will update this one soon.
On 2015/12/09 17:38:42, Zhenyao Mo wrote: > Yunchao, now https://codereview.chromium.org/1508953002/ landed and you can > finish this CL. > > You can do GLES2Util::ComputeImageDataSizeES3(..., state_.GetPackParams(), ...) > to get the sizes you will need. I have updated this change to call ComputeImageDataSizeES3. PTAL. Thanks a lot!
https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9180: width, height, 1, format, type, params, &pixels_size, NULL, NULL, NULL)) { nit: use nullptr instead. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9184: void* pixels = NULL; nit: use nullptr instead. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9201: pixels = reinterpret_cast<void *>(0); Here you will need to consider the offset parameter. Also, you need to validate that offset + pixels_size do not overflow and is the same or smaller than buffer->size() https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9203: LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels", Here you should return error::kInvalidArguments. No GL error. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9210: "pixel pack buffer should not bound"); Here you should return error::kInvalidArguments. No GL error. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9356: && c.pixels_shm_id != 0) { We do want to handle the out-of-bounds readPixels with PIXEL_PACK_BUFFER case as I mentioned previously. So for now, at least could you add a TODO and generate an INVALID_OPERATION to avoid undefined results?
Also, please fix the compile failure on some bots (due to unsigned/signed comparison)
Patchset #8 (id:200001) has been deleted
Patchset #8 (id:220001) has been deleted
Thanks for your review, Zhenyao. The code has been updated accordingly, PTAL. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9180: width, height, 1, format, type, params, &pixels_size, NULL, NULL, NULL)) { On 2015/12/15 23:50:56, Zhenyao Mo wrote: > nit: use nullptr instead. Done. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9184: void* pixels = NULL; On 2015/12/15 23:50:56, Zhenyao Mo wrote: > nit: use nullptr instead. Done. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9201: pixels = reinterpret_cast<void *>(0); On 2015/12/15 23:50:56, Zhenyao Mo wrote: > Here you will need to consider the offset parameter. > > Also, you need to validate that offset + pixels_size do not overflow and is the > same or smaller than buffer->size() Done. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9203: LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glReadPixels", On 2015/12/15 23:50:56, Zhenyao Mo wrote: > Here you should return error::kInvalidArguments. No GL error. Done. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9210: "pixel pack buffer should not bound"); On 2015/12/15 23:50:56, Zhenyao Mo wrote: > Here you should return error::kInvalidArguments. No GL error. Done. https://codereview.chromium.org/1320093002/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9356: && c.pixels_shm_id != 0) { On 2015/12/15 23:50:56, Zhenyao Mo wrote: > We do want to handle the out-of-bounds readPixels with PIXEL_PACK_BUFFER case as > I mentioned previously. So for now, at least could you add a TODO and generate > an INVALID_OPERATION to avoid undefined results? Done.
LGTM with one minor issue fixed. https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9199: if (buffer->size() < static_cast<int>(size)) { You should cast int to uint32 for comparison here.
https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9179: if (!GLES2Util::ComputeImageDataSizesES3(width, height, 1, format, type, Sorry just realized this is incorrect. You should only compute with all ES3 pack parameters if it's reading into bound pixel_pack_buffer. If it's reading into client buffer, we actually set everything to 0 (except for alignment) before making the glReadPixels calls. This makes sure we only send back meaningful pixel data to the command buffer client side, and the client side will take the responsibility to take the pixels and write to the client buffer according to the full ES3 pack parameters. So you should only call ComputeImageDataSizeES3 to get the pixels_size if a pixel_pack_buffer is bound. Otherwise, you should use the original ComputeImageDatSizes to compute the pixels_size.
On 2015/12/16 23:09:21, Zhenyao Mo wrote: > https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:9179: if > (!GLES2Util::ComputeImageDataSizesES3(width, height, 1, format, type, > Sorry just realized this is incorrect. You should only compute with all ES3 > pack parameters if it's reading into bound pixel_pack_buffer. If it's reading > into client buffer, we actually set everything to 0 (except for alignment) > before making the glReadPixels calls. This makes sure we only send back > meaningful pixel data to the command buffer client side, and the client side > will take the responsibility to take the pixels and write to the client buffer > according to the full ES3 pack parameters. > > So you should only call ComputeImageDataSizeES3 to get the pixels_size if a > pixel_pack_buffer is bound. Otherwise, you should use the original > ComputeImageDatSizes to compute the pixels_size. Thanks for your explanation, Zhenyao. Then I know why you use alignment only (not all pixelsStore parameters) in https://codereview.chromium.org/1516683007. I will update this change soon.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320093002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320093002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code has been updated. I also add your comments into the code snippet. Please have a look, Zhenyao and all. https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9179: if (!GLES2Util::ComputeImageDataSizesES3(width, height, 1, format, type, On 2015/12/16 23:09:21, Zhenyao Mo wrote: > Sorry just realized this is incorrect. You should only compute with all ES3 > pack parameters if it's reading into bound pixel_pack_buffer. If it's reading > into client buffer, we actually set everything to 0 (except for alignment) > before making the glReadPixels calls. This makes sure we only send back > meaningful pixel data to the command buffer client side, and the client side > will take the responsibility to take the pixels and write to the client buffer > according to the full ES3 pack parameters. > > So you should only call ComputeImageDataSizeES3 to get the pixels_size if a > pixel_pack_buffer is bound. Otherwise, you should use the original > ComputeImageDatSizes to compute the pixels_size. Done. https://codereview.chromium.org/1320093002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9199: if (buffer->size() < static_cast<int>(size)) { On 2015/12/16 18:12:34, Zhenyao Mo wrote: > You should cast int to uint32 for comparison here. Done.
lgtm
The CQ bit was checked by yunchao.he@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320093002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320093002/260001
Message was sent while issue was closed.
Description was changed from ========== Command Buffer: read pixels into pixel pack buffer TEST=conformance2/reading/read-pixels-into-pixel-pack-buffer.html, gpu_unittests BUG=429053 ========== to ========== Command Buffer: read pixels into pixel pack buffer TEST=conformance2/reading/read-pixels-into-pixel-pack-buffer.html, gpu_unittests BUG=429053 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Command Buffer: read pixels into pixel pack buffer TEST=conformance2/reading/read-pixels-into-pixel-pack-buffer.html, gpu_unittests BUG=429053 ========== to ========== Command Buffer: read pixels into pixel pack buffer TEST=conformance2/reading/read-pixels-into-pixel-pack-buffer.html, gpu_unittests BUG=429053 Committed: https://crrev.com/0b1fc5d170a52839cd7cdba069c3b79bc0a4aa21 Cr-Commit-Position: refs/heads/master@{#365989} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0b1fc5d170a52839cd7cdba069c3b79bc0a4aa21 Cr-Commit-Position: refs/heads/master@{#365989}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:260001) has been created in https://codereview.chromium.org/1540553003/ by jmadill@chromium.org. The reason for reverting is: Causes a timeout in Windows Debug, possibly an ASSERT: http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Debug%20%28NVIDI... WebglConformance.conformance2_reading_read_pixels_into_pixel_pack_buffer times out .
Message was sent while issue was closed.
On 2015/12/18 15:06:29, Jamie Madill wrote: > A revert of this CL (patchset #9 id:260001) has been created in > https://codereview.chromium.org/1540553003/ by mailto:jmadill@chromium.org. > > The reason for reverting is: Causes a timeout in Windows Debug, possibly an > ASSERT: > > http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Debug%20%28NVIDI... > > WebglConformance.conformance2_reading_read_pixels_into_pixel_pack_buffer times > out > . I verified on Linux Debug. There is no crash. We do have a failure but it's expected. I guess this indicates a bug in ANGLE. I'll reland this CL and suppress the test on Win Debug.
Message was sent while issue was closed.
On 2015/12/18 19:10:17, Zhenyao Mo wrote: > On 2015/12/18 15:06:29, Jamie Madill wrote: > > A revert of this CL (patchset #9 id:260001) has been created in > > https://codereview.chromium.org/1540553003/ by mailto:jmadill@chromium.org. > > > > The reason for reverting is: Causes a timeout in Windows Debug, possibly an > > ASSERT: > > > > > http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Debug%20%28NVIDI... > > > > WebglConformance.conformance2_reading_read_pixels_into_pixel_pack_buffer times > > out > > . > > I verified on Linux Debug. There is no crash. We do have a failure but it's > expected. > > I guess this indicates a bug in ANGLE. I'll reland this CL and suppress the > test on Win Debug. I found that you have re-landed this one, Zhenyao. Thank you. BTW, do you have time to write the patch which handle the pixels outside of the image when reading pixels into PIXEL_PACK buffer? I saw you discussed this situation and sent a pull request to khronos webgl spec & conformance project about reading/copying pixels outside of image. Recently I am fixing the bugs in negative*api.html in WebGL deqp tests, I want to finish that work first.
Message was sent while issue was closed.
On 2015/12/21 02:19:32, yunchao wrote: > On 2015/12/18 19:10:17, Zhenyao Mo wrote: > > On 2015/12/18 15:06:29, Jamie Madill wrote: > > > A revert of this CL (patchset #9 id:260001) has been created in > > > https://codereview.chromium.org/1540553003/ by mailto:jmadill@chromium.org. > > > > > > The reason for reverting is: Causes a timeout in Windows Debug, possibly an > > > ASSERT: > > > > > > > > > http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Debug%20%28NVIDI... > > > > > > WebglConformance.conformance2_reading_read_pixels_into_pixel_pack_buffer > times > > > out > > > . > > > > I verified on Linux Debug. There is no crash. We do have a failure but it's > > expected. > > > > I guess this indicates a bug in ANGLE. I'll reland this CL and suppress the > > test on Win Debug. > > I found that you have re-landed this one, Zhenyao. Thank you. BTW, do you have > time to write the patch which handle the pixels outside of the image when > reading pixels into PIXEL_PACK buffer? I saw you discussed this situation and > sent a pull request to khronos webgl spec & conformance project about > reading/copying pixels outside of image. Recently I am fixing the bugs in > negative*api.html in WebGL deqp tests, I want to finish that work first. Sure. I'll take this task. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
