Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2116823002:
Increase batching for AA fill rects. (Closed)
|
Created:
4 years, 5 months ago by bsalomon Modified:
4 years, 5 months ago Reviewers:
robertphillips CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@rectgeoms3 Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionIncrease batching for AA fill rects.
This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes.
It also allows batching of rects with a local matrix with rects without a local matrix.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2116823002
Committed: https://skia.googlesource.com/skia/+/e525ecaf63f225f1da6e9834f7a291c06ad44d23
Committed: https://skia.googlesource.com/skia/+/4be9a30aed390bd37681242252fe29d7839bad19
Patch Set 1 #Patch Set 2 : inline gp creation #Patch Set 3 : wrap line #
Total comments: 6
Patch Set 4 : Address comments, rename batch class, use prealloc storage #Patch Set 5 : rebase for reland #Messages
Total messages: 22 (11 generated)
Description was changed from ========== Increase batching for AA fill rects. This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes. It also allows batching of rects with a local matrix with rects without a local matrix. ========== to ========== Increase batching for AA fill rects. This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes. It also allows batching of rects with a local matrix with rects without a local matrix. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2116823002 ==========
bsalomon@google.com changed reviewers: + robertphillips@google.com
Not sure if this interferes with what you're working on. If so, I can park it. I just couldn't help but notice how close these two batches were to each other. Also, currently when there is no local matrix we don't batch when the view matrix changes and there the local coords are needed, which seemed pretty bad.
lgtm + nits I think we should go for it. I'm only handling nonAA fill rects right now. https://codereview.chromium.org/2116823002/diff/40001/src/gpu/batches/GrAAFil... File src/gpu/batches/GrAAFillRectBatch.cpp (right): https://codereview.chromium.org/2116823002/diff/40001/src/gpu/batches/GrAAFil... src/gpu/batches/GrAAFillRectBatch.cpp:293: : RectInfo(color, viewMatrix, rect, devRect, HasLocalMatrix::kNo) {} Can this just return bool and hide the 'HasLocalMatrix' enum ? https://codereview.chromium.org/2116823002/diff/40001/src/gpu/batches/GrAAFil... src/gpu/batches/GrAAFillRectBatch.cpp:309: rm ' ' ? https://codereview.chromium.org/2116823002/diff/40001/src/gpu/batches/GrAAFil... src/gpu/batches/GrAAFillRectBatch.cpp:319: RectWithLocalMatrixInfo(GrColor color, const SkMatrix& viewMatrix, const SkRect& rect, add a ' ' ?
Addressed the comments, renamed the batch to just plain 'AAFillRectBatch', and switched the storage to SkSTArray so that the first few rects won't cause a malloc. https://codereview.chromium.org/2116823002/diff/40001/src/gpu/batches/GrAAFil... File src/gpu/batches/GrAAFillRectBatch.cpp (right): https://codereview.chromium.org/2116823002/diff/40001/src/gpu/batches/GrAAFil... src/gpu/batches/GrAAFillRectBatch.cpp:293: : RectInfo(color, viewMatrix, rect, devRect, HasLocalMatrix::kNo) {} On 2016/07/01 16:04:59, robertphillips wrote: > Can this just return bool and hide the 'HasLocalMatrix' enum ? Done. https://codereview.chromium.org/2116823002/diff/40001/src/gpu/batches/GrAAFil... src/gpu/batches/GrAAFillRectBatch.cpp:309: On 2016/07/01 16:04:59, robertphillips wrote: > rm ' ' ? Done. https://codereview.chromium.org/2116823002/diff/40001/src/gpu/batches/GrAAFil... src/gpu/batches/GrAAFillRectBatch.cpp:319: RectWithLocalMatrixInfo(GrColor color, const SkMatrix& viewMatrix, const SkRect& rect, On 2016/07/01 16:04:59, robertphillips wrote: > add a ' ' ? Done.
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/v2/patch-status/codereview.chromium.or...
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 bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2116823002/#ps60001 (title: "Address comments, rename batch class, use prealloc storage")
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 ========== Increase batching for AA fill rects. This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes. It also allows batching of rects with a local matrix with rects without a local matrix. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2116823002 ========== to ========== Increase batching for AA fill rects. This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes. It also allows batching of rects with a local matrix with rects without a local matrix. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2116823002 Committed: https://skia.googlesource.com/skia/+/e525ecaf63f225f1da6e9834f7a291c06ad44d23 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/e525ecaf63f225f1da6e9834f7a291c06ad44d23
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2124603002/ by benjaminwagner@google.com. The reason for reverting is: I believe this is causing the Chromium DEPS roll to fail due to linux_blink_rel. Reverting for now to get the roll going again..
Message was sent while issue was closed.
Description was changed from ========== Increase batching for AA fill rects. This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes. It also allows batching of rects with a local matrix with rects without a local matrix. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2116823002 Committed: https://skia.googlesource.com/skia/+/e525ecaf63f225f1da6e9834f7a291c06ad44d23 ========== to ========== Increase batching for AA fill rects. This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes. It also allows batching of rects with a local matrix with rects without a local matrix. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2116823002 Committed: https://skia.googlesource.com/skia/+/e525ecaf63f225f1da6e9834f7a291c06ad44d23 ==========
On 2016/07/04 17:41:40, Ben Wagner wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/2124603002/ by mailto:benjaminwagner@google.com. > > The reason for reverting is: I believe this is causing the Chromium DEPS roll to > fail due to linux_blink_rel. Reverting for now to get the roll going again.. The two layout tests change minutely on about 3 or 4 pixels by slight amounts. They are now marked for rebaseline here: https://codereview.chromium.org/2125763002/. Relanding this
The CQ bit was checked by bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2116823002/#ps80001 (title: "rebase for reland")
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 ========== Increase batching for AA fill rects. This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes. It also allows batching of rects with a local matrix with rects without a local matrix. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2116823002 Committed: https://skia.googlesource.com/skia/+/e525ecaf63f225f1da6e9834f7a291c06ad44d23 ========== to ========== Increase batching for AA fill rects. This allows batching of rects provided without a local matrix when local coords are required and when the view matrix changes. It also allows batching of rects with a local matrix with rects without a local matrix. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2116823002 Committed: https://skia.googlesource.com/skia/+/e525ecaf63f225f1da6e9834f7a291c06ad44d23 Committed: https://skia.googlesource.com/skia/+/4be9a30aed390bd37681242252fe29d7839bad19 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/4be9a30aed390bd37681242252fe29d7839bad19 |
