New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 733691 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Compat



Sign in to add a comment

SVG shape-rendering: crispEdges is ignored

Reported by sky...@gmail.com, Jun 15 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.86 Safari/537.36

Example URL:
https://codepen.io/sky87/pen/bRBqBB

Steps to reproduce the problem:
Apply "shape-rendering: crispEdges" to a vertical SVG line with an integer x coordinate

What is the expected behavior?

What went wrong?
The line should not be antialiased.
The rendering is correct in Firefox (compare the attached images).

Does it occur on multiple sites: N/A

Is it a problem with a plugin? N/A 

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 59.0.3071.86  Channel: stable
OS Version: 10.0
Flash Version:
 
chrome.PNG
198 bytes View Download
firefox.PNG
205 bytes View Download

Comment 1 by hdodda@chromium.org, Jun 16 2017

Cc: hdodda@chromium.org
Components: Internals>GPU>Rasterization
Labels: hasbisect-per-revision M-60
Owner: ericrk@chromium.org
Status: Assigned (was: Unconfirmed)
Tested the issue on windows 10 using chrome M59 #59.0.3071.104 and issue is not reproduced , issue is reproduced in latest canary M61 #61.0.3131.0 .

Issue is a regression issue broken in M60 and is reproducible in chrome dev , beta and canary channels and is reproduced only on windows.

You are probably looking for a change made after 467197 (known good), but no later than 467198 (first known bad).

CHANGELOG URL:

The script might not always return single CL as suspectas some perf builds might get missing due to failure.

 https://chromium.googlesource.com/chromium/src/+log/f91ad00d0d78fe00a5d1ca4fc50ace6cd63718a7..4166ab857753a66e9ba6aea475ada0f14b0971d8

From the CL above, assigning the issue to the concern owner 

@ericrk- Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Review-Url: https://codereview.chromium.org/2844483003

Thanks!

Comment 2 by hdodda@chromium.org, Jun 16 2017

Labels: ReleaseBlock-Stable
Adding RBS-label for now , as it seems to be recent regression. Please feel free to edit/remove this.

Thanks!

Comment 3 by hdodda@chromium.org, Jun 16 2017

Typo error in comment #1, 

Good build: 60.0.3080.0 (Revision: 466837).
Bad build: 60.0.3082.0 (Revision: 467534).

Comment 4 by ericrk@chromium.org, Jun 16 2017

Cc: ericrk@chromium.org bsalomon@chromium.org
Owner: senorblanco@chromium.org
This appears to be an issue with MSAA path rendering in Ganesh. Non-MSAA Ganesh produces the correct result. See attached image.

My guess is that the tessellating path renderer is not snapping to full pixels when crisp edges is enabled, leading to antialiasing where there should be none (due to MSAA).

senorblanco@, can you take a look?
msaa_comparison.png
2.9 KB View Download
It doesn't look the the concave tessellating path renderer is used here, since the path is a 1px-wide line and convex (I enabled logging in the tessellator to verify).

I don't see incorrect pixels on my Linux box with --enable-gpu-rasterization, but the line is offset by a pixel to the left.

Brian, any thoughts? Seems like line drawing is doing something funky in MSAA mode.
Labels: -ReleaseBlock-Stable
BTW, I don't think this should be a release blocker, since we've presumably lived with it on Mac and Android for a number of releases. Feel free to re-add if you disagree.

Comment 7 by ericrk@chromium.org, Jun 16 2017

Note that even on more complex content that does hit the path rasterization code, MSAA breaks crispEdges. See The example here: https://codepen.io/ericrk/pen/eRgmMr

If you comment the "crispEdges" style in and out with MSAA supported, you should see no change (always smooth). If you do so with MSAA disabled, you see AA turn on and off.

Comment 8 by ericrk@chromium.org, Jun 16 2017

If this is not working across most of Skia (lines, paths), maybe we should just disable MSAA in CC in cases where any part of the doc requests no AA? Although it feels like Skia should support mixed AA/non-AA content when rendering to an MSAA backbuffer?
Yikes, that's not good. We could disable MSAA if there's any crispEdges content in the page, although we'd have to snoop the SkPicture, most likely, and toggling modes might be jarring.

Or maybe we can skip any path renderer that takes advantage of MSAA when AA is disabled, enforcing use of render in software and upload? That should fill all the samples with the same value, although I'm not 100% certain.
Skia does support rendering aliased to a MSAA buffer when GL supports it. However, OpenGL ES doesn't support disabling multisampling rasterization without GL_EXT_multisample_compatibility. We could expose this through the command buffer.
Actually, on Linux and Mac, I'm seeing aliasing on the tiger example, and MSAA is still enabled. If I comment out the crispEdges line, I see antialiasing again. So it looks like it's doing the right thing for concave paths. I think Skia is using glDisable(GL_MULTISAMPLE), which does the right thing even if the destination buffer is multisampled.

So maybe there's just something wonky with lines.
Or maybe ANGLE doesn't support glDisable(GL_MULTISAMPLE)?
AGreed - it seems possible that ANGLE/DX may not be respecting glDisable(GL_MULTISAMPLE)... Adding jmadill@ in case he has some ideas.

With --use-gl=desktop, the tiger example works fine (aliased with crisp edges).

With ANGLE/DX, we end up seeing this issue (both in the line and tiger examples).
Cc: senorblanco@chromium.org
Owner: jmad...@chromium.org
actually add jmadill@
So apparently ANGLE does not support the GL_EXT_multisample_compatibility extension on its D3D11 backend. However, it does support it on its GL backend, so the plumbing is there, and it should be fairly straightforward to implement (D3D11 has an equivalent state bit). However, one wrinkle is the SAMPLE_ALPHA_TO_ONE enum, for which Jamie wasn't sure if D3D had an equivalent.
Components: Internals>GPU>ANGLE
Cc: shannonwoods@chromium.org
Shannon, do you recall any details of our conversations about EXT_multisample_compatiblity on D3D11? I thought we talked about this with NVIDIA at some point - I don't know of a way to implement SAMPLE_ALPHA_TO_ONE trivially, without some shader rewriting. Maybe there's a blend state of some kind we could set to so the same operations, but my understanding of this feature and why it exists is a bit fuzzy. Otherwise ANGLE could expose a new extension that allows enabling/disabling multisample only - this is trivial in D3D11.
For the purposes of this bug we'd be ok having a narrower extension that just did GL_MULTISAMPLE.
I don't recall any discussions about EXT_multisample_compatibility-- I may not have been part of the conversations you're remembering.
Owner: geoffl...@chromium.org
I dug into this a bit today.  At first I thought the SAMPLE_ALPHA_TO_ONE would be the part that would be difficult.  The spec mentions how this could be emulated in D3D (maybe this is what Shannon contributed to the spec) and this would be possible in ANGLE.

Turns out that the problems is that multisampling cannot be disabled in D3D11 feature level 10.1 and higher.  From the docs:

"For feature levels 10.1 and higher, the setting of MultisampleEnable has no effect on points and triangles with regard to MSAA and impacts only the selection of the line-rendering algorithm..."

I made a test to confirm that this flag has no effect, the rendered results are always the same.

Going to speak with our contacts at Microsoft today and see if there is something I'm missing.
Cc: vmp...@chromium.org jbau...@chromium.org geoffl...@chromium.org
Owner: ----
Status: Available (was: Assigned)
It sounds like D3D11 Doesn't support disabling multisampling on a per-draw-call basis. Given this I think our best option is to update the "content is suitable for GPU rasterization" detection to always return true (not blacklist to MSAA) if any content requires crisp edges. This may require an additional bool (requires_crisp_edges?) and detection code.

The root of the logic is here: https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host.cc?rcl=b01dc5448d235367e67b6bc4c1e58aa54172fa97&l=736

Adding some additional people, in case anyone wants to take a look.
Owner: ericrk@chromium.org
Status: Started (was: Available)
Taking a look at this.
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52796123f44bf2407ba058e77159156f6da3824d

commit 52796123f44bf2407ba058e77159156f6da3824d
Author: Eric Karl <ericrk@chromium.org>
Date: Wed Jun 28 23:33:14 2017

Handle MSAA + Non-AA paths on Windows

ANGLE doesn't support disabling MSAA on a per-draw-op basis, so we need
to track whether *any* path needs MSAA disabled, and disable MSAA in
general.

To achieve this, this CL includes two changes:

Addition of HasNonAAPaint() to PaintOpBuffer - this allows us to
propogate the presece of any draw commands requesting non-AA raster
through to the layer tree host impl.

Renaming and inverting SuitableForGpuRasterization to HasSlowPaths.
This updates the naming to represent what's actually happening, as
we now always use GPU raster, even when "not suitable".

Bug:  733691 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
Change-Id: I3c2b6b55f3ecabcdc6be6312260c4bb9b2fc9ed6
Reviewed-on: https://chromium-review.googlesource.com/549236
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483193}
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/layers/layer.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/layers/layer.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/layers/picture_layer.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/layers/picture_layer.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/layers/picture_layer_impl_unittest.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/layers/picture_layer_unittest.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/layers/texture_layer_impl_unittest.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/paint/display_item_list.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/paint/paint_op_buffer.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/paint/paint_op_buffer.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/paint/paint_op_buffer_unittest.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/test/fake_content_layer_client.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/test/fake_content_layer_client.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/test/fake_picture_layer.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/test/fake_picture_layer.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/test/test_web_graphics_context_3d.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/trees/layer_tree_host.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/gpu/command_buffer/common/capabilities.h
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/52796123f44bf2407ba058e77159156f6da3824d/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)

Sign in to add a comment