Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1776003002:
GrTessellator: abstract vertex allocation into caller (Closed)
|
Created:
4 years, 9 months ago by Stephen White Modified:
4 years, 9 months ago Reviewers:
bsalomon CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionGrTessellator: abstract vertex allocation into caller.
This abstracts all vertex allocation out of GrTessellator via a VertexBuffer interface. This removes all GPU-related calls from GrTessellator.
It also factors vertex drawing into GrTessellatingPathRenderer::drawVertices(), and makes tessellate() (now draw() also responsible for drawing. This means the cache hit case is clearer as an early-out,
and storing into cache is done in draw() as well.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1776003002
Committed: https://skia.googlesource.com/skia/+/6599efffeef3168dfc68dca99c30454c5c23b859
Patch Set 1 #Patch Set 2 : Remove #includes, tweak whitespace #Patch Set 3 : Refactor drawing, cache check #Patch Set 4 : Cleanup #Patch Set 5 : Fix Mac build #Patch Set 6 : VertexAllocator -> VertexBuffer, fix #includes, namespace #
Total comments: 2
Patch Set 7 : VertexBuffer -> VertexAllocator #
Messages
Total messages: 31 (16 generated)
Description was changed from ========== GrTessellator: refactor vertex buffer allocation into a Client class. BUG=skia: ========== to ========== GrTessellator: refactor vertex buffer allocation into a Client class. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776003002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776003002/60001
Description was changed from ========== GrTessellator: refactor vertex buffer allocation into a Client class. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== GrTessellator: refactor vertex buffer allocation into a Client class. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
senorblanco@chromium.org changed reviewers: + bsalomon@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
Description was changed from ========== GrTessellator: refactor vertex buffer allocation into a Client class. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== GrTessellator: abstract vertex allocation into caller. This abstracts all vertex allocation out of GrTessellator via a VertexAllocator interface. This removes all GPU-related calls from GrTessellator. It also factors vertex drawing into GrTessellatingPathRenderer::drawVertices(), and makes tessellate() also responsible for drawing (now draw()). This means the cache hit case is clearer as an early-out, and storing into cache is done in draw() as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776003002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776003002/80001
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 senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776003002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776003002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Brian: PTAL. Thanks!
Description was changed from ========== GrTessellator: abstract vertex allocation into caller. This abstracts all vertex allocation out of GrTessellator via a VertexAllocator interface. This removes all GPU-related calls from GrTessellator. It also factors vertex drawing into GrTessellatingPathRenderer::drawVertices(), and makes tessellate() also responsible for drawing (now draw()). This means the cache hit case is clearer as an early-out, and storing into cache is done in draw() as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== GrTessellator: abstract vertex allocation into caller. This abstracts all vertex allocation out of GrTessellator via a VertexBuffer interface. This removes all GPU-related calls from GrTessellator. It also factors vertex drawing into GrTessellatingPathRenderer::drawVertices(), and makes tessellate() (now draw() also responsible for drawing. This means the cache hit case is clearer as an early-out, and storing into cache is done in draw() as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Seems like a nice direction to pull the vertex buffer code out of the tessellator. https://codereview.chromium.org/1776003002/diff/100001/src/gpu/GrTessellator.h File src/gpu/GrTessellator.h (right): https://codereview.chromium.org/1776003002/diff/100001/src/gpu/GrTessellator.... src/gpu/GrTessellator.h:24: class VertexBuffer { Reading that code below it isn't always totally obvious to me whether a vertex buffer is a GrTessallator::VertexBuffer or a GrVertexBuffer. Can we use a different name, e.g. VertexAllocator or VertexProvider?
On 2016/03/10 15:46:24, bsalomon wrote: > Seems like a nice direction to pull the vertex buffer code out of the > tessellator. > > https://codereview.chromium.org/1776003002/diff/100001/src/gpu/GrTessellator.h > File src/gpu/GrTessellator.h (right): > > https://codereview.chromium.org/1776003002/diff/100001/src/gpu/GrTessellator.... > src/gpu/GrTessellator.h:24: class VertexBuffer { > Reading that code below it isn't always totally obvious to me whether a vertex > buffer is a GrTessallator::VertexBuffer or a GrVertexBuffer. Can we use a > different name, e.g. VertexAllocator or VertexProvider? Hah.. yeah. It was VertexAllocator in patch set 5. I'll go back to that.
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776003002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776003002/140001
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/1776003002/diff/100001/src/gpu/GrTessellator.h File src/gpu/GrTessellator.h (right): https://codereview.chromium.org/1776003002/diff/100001/src/gpu/GrTessellator.... src/gpu/GrTessellator.h:24: class VertexBuffer { On 2016/03/10 15:46:24, bsalomon wrote: > Reading that code below it isn't always totally obvious to me whether a vertex > buffer is a GrTessallator::VertexBuffer or a GrVertexBuffer. Can we use a > different name, e.g. VertexAllocator or VertexProvider? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/10 16:15:19, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. lgtm
The CQ bit was checked by senorblanco@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776003002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776003002/140001
Message was sent while issue was closed.
Description was changed from ========== GrTessellator: abstract vertex allocation into caller. This abstracts all vertex allocation out of GrTessellator via a VertexBuffer interface. This removes all GPU-related calls from GrTessellator. It also factors vertex drawing into GrTessellatingPathRenderer::drawVertices(), and makes tessellate() (now draw() also responsible for drawing. This means the cache hit case is clearer as an early-out, and storing into cache is done in draw() as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== GrTessellator: abstract vertex allocation into caller. This abstracts all vertex allocation out of GrTessellator via a VertexBuffer interface. This removes all GPU-related calls from GrTessellator. It also factors vertex drawing into GrTessellatingPathRenderer::drawVertices(), and makes tessellate() (now draw() also responsible for drawing. This means the cache hit case is clearer as an early-out, and storing into cache is done in draw() as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/6599efffeef3168dfc68dca99c30454c5c23b859 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://skia.googlesource.com/skia/+/6599efffeef3168dfc68dca99c30454c5c23b859 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
