Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607373002:
Linear filter for texture object in 2d context (Closed)
|
Created:
3 years, 11 months ago by xlai (Olivia) Modified:
3 years, 11 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, Stephen Chennney, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, Rik, f(malita), blink-reviews, kinuko+watch, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinear filter for texture object in 2d context
This is a temporary fix to enforce linear filter for texture object
in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is
not urgent for OffscreenCanvas launch, is to adapt the filter to respect
image-rendering CSS property, as indicated in crbug.com/645590.
BUG=676666
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;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2607373002
Cr-Commit-Position: refs/heads/master@{#442803}
Committed: https://chromium.googlesource.com/chromium/src/+/7a66a4125991912014552fd1edbb58c37e9feede
Patch Set 1 #
Total comments: 1
Patch Set 2 : only need to change 1 place #Patch Set 3 : fix #
Total comments: 2
Patch Set 4 : remove duplicate code #Patch Set 5 : edited pixel expectations #Patch Set 6 : Mark mac and android failed #Patch Set 7 : fix #
Messages
Total messages: 35 (24 generated)
Description was changed from ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context BUG=676666 ========== to ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
Description was changed from ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context This is a temporary fix to enforce nearest_neighbor for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
xlai@chromium.org changed reviewers: + junov@chromium.org
This patch should be able to resolve crbug.com/676666, which is blocking OffscreenCanvas launch. The tricky part of fixing this via CSS is tracked by crbug.com/645590.
https://codereview.chromium.org/2607373002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2607373002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:204: resource.filter = GL_NEAREST; I don't understand how this fixes the problem. What we want is LINEAR filtering, not NEAREST. The problem was that the request for GL_LINEAR was being ignored for some reason.
After being confused for a few days by the fact that setting TransferableResource.filter to GL_LINEAR or GL_NEAREST will produce the exact opposite rendering effect for accelerated offscreen 2d context, I now realize that using this flag to force the texture filtering algorithm is not correct. Setting value on an incorrect will produce unexpected behavior which cannot be easily explained. Instead, quad->nearest_neighbor is the flag that is controlling the texture filter algorithm applied to the particular quad that we are committing. Setting it to true will result in sharp-edge image in accelerated 2d whilst setting it to false will result in blurry-edge image. I also double check all the other commit cases, including unaccelerated2d+gpu acceleration, unaccelerated2d+unaccelerated gpu, webgl accelerated, webgl with gpu unaccelerated. Their results are unaffected by the removal of this flag.
https://codereview.chromium.org/2607373002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2607373002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:271: const bool nearestNeighbor = false; What does this do?
I did a rebase just now; please ignore those extra changes due to rebase. https://codereview.chromium.org/2607373002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2607373002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:271: const bool nearestNeighbor = false; On 2017/01/06 20:06:01, Justin Novosad wrote: > What does this do? Oh it is actually doing the exact same thing as I'm adding below (didn't notice it earlier and I thought quad->nearest_neighbor is automatically default to false so I set its default value explicitly for readability). So I need to remove the below duplicate code. In summary, all commit cases will show blurry-edge images for resizing when the misleading resource.filter=GL_LINEAR is removed.
lgtm
The CQ bit was checked by xlai@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...
Description was changed from ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context This is a temporary fix to enforce nearest_neighbor for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context This is a temporary fix to enforce nearest_neighbor for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. TBR=zmo@chromium.org BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
xlai@chromium.org changed reviewers: + zmo@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xlai@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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context This is a temporary fix to enforce nearest_neighbor for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. TBR=zmo@chromium.org BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context This is a temporary fix to enforce nearest_neighbor for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
Description was changed from ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context This is a temporary fix to enforce nearest_neighbor for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context This is a temporary fix to enforce linear filter for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
+zmo@ for review of pixel tests. Also I have a question, is it possible to update an existing gpu reference image for certain platforms only? After enforcing linear filter on accelerated 2d, I find that for high-dpi devices (i.e., Mac Retina and Android) where the canvas image is scaled bigger to render, the image shows a little blurry edge between colors. I think this is expected as it is consistent with the resizing effect. Linux and Windows and other Mac machines that have normal dpi have their reference images matched. Therefore, I was trying to look for a way to update the reference image for Mac and Android and keep the current reference images for Linux and Windows unchanged. But I couldn't find a way. So I just increment the version count of Accelerated2D tests and rebaseline all platforms.
+zmo@ for review of pixel tests. Also I have a question, is it possible to update an existing gpu reference image for certain platforms only? After enforcing linear filter on accelerated 2d, I find that for high-dpi devices (i.e., Mac Retina and Android) where the canvas image is scaled bigger to render, the image shows a little blurry edge between colors. I think this is expected as it is consistent with the resizing effect. Linux and Windows and other Mac machines that have normal dpi have their reference images matched. Therefore, I was trying to look for a way to update the reference image for Mac and Android and keep the current reference images for Linux and Windows unchanged. But I couldn't find a way. So I just increment the version count of Accelerated2D tests and rebaseline all platforms.
On 2017/01/07 16:51:40, xlai (Olivia) wrote: > +zmo@ for review of pixel tests. Also I have a question, is it possible to > update an existing gpu reference image for certain platforms only? > > After enforcing linear filter on accelerated 2d, I find that for high-dpi > devices (i.e., Mac Retina and Android) where the canvas image is scaled bigger > to render, the image shows a little blurry edge between colors. I think this is > expected as it is consistent with the resizing effect. Linux and Windows and > other Mac machines that have normal dpi have their reference images matched. > > Therefore, I was trying to look for a way to update the reference > image for Mac and Android and keep the current reference > images for Linux and Windows unchanged. But I couldn't find a > way. So I just increment the version count of Accelerated2D tests > and rebaseline all platforms. stamp lgtm
Description was changed from ========== [OffscreenCanvas resize] Set nearest_neighbor for texture object in 2d context This is a temporary fix to enforce linear filter for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Linear filter for texture object in 2d context This is a temporary fix to enforce linear filter for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
The CQ bit was checked by xlai@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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xlai@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/2607373002/#ps120001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484108962531440,
"parent_rev": "e233068cdf299e2b78357576c68caa21866d7844", "commit_rev":
"7a66a4125991912014552fd1edbb58c37e9feede"}
Message was sent while issue was closed.
Description was changed from ========== Linear filter for texture object in 2d context This is a temporary fix to enforce linear filter for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Linear filter for texture object in 2d context This is a temporary fix to enforce linear filter for texture object in OffscreenCanvasRenderingContext2D in commit(). An ideal solution, which is not urgent for OffscreenCanvas launch, is to adapt the filter to respect image-rendering CSS property, as indicated in crbug.com/645590. BUG=676666 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;master.tryserver.chromium.android:android_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2607373002 Cr-Commit-Position: refs/heads/master@{#442803} Committed: https://chromium.googlesource.com/chromium/src/+/7a66a4125991912014552fd1edbb... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/7a66a4125991912014552fd1edbb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
