Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1658623002:
Add gpu implementation of SkAvoidXfermode (Closed)
|
Created:
4 years, 10 months ago by robertphillips Modified:
4 years, 10 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd gpu implementation of SkAvoidXfermode
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1658623002
TBR=bsalomon@google.com
Committed: https://skia.googlesource.com/skia/+/15691a055db9b68c9b48f589e48d8a85888cf83f
Committed: https://skia.googlesource.com/skia/+/afb188de27d047c8327ccc7b099203e8fc2a4129
Patch Set 1 #Patch Set 2 : Add gpu implementation to SkAvoidXfermode #Patch Set 3 : update #Patch Set 4 : Finally working #Patch Set 5 : Added comments #
Total comments: 7
Patch Set 6 : Address code review comments #Patch Set 7 : Removed storage of unneeded data & added new GM #Patch Set 8 : update to ToT #Patch Set 9 : Fix up virtual call signature #Patch Set 10 : Update factory counts #
Total comments: 4
Patch Set 11 : Address code review comments & fix shader test #
Total comments: 2
Patch Set 12 : Fix variable name #Patch Set 13 : Address comments #
Messages
Total messages: 53 (26 generated)
Description was changed from ========== Add gpu implementation of SkAvoidXfermode ========== to ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
robertphillips@google.com changed reviewers: + egdaniel@google.com
https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... File src/effects/SkAvoidXfermode.cpp (right): https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... src/effects/SkAvoidXfermode.cpp:259: fragBuilder->codeAppendf("dist = dist * %s.a - (%s.a - 1.0);", kColorAndTolUni, kColorAndTolUni); 100 https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... src/effects/SkAvoidXfermode.cpp:409: add_avoid_code(fragBuilder, dstColor, kColorandTolUni, fMode); lets not have this guy introduce a variable dist and then use just know to use it below. It should be probably be passed in. https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... src/effects/SkAvoidXfermode.cpp:432: fMode = avoid.mode(); why setting fMode here? Do we actually need fMode? Can't we always get it off the GrXferProcessor?
https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... File src/effects/SkAvoidXfermode.cpp (right): https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... src/effects/SkAvoidXfermode.cpp:259: fragBuilder->codeAppendf("dist = dist * %s.a - (%s.a - 1.0);", kColorAndTolUni, kColorAndTolUni); On 2016/02/02 19:10:35, egdaniel wrote: > 100 Done. https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... src/effects/SkAvoidXfermode.cpp:409: add_avoid_code(fragBuilder, dstColor, kColorandTolUni, fMode); On 2016/02/02 19:10:35, egdaniel wrote: > lets not have this guy introduce a variable dist and then use just know to use > it below. It should be probably be passed in. Done. https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... src/effects/SkAvoidXfermode.cpp:432: fMode = avoid.mode(); On 2016/02/02 19:10:35, egdaniel wrote: > why setting fMode here? Do we actually need fMode? Can't we always get it off > the GrXferProcessor? We need to pass it in to the add_avoid_code method. It does seem odd that it is set in the ctor and also set here.
https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... File src/effects/SkAvoidXfermode.cpp (right): https://codereview.chromium.org/1658623002/diff/80001/src/effects/SkAvoidXfer... src/effects/SkAvoidXfermode.cpp:432: fMode = avoid.mode(); On 2016/02/02 19:10:35, egdaniel wrote: > why setting fMode here? Do we actually need fMode? Can't we always get it off > the GrXferProcessor? Done.
lgtm
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
robertphillips@google.com changed reviewers: + bsalomon@google.com
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/170001
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 robertphillips@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/1658623002/#ps170001 (title: "Update factory counts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
Description was changed from ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR=bsalomon@google.com ==========
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/170001
Message was sent while issue was closed.
Description was changed from ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR=bsalomon@google.com ========== to ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR=bsalomon@google.com Committed: https://skia.googlesource.com/skia/+/15691a055db9b68c9b48f589e48d8a85888cf83f ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001) as https://skia.googlesource.com/skia/+/15691a055db9b68c9b48f589e48d8a85888cf83f
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:170001) has been created in https://codereview.chromium.org/1663093002/ by mtklein@google.com. The reason for reverting is: shader compilation failures error C1008: undefined variable "null" https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-S....
Message was sent while issue was closed.
https://codereview.chromium.org/1658623002/diff/170001/src/effects/SkAvoidXfe... File src/effects/SkAvoidXfermode.cpp (right): https://codereview.chromium.org/1658623002/diff/170001/src/effects/SkAvoidXfe... src/effects/SkAvoidXfermode.cpp:285: fragBuilder->codeAppendf("%s = newCoverage * %s + (vec4(1.0)-newCoverage) * %s;", Not sure how fOutputColor is null, but I just noticed shouldn't this have the if (dist < 1/255) check? https://codereview.chromium.org/1658623002/diff/170001/src/effects/SkAvoidXfe... src/effects/SkAvoidXfermode.cpp:419: fragBuilder->codeAppendf("if (%s < 0.0039) { %s = %s; } else {", just giving a second pass...for minimal perf improvement, put the top part of this if before we make newCoverage above?
Message was sent while issue was closed.
Description was changed from ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR=bsalomon@google.com Committed: https://skia.googlesource.com/skia/+/15691a055db9b68c9b48f589e48d8a85888cf83f ========== to ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR=bsalomon@google.com Committed: https://skia.googlesource.com/skia/+/15691a055db9b68c9b48f589e48d8a85888cf83f ==========
https://codereview.chromium.org/1658623002/diff/170001/src/effects/SkAvoidXfe... File src/effects/SkAvoidXfermode.cpp (right): https://codereview.chromium.org/1658623002/diff/170001/src/effects/SkAvoidXfe... src/effects/SkAvoidXfermode.cpp:285: fragBuilder->codeAppendf("%s = newCoverage * %s + (vec4(1.0)-newCoverage) * %s;", On 2016/02/03 15:09:57, egdaniel wrote: > Not sure how fOutputColor is null, but I just noticed shouldn't this have the if > (dist < 1/255) check? Done. https://codereview.chromium.org/1658623002/diff/170001/src/effects/SkAvoidXfe... src/effects/SkAvoidXfermode.cpp:419: fragBuilder->codeAppendf("if (%s < 0.0039) { %s = %s; } else {", I consolidated the computation of "newCoverage" in add_avoid_code and now use "newCoverage" in this test (so it can't be moved earlier).
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...)
lgtm with cleanup of parameters in helper function https://codereview.chromium.org/1658623002/diff/190001/src/effects/SkAvoidXfe... File src/effects/SkAvoidXfermode.cpp (right): https://codereview.chromium.org/1658623002/diff/190001/src/effects/SkAvoidXfe... src/effects/SkAvoidXfermode.cpp:247: const char* srcColor, why are src and output color neededed?
https://codereview.chromium.org/1658623002/diff/190001/src/effects/SkAvoidXfe... File src/effects/SkAvoidXfermode.cpp (right): https://codereview.chromium.org/1658623002/diff/190001/src/effects/SkAvoidXfe... src/effects/SkAvoidXfermode.cpp:247: const char* srcColor, On 2016/02/03 16:41:24, egdaniel wrote: > why are src and output color neededed? Done.
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/230001
The CQ bit was unchecked by robertphillips@google.com
The CQ bit was checked by robertphillips@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/1658623002/#ps230001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658623002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658623002/230001
Message was sent while issue was closed.
Description was changed from ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR=bsalomon@google.com Committed: https://skia.googlesource.com/skia/+/15691a055db9b68c9b48f589e48d8a85888cf83f ========== to ========== Add gpu implementation of SkAvoidXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR=bsalomon@google.com Committed: https://skia.googlesource.com/skia/+/15691a055db9b68c9b48f589e48d8a85888cf83f Committed: https://skia.googlesource.com/skia/+/afb188de27d047c8327ccc7b099203e8fc2a4129 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001) as https://skia.googlesource.com/skia/+/afb188de27d047c8327ccc7b099203e8fc2a4129 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
