Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1557083002:
Broke GrTessellatingPathRenderer's tessellator out into a separate file. (Closed)
|
Created:
4 years, 11 months ago by ethannicholas 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. |
DescriptionBroke GrTessellatingPathRenderer's tessellator out into a separate file.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1557083002
Committed: https://skia.googlesource.com/skia/+/8b05cb8a00bdb82e100f1ba74bf4de4a504cceea
Committed: https://skia.googlesource.com/skia/+/e9709e831954c3427d5cb839e84221a177bfedeb
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 5
Patch Set 7 : #
Total comments: 3
Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 37 (17 generated)
Description was changed from ========== Broke GrTessellatingPathRenderer's tessellator out into a separate file. ========== to ========== Broke GrTessellatingPathRenderer's tessellator out into a separate file. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ethannicholas@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/1557083002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557083002/40001
ethannicholas@google.com changed reviewers: + bsalomon@google.com, joshualitt@chromium.org
Per Brian's suggestion, broke the tessellator out to make the access from PLS cleaner.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bsalomon@google.com changed reviewers: + senorblanco@chromium.org
Adding Stephen since this is his baby. The context is Ethan wants to use the tessellator for his PLS path renderer. https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h File src/gpu/GrTessellator.h (right): https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:20: Maybe we should put all these structs and functions inside a namespace? GrPathTessellator? https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:33: struct TessellatorVertex { Does this get used by any client of tessellator? I don't see it in the sig of the two functions at the bottom. https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:104: Poly* path_to_polys(const SkPath& path, SkScalar tolerance, const SkRect& clipBounds, Does this need to be exposed? It looks like Poly is not exposed so the only thing that could be done with the returned value is to pass it to one of the following functions. Would a caller reuse the same returned Poly* multiple times? Wondering if we could just have PathToTriangles and PathToVertices. https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:109: int polys_to_vertices(Poly* polys, SkPath::FillType fillType, bool isLinear, Is this (and polys_to_triangles) destructive to polys?
Some preliminary comments (I reserve the right, but not the obligation, to future complaints. :) ) https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.cpp File src/gpu/GrTessellator.cpp (left): https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.c... src/gpu/GrTessellator.cpp:83: #define LOGGING_ENABLED 0 If we do manage to keep Vertex and friends in the .cpp, let's move the #defines back here as well if possible (not sure if they're shared by both cpp's). https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h File src/gpu/GrTessellator.h (right): https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:20: On 2016/01/04 21:45:21, bsalomon wrote: > Maybe we should put all these structs and functions inside a namespace? > GrPathTessellator? Maybe GrTessellator, to match the .h? Agreed that the current names are too generic to live in the global namespace. Whatever we do, we should probably do it to TessellatorVertex (below) too. (GrTessellator::Vertex?) https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:33: struct TessellatorVertex { On 2016/01/04 21:45:21, bsalomon wrote: > Does this get used by any client of tessellator? I don't see it in the sig of > the two functions at the bottom. +1. IWBN to keep this in the .cpp if not necessary to expose. https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:44: TessellatorVertex* fPrev; // Linked list of contours, then Y-sorted vertices. Nit: spacing of these is weird now. https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:77: TessellatorVertex* fTop; // The top vertex in vertex-sort-order (sweep_lt). Nit: spacing of these is weird now. https://codereview.chromium.org/1557083002/diff/40001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:104: Poly* path_to_polys(const SkPath& path, SkScalar tolerance, const SkRect& clipBounds, On 2016/01/04 21:45:21, bsalomon wrote: > Does this need to be exposed? It looks like Poly is not exposed so the only > thing that could be done with the returned value is to pass it to one of the > following functions. Would a caller reuse the same returned Poly* multiple > times? Wondering if we could just have PathToTriangles and PathToVertices. Maybe they should be in the GrTessellator namespace too? e.g., GrTessellator::PathToVertices()? Then it would be a little clearer that the paths were being tessellated.
I left TESSELLATOR_WIREFRAME where it is, as it is used both in GrTessellator.cpp and GrTessellatingPathRenderer.cpp, but believe I have addressed the rest of the feedback.
https://codereview.chromium.org/1557083002/diff/60001/src/gpu/GrTessellator.h File src/gpu/GrTessellator.h (right): https://codereview.chromium.org/1557083002/diff/60001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:32: struct Edge { Does this (and EdgeList) need to be public? It only seems to be used in GrTessellator.cpp. But maybe I missed something.
https://codereview.chromium.org/1557083002/diff/60001/src/gpu/GrTessellator.h File src/gpu/GrTessellator.h (right): https://codereview.chromium.org/1557083002/diff/60001/src/gpu/GrTessellator.h... src/gpu/GrTessellator.h:32: struct Edge { On 2016/01/05 15:59:04, Stephen White wrote: > Does this (and EdgeList) need to be public? It only seems to be used in > GrTessellator.cpp. But maybe I missed something. It did at one point, but you are of course correct that it does not need to be anymore. My bad!
Improved the comment on PathToVertices
https://codereview.chromium.org/1557083002/diff/100001/src/gpu/GrTessellator.cpp File src/gpu/GrTessellator.cpp (right): https://codereview.chromium.org/1557083002/diff/100001/src/gpu/GrTessellator.... src/gpu/GrTessellator.cpp:89: namespace GrTessellator { Now that they're no longer public, can these things go back in the unnamed namespace? (And maybe back into their original order, to minimize diffs?) https://codereview.chromium.org/1557083002/diff/100001/src/gpu/GrTessellator.... src/gpu/GrTessellator.cpp:1366: return contours_to_polys(contours.get(), contourCnt, path.getBounds(), alloc); We're now returning a pointer to Polys which were allocated by the chunk allocator, which goes out of scope at the end of this function (along with its memory). This doesn't look right. I think the chunk allocator should be allocated by the caller.
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
Patchset #7 (id:180001) has been deleted
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/1557083002/diff/200001/src/gpu/GrTessellator.cpp File src/gpu/GrTessellator.cpp (right): https://codereview.chromium.org/1557083002/diff/200001/src/gpu/GrTessellator.... src/gpu/GrTessellator.cpp:1290: Poly* contours_to_polys(Vertex** contours, int contourCnt, SkRect pathBounds, SkChunkAlloc& alloc) { Nit: SkRect could be const-ref. https://codereview.chromium.org/1557083002/diff/200001/src/gpu/GrTessellator.h File src/gpu/GrTessellator.h (right): https://codereview.chromium.org/1557083002/diff/200001/src/gpu/GrTessellator.... src/gpu/GrTessellator.h:12: #include "GrPathRenderer.h" Are these #includes still necessary? https://codereview.chromium.org/1557083002/diff/200001/src/gpu/batches/GrTess... File src/gpu/batches/GrTessellatingPathRenderer.cpp (right): https://codereview.chromium.org/1557083002/diff/200001/src/gpu/batches/GrTess... src/gpu/batches/GrTessellatingPathRenderer.cpp:61: bool cache_match(GrVertexBuffer* vertexBuffer, SkScalar tol, int* actualCount) { Does this really need to be outside the anonymous namespace? https://codereview.chromium.org/1557083002/diff/200001/src/gpu/batches/GrTess... src/gpu/batches/GrTessellatingPathRenderer.cpp:132: int count = GrTessellator::PathToTriangles(path, tol, fClipBounds, resourceProvider, Should we return without caching here if count is 0?
https://codereview.chromium.org/1557083002/diff/200001/src/gpu/batches/GrTess... File src/gpu/batches/GrTessellatingPathRenderer.cpp (right): https://codereview.chromium.org/1557083002/diff/200001/src/gpu/batches/GrTess... src/gpu/batches/GrTessellatingPathRenderer.cpp:132: int count = GrTessellator::PathToTriangles(path, tol, fClipBounds, resourceProvider, On 2016/01/05 19:12:51, Stephen White wrote: > Should we return without caching here if count is 0? I had assumed that if the triangulation failed, it would continue to do so in the future, so we might as well cache the zero result. I'm happy to change it if you think we should, though.
On 2016/01/05 20:32:05, ethannicholas wrote: > https://codereview.chromium.org/1557083002/diff/200001/src/gpu/batches/GrTess... > File src/gpu/batches/GrTessellatingPathRenderer.cpp (right): > > https://codereview.chromium.org/1557083002/diff/200001/src/gpu/batches/GrTess... > src/gpu/batches/GrTessellatingPathRenderer.cpp:132: int count = > GrTessellator::PathToTriangles(path, tol, fClipBounds, resourceProvider, > On 2016/01/05 19:12:51, Stephen White wrote: > > Should we return without caching here if count is 0? > > I had assumed that if the triangulation failed, it would continue to do so in > the future, so we might as well cache the zero result. I'm happy to change it if > you think we should, though. Good point. I suppose the only way it could succeed in the future is if a vertex buffer failed to allocate due to low memory conditions the first time. But that doesn't seem worth handling, since you'd have bigger problems anyway.
LGTM; remaining comments are at your discretion. https://codereview.chromium.org/1557083002/diff/220001/src/gpu/GrTessellator.cpp File src/gpu/GrTessellator.cpp (right): https://codereview.chromium.org/1557083002/diff/220001/src/gpu/GrTessellator.... src/gpu/GrTessellator.cpp:1342: void get_contour_count_and_size_estimate(const SkPath& path, SkScalar tolerance, int* contourCnt, The code might be slightly clearer (and less error-prone) if it returned the contour count as return value. I leave it up to you, though. https://codereview.chromium.org/1557083002/diff/220001/src/gpu/GrTessellator.... src/gpu/GrTessellator.cpp:1436: for (Poly* poly = polys; poly; poly = poly->fNext) { Nit: this could be refactored with the same code in PathToTriangles above. https://codereview.chromium.org/1557083002/diff/220001/src/gpu/batches/GrTess... File src/gpu/batches/GrTessellatingPathRenderer.cpp (right): https://codereview.chromium.org/1557083002/diff/220001/src/gpu/batches/GrTess... src/gpu/batches/GrTessellatingPathRenderer.cpp:29: GrTessellatingPathRenderer::GrTessellatingPathRenderer() { Nit: please put this with the rest of the class's functions below the anonymous namespace.
The CQ bit was checked by ethannicholas@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/1557083002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1557083002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557083002/240001
Message was sent while issue was closed.
Description was changed from ========== Broke GrTessellatingPathRenderer's tessellator out into a separate file. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Broke GrTessellatingPathRenderer's tessellator out into a separate file. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/8b05cb8a00bdb82e100f1ba74bf4de4a504cceea ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001) as https://skia.googlesource.com/skia/+/8b05cb8a00bdb82e100f1ba74bf4de4a504cceea
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:240001) has been created in https://codereview.chromium.org/1570503002/ by caryclark@google.com. The reason for reverting is: broke valgrind bot.
Message was sent while issue was closed.
Description was changed from ========== Broke GrTessellatingPathRenderer's tessellator out into a separate file. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/8b05cb8a00bdb82e100f1ba74bf4de4a504cceea ========== to ========== Broke GrTessellatingPathRenderer's tessellator out into a separate file. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/8b05cb8a00bdb82e100f1ba74bf4de4a504cceea ==========
The CQ bit was checked by ethannicholas@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/1557083002/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1557083002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557083002/260001
Message was sent while issue was closed.
Description was changed from ========== Broke GrTessellatingPathRenderer's tessellator out into a separate file. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/8b05cb8a00bdb82e100f1ba74bf4de4a504cceea ========== to ========== Broke GrTessellatingPathRenderer's tessellator out into a separate file. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/8b05cb8a00bdb82e100f1ba74bf4de4a504cceea Committed: https://skia.googlesource.com/skia/+/e9709e831954c3427d5cb839e84221a177bfedeb ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://skia.googlesource.com/skia/+/e9709e831954c3427d5cb839e84221a177bfedeb |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
