Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2183223002:
Implement coverage AA for skewed rects with local coords (Closed)
|
Created:
4 years, 4 months ago by csmartdalton Modified:
4 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionImplement coverage AA for skewed rects with local coords
Adds a path fallback for rects with local coords that can't be drawn
with an analytic shader. This is accomplished by modifying the view
matrix and then drawing the local rect/quad.
BUG=skia:5500, 7508
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2183223002
Committed: https://skia.googlesource.com/skia/+/fc49d56feb2d890ccb3827ed087ab32e18a9da12
Patch Set 1 #
Total comments: 4
Patch Set 2 : Implement coverage AA for skewed rects with local coords #Patch Set 3 : Implement coverage AA for skewed rects with local coords #Messages
Total messages: 24 (10 generated)
Description was changed from ========== Implement coverage AA for skewed rects with local coords Adds a path fallback for rects with local coords that can't be drawn with an analytic shader. This is accomplished by modifying the view matrix and then drawing the local rect/quad. BUG=skia: ========== to ========== Implement coverage AA for skewed rects with local coords Adds a path fallback for rects with local coords that can't be drawn with an analytic shader. This is accomplished by modifying the view matrix and then drawing the local rect/quad. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2183223002 ==========
csmartdalton@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com, xidachen@chromium.org
https://codereview.chromium.org/2183223002/diff/1/src/gpu/GrDrawContext.cpp File src/gpu/GrDrawContext.cpp (right): https://codereview.chromium.org/2183223002/diff/1/src/gpu/GrDrawContext.cpp#n... src/gpu/GrDrawContext.cpp:650: SkFAIL("Bogus local rect."); // I'm hoping this is illegal and we can just bail? Is an empty local rect legal? https://codereview.chromium.org/2183223002/diff/1/src/gpu/GrDrawContext.cpp#n... src/gpu/GrDrawContext.cpp:706: SkFAIL("Bogus local matrix."); // I'm hoping this is illegal and we can just bail? Is a degenerate local matrix legal?
https://codereview.chromium.org/2183223002/diff/1/src/gpu/GrDrawContext.cpp File src/gpu/GrDrawContext.cpp (right): https://codereview.chromium.org/2183223002/diff/1/src/gpu/GrDrawContext.cpp#n... src/gpu/GrDrawContext.cpp:650: SkFAIL("Bogus local rect."); // I'm hoping this is illegal and we can just bail? On 2016/07/26 20:28:28, csmartdalton wrote: > Is an empty local rect legal? I don't think so. Seems ok to bail to me. https://codereview.chromium.org/2183223002/diff/1/src/gpu/GrDrawContext.cpp#n... src/gpu/GrDrawContext.cpp:706: SkFAIL("Bogus local matrix."); // I'm hoping this is illegal and we can just bail? On 2016/07/26 20:28:28, csmartdalton wrote: > Is a degenerate local matrix legal? I think it's ok to bail.
xidachen@chromium.org changed reviewers: + junov@chromium.org
I can confirm that this patch does fix the issue when rotation + scale is applied to the SkCanvas. Are there any perf tests related to this? Would this cause any performance regression to SkCanvas::drawImageRect()?
On 2016/07/26 20:38:45, xidachen wrote: > I can confirm that this patch does fix the issue when rotation + scale is > applied to the SkCanvas. Are there any perf tests related to this? Would this > cause any performance regression to SkCanvas::drawImageRect()? Correctness before performance.
On 2016/07/26 20:42:26, Justin Novosad wrote: > On 2016/07/26 20:38:45, xidachen wrote: > > I can confirm that this patch does fix the issue when rotation + scale is > > applied to the SkCanvas. Are there any perf tests related to this? Would this > > cause any performance regression to SkCanvas::drawImageRect()? > > Correctness before performance. Yeah, this can cause a regression, but only in skewed-rect cases that used to be incorrect. Normal rects will be unaffected.
lgtm
On 2016/07/26 20:46:17, xidachen wrote: > lgtm lgtm 2
With a passing test lgtm
Also, I think it'd be safe to print and return rather than SkFAIL() in the degenerate cases.
On 2016/07/26 20:43:47, csmartdalton wrote: > On 2016/07/26 20:42:26, Justin Novosad wrote: > > On 2016/07/26 20:38:45, xidachen wrote: > > > I can confirm that this patch does fix the issue when rotation + scale is > > > applied to the SkCanvas. Are there any perf tests related to this? Would > this > > > cause any performance regression to SkCanvas::drawImageRect()? > > > > Correctness before performance. > > Yeah, this can cause a regression, but only in skewed-rect cases that used to be > incorrect. Normal rects will be unaffected. Could you please update the bug number before committing, skia:5500, 7508
Description was changed from ========== Implement coverage AA for skewed rects with local coords Adds a path fallback for rects with local coords that can't be drawn with an analytic shader. This is accomplished by modifying the view matrix and then drawing the local rect/quad. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2183223002 ========== to ========== Implement coverage AA for skewed rects with local coords Adds a path fallback for rects with local coords that can't be drawn with an analytic shader. This is accomplished by modifying the view matrix and then drawing the local rect/quad. BUG=skia:5500, 7508 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2183223002 ==========
Ok, I think this covers all the feedback. I verified that these cases both have test coverage already in gm. I'll kick off a couple trybots and verify they're all passing.
lgtm
The CQ bit was checked by csmartdalton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xidachen@chromium.org, junov@chromium.org Link to the patchset: https://codereview.chromium.org/2183223002/#ps20001 (title: "Implement coverage AA for skewed rects with local coords")
The CQ bit was unchecked by csmartdalton@google.com
The CQ bit was checked by csmartdalton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xidachen@chromium.org, bsalomon@google.com, junov@chromium.org Link to the patchset: https://codereview.chromium.org/2183223002/#ps40001 (title: "Implement coverage AA for skewed rects with local coords")
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.
Description was changed from ========== Implement coverage AA for skewed rects with local coords Adds a path fallback for rects with local coords that can't be drawn with an analytic shader. This is accomplished by modifying the view matrix and then drawing the local rect/quad. BUG=skia:5500, 7508 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2183223002 ========== to ========== Implement coverage AA for skewed rects with local coords Adds a path fallback for rects with local coords that can't be drawn with an analytic shader. This is accomplished by modifying the view matrix and then drawing the local rect/quad. BUG=skia:5500, 7508 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2183223002 Committed: https://skia.googlesource.com/skia/+/fc49d56feb2d890ccb3827ed087ab32e18a9da12 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/fc49d56feb2d890ccb3827ed087ab32e18a9da12 |
