Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243313002:
Implement Conics in SkCurveMeasure (Closed)
|
Created:
4 years, 4 months ago by Harry Stern Modified:
3 years, 11 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@cubic_fix Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionImplement Conics in SkCurveMeasure
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2243313002
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fix several things #Patch Set 3 : Make some consts static const #Patch Set 4 : Fix flipped sign in conic evalutaion, add comment explaining derivation #Patch Set 5 : Add comment about fixed precision of quadrature used #
Total comments: 8
Patch Set 6 : Refactor coefficients and minor fixups #Messages
Total messages: 27 (16 generated)
Description was changed from ========== Implement Conics in SkCurveMeasure BUG=skia: ========== to ========== Implement Conics in SkCurveMeasure BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2243313002 ==========
The CQ bit was checked by hstern@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...
hstern@google.com changed reviewers: + caryclark@google.com, reed@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
1. When/where do we set fConicW ? 2. Can you add a test or gm that exercises the conic case?
On 2016/08/15 17:55:49, reed1 wrote: > 1. When/where do we set fConicW ? > 2. Can you add a test or gm that exercises the conic case? Yes, I was going to add that in the SkCurvePathMeasure tests that we just spoke about, hopefully in the same CL with the actual SkCurvePathMeasure class
https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cpp File src/utils/SkCurveMeasure.cpp (right): https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cp... src/utils/SkCurveMeasure.cpp:154: fConicW = pts[3].x(); fConicW gets set here
https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cpp File src/utils/SkCurveMeasure.cpp (right): https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cp... src/utils/SkCurveMeasure.cpp:14: #define UNIMPLEMENTED SkDEBUGF(("%s:%d unimplemented\n", __FILE__, __LINE__)) Make this an assert. https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cp... src/utils/SkCurveMeasure.cpp:90: 8*w*w + ts*(4 - 8*w + 4*w*w)))) + 1; Where did this come from? That is, please document your derivation. C++ compilers will not do a good job of refactoring this. I see trivial common terms: -8 + 16*w - 8*w*w 4 - 8*w + 4*w*w Have you looked at the compiled output? Are you getting the SIMD you expect? https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cp... src/utils/SkCurveMeasure.cpp:93: y = ynum / denom; Is there a unit test for this? https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h File src/utils/SkCurveMeasure.h (right): https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:26: static Sk8f weights = Sk8f::Load(weights8); Writable globals are seldom a good idea. Another thread may run before this is initialized. Put all of this where it is used, not here. It is not part of the interface; it is part of the implementation. https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:32: ArcLengthIntegrator() {} Why does this leave fSegType uninitialized? Do you ever call this constructor? If not, remove it. https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:40: Sk8f xCoeff[3]; Preceed with 'f' https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:43: SkScalar fConicW = 0; Why zero? That's a valid weight; do you mean for it to mean something special? If you want to detect uninitialized fields, choose a number that is negative or NaN. Also, if this is a special value, document it. https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:54: // kConic_SegType -> 4 points: start control end (w, w) Do you repeat w twice? Do you rely on this? Why?
https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cpp File src/utils/SkCurveMeasure.cpp (right): https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cp... src/utils/SkCurveMeasure.cpp:14: #define UNIMPLEMENTED SkDEBUGF(("%s:%d unimplemented\n", __FILE__, __LINE__)) On 2016/08/15 19:13:04, caryclark wrote: > Make this an assert. Done, depends on https://codereview.chromium.org/2244253003/ to build. https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cp... src/utils/SkCurveMeasure.cpp:90: 8*w*w + ts*(4 - 8*w + 4*w*w)))) + 1; On 2016/08/15 19:13:04, caryclark wrote: > Where did this come from? That is, please document your derivation. > > C++ compilers will not do a good job of refactoring this. I see trivial common > terms: > -8 + 16*w - 8*w*w > 4 - 8*w + 4*w*w > > Have you looked at the compiled output? Are you getting the SIMD you expect? I took out those terms. I honestly didn't notice them when I looked because I was too fixated on the 12. I am getting SIMD from this function in the output. `objdump -S -w -d out/Release/obj/src/utils/utils.SkCurveMeasure.o` is showing me that the function was inlined and is using e.g. mulps, addps, sqrtps https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cp... src/utils/SkCurveMeasure.cpp:93: y = ynum / denom; On 2016/08/15 19:13:04, caryclark wrote: > Is there a unit test for this? It's on my todo by tomorrow. https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h File src/utils/SkCurveMeasure.h (right): https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:26: static Sk8f weights = Sk8f::Load(weights8); On 2016/08/15 19:13:04, caryclark wrote: > Writable globals are seldom a good idea. Another thread may run before this is > initialized. Put all of this where it is used, not here. It is not part of the > interface; it is part of the implementation. These should all have be static const - they never get written to. Should I still put them as class members of ArcLengthIntegrator or are they fine as long as they're const? https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:32: ArcLengthIntegrator() {} On 2016/08/15 19:13:04, caryclark wrote: > Why does this leave fSegType uninitialized? > Do you ever call this constructor? If not, remove it. I think I need the default constructor for the CurveMeasure constructor: SkCurveMeasure contains an ArcLengthIntegrator and it doesn't get initialized until the bottom of the SkCurveMeasure constructor. I don't know a better way to do this. https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:40: Sk8f xCoeff[3]; On 2016/08/15 19:13:04, caryclark wrote: > Preceed with 'f' Done. https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:43: SkScalar fConicW = 0; On 2016/08/15 19:13:04, caryclark wrote: > Why zero? That's a valid weight; do you mean for it to mean something special? > If you want to detect uninitialized fields, choose a number that is negative or > NaN. > > Also, if this is a special value, document it. This variable only gets read when we are a conic, and it will always be initialized to a correct value in such a case. Should I just make it uninitialized or set it to an invalid value e.g. NaN as you suggest? https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.h#... src/utils/SkCurveMeasure.h:54: // kConic_SegType -> 4 points: start control end (w, w) On 2016/08/15 19:13:04, caryclark wrote: > Do you repeat w twice? Do you rely on this? Why? I do set it to (w, w) for no particular reason, but I don't rely on it. I'm pretty sure everywhere I use it, I just take the x value.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cpp File src/utils/SkCurveMeasure.cpp (right): https://codereview.chromium.org/2243313002/diff/1/src/utils/SkCurveMeasure.cp... src/utils/SkCurveMeasure.cpp:90: 8*w*w + ts*(4 - 8*w + 4*w*w)))) + 1; On 2016/08/15 20:55:16, Harry Stern wrote: > On 2016/08/15 19:13:04, caryclark wrote: > > Where did this come from? That is, please document your derivation. > > > > C++ compilers will not do a good job of refactoring this. I see trivial common > > terms: > > -8 + 16*w - 8*w*w > > 4 - 8*w + 4*w*w > > > > Have you looked at the compiled output? Are you getting the SIMD you expect? > > I took out those terms. I honestly didn't notice them when I looked because I > was too fixated on the 12. > > I am getting SIMD from this function in the output. `objdump -S -w -d > out/Release/obj/src/utils/utils.SkCurveMeasure.o` is showing me that the > function was inlined and is using e.g. mulps, addps, sqrtps Also, I have added the mathematica commands used to generate the coefficients. Is there something better I could do?
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by hstern@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.
cary, is this good to go?
https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... File src/utils/SkCurveMeasure.cpp (right): https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... src/utils/SkCurveMeasure.cpp:130: + ts*(w484)))) + 1; More factoring is possible here, e.g. -(-4 + 4*w) + w484 https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... src/utils/SkCurveMeasure.cpp:205: fYCoeff[2] = Sk8f(-2*(Ay*w - By*w)); Can come elements, e.g. -2*(Ax*w - Bx*w) be factored out here? https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... src/utils/SkCurveMeasure.cpp:223: lengths = lengths*(t*0.5f); Thanks for adding spaces around the 'weight * lengths' above. Can you add spaces everywhere on both sides of multiplies? https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... src/utils/SkCurveMeasure.cpp:288: (SkScalarNearlyEqual(targetLength, currentLength))) { parens unneeded targetLength - some_epsilon >= currentLength
https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... File src/utils/SkCurveMeasure.cpp (right): https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... src/utils/SkCurveMeasure.cpp:130: + ts*(w484)))) + 1; On 2016/08/19 11:48:29, caryclark wrote: > More factoring is possible here, e.g. -(-4 + 4*w) + w484 Oh clever, I didn't think of that. Done. https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... src/utils/SkCurveMeasure.cpp:205: fYCoeff[2] = Sk8f(-2*(Ay*w - By*w)); On 2016/08/19 11:48:29, caryclark wrote: > Can come elements, e.g. -2*(Ax*w - Bx*w) be factored out here? Sure. The only reason I didn't do it initially is because this is called at construction, so it only happens once per segment of a path. Done. https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... src/utils/SkCurveMeasure.cpp:223: lengths = lengths*(t*0.5f); On 2016/08/19 11:48:30, caryclark wrote: > Thanks for adding spaces around the 'weight * lengths' above. Can you add spaces > everywhere on both sides of multiplies? In a previous review mtklein basically said not to do so everywhere, though I thought it was fine for the above two. He gave the reasoning that multiplication binds tighter than addition, essentially. I really don't have a preference either way. https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... src/utils/SkCurveMeasure.cpp:288: (SkScalarNearlyEqual(targetLength, currentLength))) { On 2016/08/19 11:48:29, caryclark wrote: > parens unneeded > > targetLength - some_epsilon >= currentLength Per discussion we're going to leave it like it is.
The CQ bit was checked by hstern@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.
On 2016/08/19 at 16:31:28, hstern wrote: > https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... > File src/utils/SkCurveMeasure.cpp (right): > > https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... > src/utils/SkCurveMeasure.cpp:130: + ts*(w484)))) + 1; > On 2016/08/19 11:48:29, caryclark wrote: > > More factoring is possible here, e.g. -(-4 + 4*w) + w484 > > Oh clever, I didn't think of that. Done. > > https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... > src/utils/SkCurveMeasure.cpp:205: fYCoeff[2] = Sk8f(-2*(Ay*w - By*w)); > On 2016/08/19 11:48:29, caryclark wrote: > > Can come elements, e.g. -2*(Ax*w - Bx*w) be factored out here? > > Sure. The only reason I didn't do it initially is because this is called at construction, so it only happens once per segment of a path. > > Done. > > https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... > src/utils/SkCurveMeasure.cpp:223: lengths = lengths*(t*0.5f); > On 2016/08/19 11:48:30, caryclark wrote: > > Thanks for adding spaces around the 'weight * lengths' above. Can you add spaces > > everywhere on both sides of multiplies? > > In a previous review mtklein basically said not to do so everywhere, though I thought it was fine for the above two. > > He gave the reasoning that multiplication binds tighter than addition, essentially. > > I really don't have a preference either way. > > https://codereview.chromium.org/2243313002/diff/100001/src/utils/SkCurveMeasu... > src/utils/SkCurveMeasure.cpp:288: (SkScalarNearlyEqual(targetLength, currentLength))) { > On 2016/08/19 11:48:29, caryclark wrote: > > parens unneeded > > > > targetLength - some_epsilon >= currentLength > > Per discussion we're going to leave it like it is. ping/ptal |
