Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2046483003:
Fix dashing bug where hwaa was unintentionally disabled (Closed)
|
Created:
4 years, 6 months ago by csmartdalton Modified:
4 years, 6 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionFix dashing bug where hwaa was unintentionally disabled
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003
Committed: https://skia.googlesource.com/skia/+/ddc2cd6a1f55f4f235db60d7545a74620722ba25
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : simplify #Messages
Total messages: 21 (8 generated)
Description was changed from ========== Fix non-aa dashing on MSAA render targets BUG=skia: ========== to ========== Fix non-aa dashing on MSAA render targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 ==========
csmartdalton@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com
Just a simple prereq for https://codereview.chromium.org/2041283002
bsalomon@google.com changed reviewers: + egdaniel@google.com
There is some history here where I think we are enabling dashing coverage based AA when on MSAA as some special exceptional case. +Greg.
On 2016/06/07 13:25:30, bsalomon wrote: > There is some history here where I think we are enabling dashing coverage based > AA when on MSAA as some special exceptional case. +Greg. So we actually intentionally made this dashing effect force the use of MSAA regardless of if the paint requested it or not. So in general this effect draws the entire dashed line as a single rect geometry and analytically computes the coverage between the dashes and the edges. So for MSAA where we want AA we have a special mode where we can only compute coverage on the interior and let the hardware handle the outer edges. The problem with nonAA is that some GPUs will say they support disabling MSAA but actually don't. If we just drew the dash using the nonAA version, we'd end up with AA on the outside edges and nonAA on the inside edges in cases we can't disable MSAA correctly. In the end we decided it was a better trade off to have everything always AA when in MSAA for dashed lines (really who makes msaa buffers but wants nonAA draws....) then to occasionally get stuck in this mixed AA state.
On 2016/06/07 13:25:30, bsalomon wrote: > There is some history here where I think we are enabling dashing coverage based > AA when on MSAA as some special exceptional case. +Greg. In this particular case, the dash handler was always using MSAA on unified multisampled RTs, even if the paint was non-aa. What the change does is makes sure the dash gets rendered as "bw" when the paint is non-aa. If I understand the exceptional case you're describing, then I think it it and this change aren't related? The code here couldn't have used coverage AA on a unified multisampled target because it was enabling MSAA 100% of the time it encountered that kind of RT.
On 2016/06/07 14:39:53, csmartdalton wrote: > On 2016/06/07 13:25:30, bsalomon wrote: > > There is some history here where I think we are enabling dashing coverage > based > > AA when on MSAA as some special exceptional case. +Greg. > > In this particular case, the dash handler was always using MSAA on unified > multisampled RTs, even if the paint was non-aa. What the change does is makes > sure the dash gets rendered as "bw" when the paint is non-aa. > > If I understand the exceptional case you're describing, then I think it it and > this change aren't related? The code here couldn't have used coverage AA on a > unified multisampled target because it was enabling MSAA 100% of the time it > encountered that kind of RT. Even when we enabled MSAA we still are doing coverage AA between the dashes since those are not on the edges of the geometry. So in your change you want to draw to an MSAA buffer, disable the HWAntiAliasing, and use the bw version of the shader. In theory this works nicely. However, if the GPU doesn't actually disable HWAntiAliasing even if you request it (which does happen), then we will end up using the bw shader to get nonAA interior edges between dashes, but the outer edges will all get HWAntiAliased. This puts us into a weird state of the dashes being half AA and half nonAA. This is why we intentially always used MSAA on unified multisampled RTs and ignored the paints AA request.
On 2016/06/07 14:44:59, egdaniel wrote: > On 2016/06/07 14:39:53, csmartdalton wrote: > > On 2016/06/07 13:25:30, bsalomon wrote: > > > There is some history here where I think we are enabling dashing coverage > > based > > > AA when on MSAA as some special exceptional case. +Greg. > > > > In this particular case, the dash handler was always using MSAA on unified > > multisampled RTs, even if the paint was non-aa. What the change does is makes > > sure the dash gets rendered as "bw" when the paint is non-aa. > > > > If I understand the exceptional case you're describing, then I think it it and > > this change aren't related? The code here couldn't have used coverage AA on a > > unified multisampled target because it was enabling MSAA 100% of the time it > > encountered that kind of RT. > > Even when we enabled MSAA we still are doing coverage AA between the dashes > since those are not on the edges of the geometry. So in your change you want to > draw to an MSAA buffer, disable the HWAntiAliasing, and use the bw version of > the shader. In theory this works nicely. However, if the GPU doesn't actually > disable HWAntiAliasing even if you request it (which does happen), then we will > end up using the bw shader to get nonAA interior edges between dashes, but the > outer edges will all get HWAntiAliased. This puts us into a weird state of the > dashes being half AA and half nonAA. This is why we intentially always used MSAA > on unified multisampled RTs and ignored the paints AA request. Ok I see. There is a cap for multisampleDisableSupport now, so we ought to be able to selectively do this WAR on platforms we know will have a problem. I'll try and update the change.
On 2016/06/07 14:49:38, csmartdalton wrote: > On 2016/06/07 14:44:59, egdaniel wrote: > > On 2016/06/07 14:39:53, csmartdalton wrote: > > > On 2016/06/07 13:25:30, bsalomon wrote: > > > > There is some history here where I think we are enabling dashing coverage > > > based > > > > AA when on MSAA as some special exceptional case. +Greg. > > > > > > In this particular case, the dash handler was always using MSAA on unified > > > multisampled RTs, even if the paint was non-aa. What the change does is > makes > > > sure the dash gets rendered as "bw" when the paint is non-aa. > > > > > > If I understand the exceptional case you're describing, then I think it it > and > > > this change aren't related? The code here couldn't have used coverage AA on > a > > > unified multisampled target because it was enabling MSAA 100% of the time it > > > encountered that kind of RT. > > > > Even when we enabled MSAA we still are doing coverage AA between the dashes > > since those are not on the edges of the geometry. So in your change you want > to > > draw to an MSAA buffer, disable the HWAntiAliasing, and use the bw version of > > the shader. In theory this works nicely. However, if the GPU doesn't actually > > disable HWAntiAliasing even if you request it (which does happen), then we > will > > end up using the bw shader to get nonAA interior edges between dashes, but the > > outer edges will all get HWAntiAliased. This puts us into a weird state of the > > dashes being half AA and half nonAA. This is why we intentially always used > MSAA > > on unified multisampled RTs and ignored the paints AA request. > > Ok I see. There is a cap for multisampleDisableSupport now, so we ought to be > able to selectively do this WAR on platforms we know will have a problem. I'll > try and update the change. Hold off just a sec... I have a CL coming up for review that should make this a bit clearer.
Here's a proposed fix for platforms that can disable msaa. It also fixes the problem where hwaa wasn't being enabled as expected.
Description was changed from ========== Fix non-aa dashing on MSAA render targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 ========== to ========== Don't always force msaa dashing Skips the "always enable msaa" workaround for dashing on platforms that can actually disable multisampling. Also fixes a subtle bug where hwaa wasn't always being set as expected. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 ==========
https://codereview.chromium.org/2046483003/diff/20001/include/gpu/GrDrawConte... File include/gpu/GrDrawContext.h (right): https://codereview.chromium.org/2046483003/diff/20001/include/gpu/GrDrawConte... include/gpu/GrDrawContext.h:268: const GrCaps* caps() const { return fContext->caps(); } IIRC, Rob is fairly adamant that we don't expose caps through here, that we should see drawContext as complete drawing API that doesn't require the client to make decisions about how to implement drawing. I pretty much agree with that (and think isUnifiedMultisampled() and hasMixedSamples() should be removed for similar reasons). However, the path renderers are sort of violating the layering. They're effectively implementing some of the GrDC's draw methods, yet they do it by going back through the public GrDC API (drawBatch). The long term plan here is to not install pipelines in batches before they're recorded, allowing multipass PRs to produce a single GrBatch for their passes. Then, PRs become batch factories and don't go through the GrDC interface (or even see a GrDC), but rather are just invoked by GrDC to produce a batch that GrDC enqueues. Shorter term, we could pass GrCaps in the args structs that GrPR see. I don't want to ask you to do that work now, though, just to unblock the work you're trying to do around instancing. So, alternatively, we could just file a bug about this (not using the disable msaa cap to avoid forcing aa for dashed lines) and fix it later. I'm going to be messing around with the interface between GrDC and GrPR and can tackle this over the next few weeks.
Description was changed from ========== Don't always force msaa dashing Skips the "always enable msaa" workaround for dashing on platforms that can actually disable multisampling. Also fixes a subtle bug where hwaa wasn't always being set as expected. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 ========== to ========== Fix dashing bug where hwaa was unintentionally disabled' BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 ==========
On 2016/06/07 19:32:09, bsalomon wrote: > https://codereview.chromium.org/2046483003/diff/20001/include/gpu/GrDrawConte... > File include/gpu/GrDrawContext.h (right): > > https://codereview.chromium.org/2046483003/diff/20001/include/gpu/GrDrawConte... > include/gpu/GrDrawContext.h:268: const GrCaps* caps() const { return > fContext->caps(); } > IIRC, Rob is fairly adamant that we don't expose caps through here, that we > should see drawContext as complete drawing API that doesn't require the client > to make decisions about how to implement drawing. I pretty much agree with that > (and think isUnifiedMultisampled() and hasMixedSamples() should be removed for > similar reasons). > > However, the path renderers are sort of violating the layering. They're > effectively implementing some of the GrDC's draw methods, yet they do it by > going back through the public GrDC API (drawBatch). The long term plan here is > to not install pipelines in batches before they're recorded, allowing multipass > PRs to produce a single GrBatch for their passes. Then, PRs become batch > factories and don't go through the GrDC interface (or even see a GrDC), but > rather are just invoked by GrDC to produce a batch that GrDC enqueues. > > Shorter term, we could pass GrCaps in the args structs that GrPR see. I don't > want to ask you to do that work now, though, just to unblock the work you're > trying to do around instancing. So, alternatively, we could just file a bug > about this (not using the disable msaa cap to avoid forcing aa for dashed lines) > and fix it later. I'm going to be messing around with the interface between GrDC > and GrPR and can tackle this over the next few weeks. sgtm. I simplified it a lot. Now the sole purpose of this change is so https://codereview.chromium.org/2041283002 can be a pure refactor.
Description was changed from ========== Fix dashing bug where hwaa was unintentionally disabled' BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 ========== to ========== Fix dashing bug where hwaa was unintentionally disabled BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 ==========
lgtm
The CQ bit was checked by csmartdalton@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2046483003/40001
Message was sent while issue was closed.
Description was changed from ========== Fix dashing bug where hwaa was unintentionally disabled BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 ========== to ========== Fix dashing bug where hwaa was unintentionally disabled BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2046483003 Committed: https://skia.googlesource.com/skia/+/ddc2cd6a1f55f4f235db60d7545a74620722ba25 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/ddc2cd6a1f55f4f235db60d7545a74620722ba25 |
