Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165913003:
Invalidate m_image after GPU-GPU texture copy (Closed)
|
Created:
4 years, 5 months ago by xidachen Modified:
4 years, 5 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Rik, f(malita), blink-reviews, Stephen Chennney, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis, Ken Russell (switch to Gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInvalidate m_image after GPU-GPU texture copy
The image_bitmap_from_canvas conformance1 tests are failing with WebGL
2.0. It fails in this case:
Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then
call texSubImage2D(ImageBitmap), which does GPU readback. In the call
to texSubImage2D, there is a error says glTexSubImage2D: invalid
internalformat/format/type combination, which doesn't make sense.
The solution here is to set m_image to be null after GPU-GPU texture
copy. So next time when m_image is needed, it has to use a new context
provider to consume the mailbox to get a new m_image.
To make sure this CL does the right thing, I have tested the following:
conformance1 tests with WebGL 1.0 and 2.0 (with GPU accelerated canvas)
conformance2 tests (with GPU accelerated canvas, WebGL 2.0 only)
layout tests:
fast/canvas/,
virtual/gpu/fast/canvas,
virtual/display_list_2d_canvas/fast/canvas.
All the above tests pass.
When committing this CL, we should add the optional GPU bots to make
sure they are green.
BUG=628954
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/f5c988930b7a2f2aacd7cabf2573caeed2770b69
Cr-Commit-Position: refs/heads/master@{#406660}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #Patch Set 3 : update WebGL2 expectation #Patch Set 4 : 3 formats still fails on windows #
Messages
Total messages: 27 (14 generated)
Description was changed from ========== Invalidate m_image after GPU-GPU texture copy The image_bitmap_from_canvas conformance1 tests are failing with WebGL 2.0. It fails in this case: Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then call texSubImage2D(ImageBitmap), which does GPU readback. In the call to texSubImage2D, there is a error says glTexSubImage2D: invalid internalformat/format/type combination, which doesn't make sense. The solution here is to set m_image to be null after GPU-GPU texture copy. So next time when m_image is needed, it has to use a new context provider to consume the mailbox to get a new m_image. To make sure this CL does the right thing, I have tested the following: conformance1 tests with WebGL 1.0 and 2.0 conformance2 tests (WebGL 2.0 only) layout tests: fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/fast/canvas. All the above tests pass. When committing this CL, we should add the optional GPU bots to make sure they are green. BUG=628954 ========== to ========== Invalidate m_image after GPU-GPU texture copy The image_bitmap_from_canvas conformance1 tests are failing with WebGL 2.0. It fails in this case: Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then call texSubImage2D(ImageBitmap), which does GPU readback. In the call to texSubImage2D, there is a error says glTexSubImage2D: invalid internalformat/format/type combination, which doesn't make sense. The solution here is to set m_image to be null after GPU-GPU texture copy. So next time when m_image is needed, it has to use a new context provider to consume the mailbox to get a new m_image. To make sure this CL does the right thing, I have tested the following: conformance1 tests with WebGL 1.0 and 2.0 (with GPU accelerated canvas) conformance2 tests (WebGL 2.0 only) layout tests: fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/fast/canvas. All the above tests pass. When committing this CL, we should add the optional GPU bots to make sure they are green. BUG=628954 ==========
xidachen@chromium.org changed reviewers: + junov@chromium.org
Description was changed from ========== Invalidate m_image after GPU-GPU texture copy The image_bitmap_from_canvas conformance1 tests are failing with WebGL 2.0. It fails in this case: Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then call texSubImage2D(ImageBitmap), which does GPU readback. In the call to texSubImage2D, there is a error says glTexSubImage2D: invalid internalformat/format/type combination, which doesn't make sense. The solution here is to set m_image to be null after GPU-GPU texture copy. So next time when m_image is needed, it has to use a new context provider to consume the mailbox to get a new m_image. To make sure this CL does the right thing, I have tested the following: conformance1 tests with WebGL 1.0 and 2.0 (with GPU accelerated canvas) conformance2 tests (WebGL 2.0 only) layout tests: fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/fast/canvas. All the above tests pass. When committing this CL, we should add the optional GPU bots to make sure they are green. BUG=628954 ========== to ========== Invalidate m_image after GPU-GPU texture copy The image_bitmap_from_canvas conformance1 tests are failing with WebGL 2.0. It fails in this case: Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then call texSubImage2D(ImageBitmap), which does GPU readback. In the call to texSubImage2D, there is a error says glTexSubImage2D: invalid internalformat/format/type combination, which doesn't make sense. The solution here is to set m_image to be null after GPU-GPU texture copy. So next time when m_image is needed, it has to use a new context provider to consume the mailbox to get a new m_image. To make sure this CL does the right thing, I have tested the following: conformance1 tests with WebGL 1.0 and 2.0 (with GPU accelerated canvas) conformance2 tests (with GPU accelerated canvas, WebGL 2.0 only) layout tests: fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/fast/canvas. All the above tests pass. When committing this CL, we should add the optional GPU bots to make sure they are green. BUG=628954 ==========
PTAL
https://codereview.chromium.org/2165913003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2165913003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:116: m_image = nullptr; I think a better place for this would be at the end of switchStorageToMailbox
https://codereview.chromium.org/2165913003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2165913003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:116: m_image = nullptr; On 2016/07/20 15:07:52, Justin Novosad wrote: > I think a better place for this would be at the end of switchStorageToMailbox I don't think that would work. I tried, and what happened is this: at the end of switchStorageToMailbox, we set m_image = nullptr; After that, we need to call gl->CopyTextureCHROMIUM(), that fails with error: GL ERROR :GL_INVALID_VALUE : glCopyTextureCHROMIUM: unknown texture id I believe that in order to have a valid texture id, the m_image has to be alive. Does that make sense?
https://codereview.chromium.org/2165913003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2165913003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:116: m_image = nullptr; Sorry. Here you need to call switchStorageToMailbox, and at the end of switchStorageToMailbox is where you'd reset m_image to null. The fix you have now loses the texture forever. Also, for clarity, I think textureIdForWebGL should be renamed switchStorageToSkImageForWebGL
https://codereview.chromium.org/2165913003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp (right): https://codereview.chromium.org/2165913003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:116: m_image = nullptr; On 2016/07/20 15:51:59, Justin Novosad wrote: > Sorry. Here you need to call switchStorageToMailbox, and at the end of > switchStorageToMailbox is where you'd reset m_image to null. The fix you have > now loses the texture forever. > > Also, for clarity, I think textureIdForWebGL should be renamed > switchStorageToSkImageForWebGL Right, that makes perfect sense, changed in the new PS.
lgtm
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! Can you also remove the related test expectations in content/test/gpu/gpu_tests/webgl2_conformance_expectations.py?
The CQ bit was unchecked by xidachen@chromium.org
Description was changed from ========== Invalidate m_image after GPU-GPU texture copy The image_bitmap_from_canvas conformance1 tests are failing with WebGL 2.0. It fails in this case: Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then call texSubImage2D(ImageBitmap), which does GPU readback. In the call to texSubImage2D, there is a error says glTexSubImage2D: invalid internalformat/format/type combination, which doesn't make sense. The solution here is to set m_image to be null after GPU-GPU texture copy. So next time when m_image is needed, it has to use a new context provider to consume the mailbox to get a new m_image. To make sure this CL does the right thing, I have tested the following: conformance1 tests with WebGL 1.0 and 2.0 (with GPU accelerated canvas) conformance2 tests (with GPU accelerated canvas, WebGL 2.0 only) layout tests: fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/fast/canvas. All the above tests pass. When committing this CL, we should add the optional GPU bots to make sure they are green. BUG=628954 ========== to ========== Invalidate m_image after GPU-GPU texture copy The image_bitmap_from_canvas conformance1 tests are failing with WebGL 2.0. It fails in this case: Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then call texSubImage2D(ImageBitmap), which does GPU readback. In the call to texSubImage2D, there is a error says glTexSubImage2D: invalid internalformat/format/type combination, which doesn't make sense. The solution here is to set m_image to be null after GPU-GPU texture copy. So next time when m_image is needed, it has to use a new context provider to consume the mailbox to get a new m_image. To make sure this CL does the right thing, I have tested the following: conformance1 tests with WebGL 1.0 and 2.0 (with GPU accelerated canvas) conformance2 tests (with GPU accelerated canvas, WebGL 2.0 only) layout tests: fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/fast/canvas. All the above tests pass. When committing this CL, we should add the optional GPU bots to make sure they are green. BUG=628954 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
xidachen@chromium.org changed reviewers: + kbr@chromium.org, zmo@chromium.org
zmo@, kbr@: could you also LGTM now that I have changed the webgl2_expectation file? Mac is fine now, there are three formats fails on Win, I have to take a deeper look into that.
On 2016/07/20 18:35:26, xidachen wrote: > zmo@, kbr@: could you also LGTM now that I have changed the webgl2_expectation > file? > > Mac is fine now, there are three formats fails on Win, I have to take a deeper > look into that. lgtm
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/2165913003/#ps60001 (title: "3 formats still fails on windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Invalidate m_image after GPU-GPU texture copy The image_bitmap_from_canvas conformance1 tests are failing with WebGL 2.0. It fails in this case: Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then call texSubImage2D(ImageBitmap), which does GPU readback. In the call to texSubImage2D, there is a error says glTexSubImage2D: invalid internalformat/format/type combination, which doesn't make sense. The solution here is to set m_image to be null after GPU-GPU texture copy. So next time when m_image is needed, it has to use a new context provider to consume the mailbox to get a new m_image. To make sure this CL does the right thing, I have tested the following: conformance1 tests with WebGL 1.0 and 2.0 (with GPU accelerated canvas) conformance2 tests (with GPU accelerated canvas, WebGL 2.0 only) layout tests: fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/fast/canvas. All the above tests pass. When committing this CL, we should add the optional GPU bots to make sure they are green. BUG=628954 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Invalidate m_image after GPU-GPU texture copy The image_bitmap_from_canvas conformance1 tests are failing with WebGL 2.0. It fails in this case: Call texImage2D(ImageBitmap), which does GPU-GPU texture copy. Then call texSubImage2D(ImageBitmap), which does GPU readback. In the call to texSubImage2D, there is a error says glTexSubImage2D: invalid internalformat/format/type combination, which doesn't make sense. The solution here is to set m_image to be null after GPU-GPU texture copy. So next time when m_image is needed, it has to use a new context provider to consume the mailbox to get a new m_image. To make sure this CL does the right thing, I have tested the following: conformance1 tests with WebGL 1.0 and 2.0 (with GPU accelerated canvas) conformance2 tests (with GPU accelerated canvas, WebGL 2.0 only) layout tests: fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/fast/canvas. All the above tests pass. When committing this CL, we should add the optional GPU bots to make sure they are green. BUG=628954 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/f5c988930b7a2f2aacd7cabf2573caeed2770b69 Cr-Commit-Position: refs/heads/master@{#406660} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f5c988930b7a2f2aacd7cabf2573caeed2770b69 Cr-Commit-Position: refs/heads/master@{#406660}
Message was sent while issue was closed.
LGTM after the fact. Sorry for the delay reviewing. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
