Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1764923003:
Circle GP/batch housecleaning (Closed)
DescriptionCircle GP/batch housecleaning
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1764923003
Committed: https://skia.googlesource.com/skia/+/cdaa97bf664e0d584187efc125bfff670a064a9a
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : Apply same changes to ellipse gp/batches #Patch Set 4 : more cleanup #Patch Set 5 : more cleanup #Patch Set 6 : fix indent #
Total comments: 3
Patch Set 7 : remove diellipse gp cons #Messages
Total messages: 19 (8 generated)
Description was changed from ========== Circle GP/batch housecleaning ========== to ========== Circle GP/batch housecleaning GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
bsalomon@google.com changed reviewers: + brianosman@google.com, egdaniel@google.com, joshualitt@chromium.org
I was starting to write a new batch using CircleBatch as a model and realized it could use some tidying (as most of our batch classes probably could). More and more I'm leaning toward removing corner case optimizations and make the system simpler and make it easier to write batch classes or refactor existing ones. So looking at the xp-to-batch overrides: Is readsCoverage() worth exposing at all? I can't imagine it is ever true when there really is coverage. How about readsColor()? Color changes don't really block batching anymore (excluding batches that we just haven't updated yet to use attr color). There is the rarely used clear porter-duff mode (and maybe other similar modes) which can't really be worth optimizing for. There is also the coverage set op xp, but why would we vary the color anyway when using that? Also, there probably isn't much batching to be had when using that xp anyway. canTweakAlphaForCoverage() - I understand the benefit of this for the XP, but is it worth the complexity to avoid adding separate attributes in the verts? Maybe worth it since this is the common case (src-over). willColorBlendWithDst() - used to allow stencil/cover/stencil/cover into stencil*2/cover*2. Maybe worth keeping.
The CQ bit was checked by bsalomon@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/1764923003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1764923003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1764923003/diff/100001/src/gpu/GrOvalRenderer... File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/1764923003/diff/100001/src/gpu/GrOvalRenderer... src/gpu/GrOvalRenderer.cpp:358: static GrGeometryProcessor* Create(const SkMatrix& viewMatrix, DIEllipseStyle style) { Any reason to keep the factory method here (vs. Circle & Ellipse where you just made the constructor public)?
https://codereview.chromium.org/1764923003/diff/100001/src/gpu/GrOvalRenderer... File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/1764923003/diff/100001/src/gpu/GrOvalRenderer... src/gpu/GrOvalRenderer.cpp:358: static GrGeometryProcessor* Create(const SkMatrix& viewMatrix, DIEllipseStyle style) { On 2016/03/07 14:45:45, Brian Osman wrote: > Any reason to keep the factory method here (vs. Circle & Ellipse where you just > made the constructor public)? Nope, just bleariness. Will change.
https://codereview.chromium.org/1764923003/diff/100001/src/gpu/GrOvalRenderer... File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/1764923003/diff/100001/src/gpu/GrOvalRenderer... src/gpu/GrOvalRenderer.cpp:358: static GrGeometryProcessor* Create(const SkMatrix& viewMatrix, DIEllipseStyle style) { On 2016/03/07 14:57:39, bsalomon wrote: > On 2016/03/07 14:45:45, Brian Osman wrote: > > Any reason to keep the factory method here (vs. Circle & Ellipse where you > just > > made the constructor public)? > > Nope, just bleariness. Will change. Done, brings it to an even 200 net LOC reduction.
On 2016/03/07 15:53:50, bsalomon wrote: > https://codereview.chromium.org/1764923003/diff/100001/src/gpu/GrOvalRenderer... > File src/gpu/GrOvalRenderer.cpp (right): > > https://codereview.chromium.org/1764923003/diff/100001/src/gpu/GrOvalRenderer... > src/gpu/GrOvalRenderer.cpp:358: static GrGeometryProcessor* Create(const > SkMatrix& viewMatrix, DIEllipseStyle style) { > On 2016/03/07 14:57:39, bsalomon wrote: > > On 2016/03/07 14:45:45, Brian Osman wrote: > > > Any reason to keep the factory method here (vs. Circle & Ellipse where you > > just > > > made the constructor public)? > > > > Nope, just bleariness. Will change. > > Done, brings it to an even 200 net LOC reduction. lgtm
The CQ bit was checked by bsalomon@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1764923003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1764923003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by bsalomon@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1764923003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1764923003/120001
Message was sent while issue was closed.
Description was changed from ========== Circle GP/batch housecleaning GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Circle GP/batch housecleaning GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/cdaa97bf664e0d584187efc125bfff670a064a9a ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/cdaa97bf664e0d584187efc125bfff670a064a9a |
