Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1643143002:
Generate Signed Distance Field directly from vector path (Closed)
|
Created:
4 years, 10 months ago by Joel.Liang Modified:
3 years, 11 months ago CC:
reviews_skia.org, developer_arm.com, rmistry Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionGenerate Signed Distance Field directly from vector path
Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1643143002
Committed: https://skia.googlesource.com/skia/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f
Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549
Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268
Review-Url: https://codereview.chromium.org/1643143002
Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4
Review-Url: https://codereview.chromium.org/1643143002
Committed: https://skia.googlesource.com/skia/+/6d2f73c364d0d823f14d1ddebc88e0bcbc8f0634
Review-Url: https://codereview.chromium.org/1643143002
Committed: https://skia.googlesource.com/skia/+/8cbb4246e58c97e2ac51087d2708795b3b59f5e9
Patch Set 1 #Patch Set 2 : Fix failed to build on some Trybot #Patch Set 3 : Fix MSVC build #Patch Set 4 : Move implementation to src/gpu #
Total comments: 36
Patch Set 5 : Simplify path and add comments #
Total comments: 2
Patch Set 6 : DScalar to double #Patch Set 7 : Clear distance field for degenerate paths #
Total comments: 12
Patch Set 8 : Remove get direction and add text SDF support #
Total comments: 2
Patch Set 9 : Change distance mapping #Patch Set 10 : Rebase and sync SDF encoding implementation #Patch Set 11 : Fix Clang & MSVC build error #Patch Set 12 : Fix MSVC build error #Patch Set 13 : Fix convert double to float #Patch Set 14 : Fix GM test artifacts & winding number assertion #Patch Set 15 : Transform tolerance to fix winding number assertion issue. #Patch Set 16 : Rebase #Patch Set 17 : Check Simplify result for SDF renderer #Patch Set 18 : fix clang warning #Patch Set 19 : Tune tolerance and use SkPath::contains #Patch Set 20 : rebase #Patch Set 21 : Added tangent line tolerance and tuned performance #Patch Set 22 : rebase #Patch Set 23 : rebase #Patch Set 24 : Added assertion to check path bounds #
Total comments: 12
Patch Set 25 : Temporary use legacy distance field #
Messages
Total messages: 171 (62 generated)
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. Currently only support even odd fill type. BUG=skia: ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. Currently only support even odd fill type. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. Currently only support even odd fill type. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. Currently only support even odd fill type. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
joel.liang@arm.com changed reviewers: + bsalomon@google.com, jvanverth@google.com
Hi Brian, Jim, I have submitted our Signed Distance Field generation algorithm. Please review it. Feel free to ask me if you have any questions. Best regards, Joel Liang
The CQ bit was checked by joel.liang@arm.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/1643143002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-01-29 11:14 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by joel.liang@arm.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/1643143002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 joel.liang@arm.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/1643143002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Sorry, I can't pass the CPU related build. That's because the SkDistanceFieldGenFromVector.cpp file which in src/core directory needs to use a function GrPathUtils::convertCubicToQuads which defined in the src/gpu directory. It will failed to include/link to the GPU directory in the CPU related build. Please help me to solve this problem. On 2016/01/29 05:11:28, Joel.Liang wrote: > Hi Brian, Jim, > > I have submitted our Signed Distance Field generation algorithm. > Please review it. > > Feel free to ask me if you have any questions. > > Best regards, > Joel Liang
On 2016/01/29 07:50:44, Joel.Liang wrote: > Sorry, I can't pass the CPU related build. > > That's because the SkDistanceFieldGenFromVector.cpp file which in src/core > directory needs to > use a function GrPathUtils::convertCubicToQuads which defined in the src/gpu > directory. > > It will failed to include/link to the GPU directory in the CPU related build. > > Please help me to solve this problem. Would it be OK to just move the implementation to src/gpu (and rename from SkFoo to GrFoo)? I don't think we need this built in a CPU-only build. > > On 2016/01/29 05:11:28, Joel.Liang wrote: > > Hi Brian, Jim, > > > > I have submitted our Signed Distance Field generation algorithm. > > Please review it. > > > > Feel free to ask me if you have any questions. > > > > Best regards, > > Joel Liang
The CQ bit was checked by joel.liang@arm.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/1643143002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
On 2016/01/29 16:48:13, bsalomon wrote: > On 2016/01/29 07:50:44, Joel.Liang wrote: > > Sorry, I can't pass the CPU related build. > > > > That's because the SkDistanceFieldGenFromVector.cpp file which in src/core > > directory needs to > > use a function GrPathUtils::convertCubicToQuads which defined in the src/gpu > > directory. > > > > It will failed to include/link to the GPU directory in the CPU related build. > > > > Please help me to solve this problem. > > Would it be OK to just move the implementation to src/gpu (and rename from SkFoo > to GrFoo)? I don't think we need this built in a CPU-only build. > Done. I have moved the implementation to src/gpu. And rename the function/file prefix to Gr. > > > > On 2016/01/29 05:11:28, Joel.Liang wrote: > > > Hi Brian, Jim, > > > > > > I have submitted our Signed Distance Field generation algorithm. > > > Please review it. > > > > > > Feel free to ask me if you have any questions. > > > > > > Best regards, > > > Joel Liang
On 2016/02/01 05:54:09, Joel.Liang wrote: > On 2016/01/29 16:48:13, bsalomon wrote: > > On 2016/01/29 07:50:44, Joel.Liang wrote: > > > Sorry, I can't pass the CPU related build. > > > > > > That's because the SkDistanceFieldGenFromVector.cpp file which in src/core > > > directory needs to > > > use a function GrPathUtils::convertCubicToQuads which defined in the src/gpu > > > directory. > > > > > > It will failed to include/link to the GPU directory in the CPU related > build. > > > > > > Please help me to solve this problem. > > > > Would it be OK to just move the implementation to src/gpu (and rename from > SkFoo > > to GrFoo)? I don't think we need this built in a CPU-only build. > > > > Done. I have moved the implementation to src/gpu. > And rename the function/file prefix to Gr. > > > > > > > On 2016/01/29 05:11:28, Joel.Liang wrote: > > > > Hi Brian, Jim, > > > > > > > > I have submitted our Signed Distance Field generation algorithm. > > > > Please review it. > > > > > > > > Feel free to ask me if you have any questions. > > > > > > > > Best regards, > > > > Joel Liang Hi Joel, just wanted to let you know I've been looking at it and will have comments to publish tomorrow.
https://codereview.chromium.org/1643143002/diff/60001/src/core/SkDistanceFiel... File src/core/SkDistanceFieldGen.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/core/SkDistanceFiel... src/core/SkDistanceFieldGen.cpp:320: if (dist <= -(distanceMagnitude*(1.0f-1.0f/128.0f))) { Can you add a comment that explains this constant? https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:36: enum SegSide { nit, we usually put the full name of the enum after the underscore so either enum SegSide { kLeft_SegSide, ... }; or enum Side { kLeft_Side, ... }; https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:92: class DMatrix { SkMatrix promotes itself to doubles for some operations (offhand not totally sure which). I'm just wondering if you tried it and it didn't give sufficient precision or the conversions were too expensive. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:161: static inline bool between_closed_open(double a, double b, double c, Should these doubles be DScalars? (same for the constants above) https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:187: static inline float sign_of(const float &val) { Do we not have a helper for this already? https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:267: const double a = pow(b0y - (2.0 * cp1y) + b2y, 2.0); DScalars? https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:269: const double b = pow(b0x - (2.0 * cp1x) + b2x, 2.0); Are pow(<foo>, 2.0)s as fast as <foo>*<foo>? https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:389: #define THIRD float(0.33333333333f) If you're only using these in this function maybe static const float? https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:423: struct RowData Some comments on this struct might be useful (e.g. what fSignB0B2 is) https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:424: { { on prev line https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:432: double fPow2x; DScalars? https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:452: const double x1 = xFormPtLeft.x(); DScalars? https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:632: && !segment.fBoundingBox.roundOut() && on prev line, looks like everything following && fits on one line (we use 100 col). https://codereview.chromium.org/1643143002/diff/60001/src/gpu/batches/GrAADis... File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/gpu/batches/GrAADis... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:376: // TODO: Support non-zero fill type (SkPath::kWinding_FillType) Have you tried using using Simplify from SkPathOps.h for winding fill?
Hi Brian, I am using the Simplify from SkPathOps.h to simplify and change the fill type to even-odd. I can now pass all the tests in DM and generate all SDFs using new algorithm correctly. :) Best Regards, Joel Liang https://codereview.chromium.org/1643143002/diff/60001/src/core/SkDistanceFiel... File src/core/SkDistanceFieldGen.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/core/SkDistanceFiel... src/core/SkDistanceFieldGen.cpp:320: if (dist <= -(distanceMagnitude*(1.0f-1.0f/128.0f))) { On 2016/02/02 18:03:48, bsalomon wrote: > Can you add a comment that explains this constant? Done. And I'd like to give a example here: If dist is -3.99999, the result became 0. The correct result should be 255. input: distanceMagnitude = 4.0 dist = -3.99999 result: (unsigned char)((distanceMagnitude-dist)*128.0/distanceMagnitude) => (unsigned char)((4.0-(-3.99999))*128.0/4.0) => (unsigned char)(255.99968) => 0 EDIT: I have changed the way we implement this method to more clear about the constant. The result should be the same as the original one. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:36: enum SegSide { On 2016/02/02 18:03:48, bsalomon wrote: > nit, we usually put the full name of the enum after the underscore so either > > enum SegSide { > kLeft_SegSide, > ... > }; > > or > > enum Side { > kLeft_Side, > ... > }; > > > Done. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:92: class DMatrix { On 2016/02/02 18:03:48, bsalomon wrote: > SkMatrix promotes itself to doubles for some operations (offhand not totally > sure which). I'm just wondering if you tried it and it didn't give sufficient > precision or the conversions were too expensive. I just tried SkMatrix, it didn't give sufficient precision for this algorithm. And we only use this matrix for affine transformation. So I did some modifications here. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:161: static inline bool between_closed_open(double a, double b, double c, On 2016/02/02 18:03:48, bsalomon wrote: > Should these doubles be DScalars? (same for the constants above) Done. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:187: static inline float sign_of(const float &val) { On 2016/02/02 18:03:48, bsalomon wrote: > Do we not have a helper for this already? For input value 0.0, we need 1.0 return here. The helper SkScalarSignAsScalar we have returns 0.0 in this case. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:247: ); This matrix is a transposed matrix of general OpenGL/Skia matrix. So I decided to transpose back in the next commit. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:267: const double a = pow(b0y - (2.0 * cp1y) + b2y, 2.0); On 2016/02/02 18:03:48, bsalomon wrote: > DScalars? Done. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:269: const double b = pow(b0x - (2.0 * cp1x) + b2x, 2.0); On 2016/02/02 18:03:48, bsalomon wrote: > Are pow(<foo>, 2.0)s as fast as <foo>*<foo>? I done a performance test for pow and <foo>*<foo>, the latter one have better performance. So I decided change pow to <foo>*<foo>. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:308: ); Transpose the matrix in the next commit. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:389: #define THIRD float(0.33333333333f) On 2016/02/02 18:03:48, bsalomon wrote: > If you're only using these in this function maybe static const float? Done. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:423: struct RowData On 2016/02/02 18:03:48, bsalomon wrote: > Some comments on this struct might be useful (e.g. what fSignB0B2 is) Done. I have changed some member variable name and added comments to them. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:424: { On 2016/02/02 18:03:48, bsalomon wrote: > { on prev line Done. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:432: double fPow2x; On 2016/02/02 18:03:48, bsalomon wrote: > DScalars? Done. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:452: const double x1 = xFormPtLeft.x(); On 2016/02/02 18:03:48, bsalomon wrote: > DScalars? Done. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:632: && !segment.fBoundingBox.roundOut() On 2016/02/02 18:03:48, bsalomon wrote: > && on prev line, looks like everything following && fits on one line (we use 100 > col). Not fits within 100 col. So I split it into 2 lines instead of 4 lines. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/batches/GrAADis... File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/gpu/batches/GrAADis... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:376: // TODO: Support non-zero fill type (SkPath::kWinding_FillType) On 2016/02/02 18:03:48, bsalomon wrote: > Have you tried using using Simplify from SkPathOps.h for winding fill? Wow, now we can pass all test cases in DM by using the new algorithm after simplify the paths.
https://codereview.chromium.org/1643143002/diff/60001/src/core/SkDistanceFiel... File src/core/SkDistanceFieldGen.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/core/SkDistanceFiel... src/core/SkDistanceFieldGen.cpp:320: if (dist <= -(distanceMagnitude*(1.0f-1.0f/128.0f))) { On 2016/02/04 12:12:04, Joel.Liang wrote: > On 2016/02/02 18:03:48, bsalomon wrote: > > Can you add a comment that explains this constant? > > Done. And I'd like to give a example here: > > If dist is -3.99999, the result became 0. > The correct result should be 255. > > input: > distanceMagnitude = 4.0 > dist = -3.99999 > result: > (unsigned char)((distanceMagnitude-dist)*128.0/distanceMagnitude) > => (unsigned char)((4.0-(-3.99999))*128.0/4.0) > => (unsigned char)(255.99968) > => 0 > > EDIT: I have changed the way we implement this method to more clear about the > constant. The result should be the same as the original one. Thanks, that equation is a lot clearer (and thanks for finding that bug).
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:55: typedef double DScalar; We could probably just type "double". SkScalar only exists to abstract away whether it's float or double, but this one's always going to be double. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:148: return DPoint::Make(fMat[kMScaleX] * src.x() + fMat[kMSkewY] * src.y() + fMat[kMPersp0], If you only use this matrix for affine transformations, why does it have 9 components, and why is this map points using the perspective components? Typically we'd expect an affine transformation to take the form: x' = x*sx + y*kx + tx y' = y*sy + x*ky + ty, where s == scale, k == skew, t == translate. You appear to have written this as x' = x*sx + y*ky + p0 y' = x*kx + y*sy + p1 I'm still not quite awake, but I think what you've written here is just the transpose of what your terms say they mean. Let's at least try relabeling the coefficients and removing unused coefficients. We might even try using SkMatrix again now that we've realized we were working with the transpose...
wasim.abbas@arm.com changed reviewers: + wasim.abbas@arm.com
https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:148: return DPoint::Make(fMat[kMScaleX] * src.x() + fMat[kMSkewY] * src.y() + fMat[kMPersp0], On 2016/02/04 14:32:59, mtklein wrote: > If you only use this matrix for affine transformations, why does it have 9 > components, and why is this map points using the perspective components? > > Typically we'd expect an affine transformation to take the form: > x' = x*sx + y*kx + tx > y' = y*sy + x*ky + ty, > > where s == scale, k == skew, t == translate. You appear to have written this as > > x' = x*sx + y*ky + p0 > y' = x*kx + y*sy + p1 > > I'm still not quite awake, but I think what you've written here is just the > transpose of what your terms say they mean. > > Let's at least try relabeling the coefficients and removing unused coefficients. > We might even try using SkMatrix again now that we've realized we were working > with the transpose... In Patch Set 5 we have changed this matrix code. It only has 6 components now. And I think we have answered your concerns of transpose as well.
On 2016/02/04 14:48:38, Wasim.Abbas wrote: > https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... > File src/gpu/GrDistanceFieldGenFromVector.cpp (right): > > https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... > src/gpu/GrDistanceFieldGenFromVector.cpp:148: return DPoint::Make(fMat[kMScaleX] > * src.x() + fMat[kMSkewY] * src.y() + fMat[kMPersp0], > On 2016/02/04 14:32:59, mtklein wrote: > > If you only use this matrix for affine transformations, why does it have 9 > > components, and why is this map points using the perspective components? > > > > Typically we'd expect an affine transformation to take the form: > > x' = x*sx + y*kx + tx > > y' = y*sy + x*ky + ty, > > > > where s == scale, k == skew, t == translate. You appear to have written this > as > > > > x' = x*sx + y*ky + p0 > > y' = x*kx + y*sy + p1 > > > > I'm still not quite awake, but I think what you've written here is just the > > transpose of what your terms say they mean. > > > > Let's at least try relabeling the coefficients and removing unused > coefficients. > > We might even try using SkMatrix again now that we've realized we were > working > > with the transpose... > > In Patch Set 5 we have changed this matrix code. It only has 6 components now. > And I think we have answered your concerns of transpose as well. Sorry I missed that! Yes, looks completely sorted out.
https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, &simplifiedPath); I suppose we don't need to simplify every path. should this be conditional? We only added this to not have to deal with ZeroFill rule.
On 2016/02/04 16:43:41, Wasim.Abbas wrote: > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, > &simplifiedPath); > I suppose we don't need to simplify every path. should this be conditional? We > only added this to not have to deal with ZeroFill rule. Do we have a test case for even/odd with coincident edges?
https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, &simplifiedPath); On 2016/02/04 16:43:41, Wasim.Abbas wrote: > I suppose we don't need to simplify every path. should this be conditional? We > only added this to not have to deal with ZeroFill rule. Sorry I meant non-zero file rule.
On 2016/02/04 17:00:47, bsalomon wrote: > On 2016/02/04 16:43:41, Wasim.Abbas wrote: > > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > > File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): > > > > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > > src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, > > &simplifiedPath); > > I suppose we don't need to simplify every path. should this be conditional? We > > only added this to not have to deal with ZeroFill rule. > > Do we have a test case for even/odd with coincident edges? No, we don't have that test case for even-odd but we have it for non-zero. And it will generate a wrong SDF in that case. Because we are now calculate distance for every edges. So I assume it will generate a wrong SDF too for coincident edges with even-odd fill rule. Simplify is needed for that case.
I have uploaded a patch set for DScalar to double. https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... src/gpu/GrDistanceFieldGenFromVector.cpp:55: typedef double DScalar; On 2016/02/04 14:32:59, mtklein wrote: > We could probably just type "double". SkScalar only exists to abstract away > whether it's float or double, but this one's always going to be double. Done.
On 2016/02/05 02:29:30, Joel.Liang wrote: > On 2016/02/04 17:00:47, bsalomon wrote: > > On 2016/02/04 16:43:41, Wasim.Abbas wrote: > > > > > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > > > File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): > > > > > > > > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > > > src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, > > > &simplifiedPath); > > > I suppose we don't need to simplify every path. should this be conditional? > We > > > only added this to not have to deal with ZeroFill rule. > > > > Do we have a test case for even/odd with coincident edges? > > No, we don't have that test case for even-odd but we have it for non-zero. > And it will generate a wrong SDF in that case. Because we are now calculate > distance for every edges. > So I assume it will generate a wrong SDF too for coincident edges with even-odd > fill rule. > Simplify is needed for that case. Just to make sure I understand - with simplify the non-zero test case does draw correctly? Can you add a non-zero coincident test case to one of the GMs?
On 2016/02/05 10:10:55, Joel.Liang wrote: > I have uploaded a patch set for DScalar to double. > > https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... > File src/gpu/GrDistanceFieldGenFromVector.cpp (right): > > https://codereview.chromium.org/1643143002/diff/60001/src/gpu/GrDistanceField... > src/gpu/GrDistanceFieldGenFromVector.cpp:55: typedef double DScalar; > On 2016/02/04 14:32:59, mtklein wrote: > > We could probably just type "double". SkScalar only exists to abstract away > > whether it's float or double, but this one's always going to be double. > > Done. I like the change to just use double.
On 2016/02/05 13:22:02, bsalomon wrote: > On 2016/02/05 02:29:30, Joel.Liang wrote: > > On 2016/02/04 17:00:47, bsalomon wrote: > > > On 2016/02/04 16:43:41, Wasim.Abbas wrote: > > > > > > > > > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > > > > File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > > > > src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, > > > > &simplifiedPath); > > > > I suppose we don't need to simplify every path. should this be > conditional? > > We > > > > only added this to not have to deal with ZeroFill rule. > > > > > > Do we have a test case for even/odd with coincident edges? > > > > No, we don't have that test case for even-odd but we have it for non-zero. > > And it will generate a wrong SDF in that case. Because we are now calculate > > distance for every edges. > > So I assume it will generate a wrong SDF too for coincident edges with > even-odd > > fill rule. > > Simplify is needed for that case. > > Just to make sure I understand - with simplify the non-zero test case does draw > correctly? Yes > Can you add a non-zero coincident test case to one of the GMs? Yes we can add one. We trusted Simplify() will take care of such issues and we thought its already tested? We don't have any special code path for non-zero fill rule paths.
The CQ bit was checked by joel.liang@arm.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/1643143002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
Hi Brian, I have passed all the test on Galaxy S6 before, but it broke the Trybot on other test platforms. Sorry about that. And here is a fix for it. (Now I have passed the DM and nanobench tests for both S6 and Ubuntu platforms) (Another approach to fix this issue is remove the assertion when fallback to the old algorithm.) BTW: The Lunar New Year is coming. I will on holiday next week. Any question please ask Wasim for help. Happy Lunar New Year. :) Best Regards, Joel Liang https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.cpp:695: // Clear distance field for degenerate paths. To avoid "return false" and then fallback to old algorithm. I decided to clear the distance field and return true here. The old algorithm also generate an empty distance field in this case.
On 2016/02/05 16:46:33, Wasim.Abbas wrote: > On 2016/02/05 13:22:02, bsalomon wrote: > > On 2016/02/05 02:29:30, Joel.Liang wrote: > > > On 2016/02/04 17:00:47, bsalomon wrote: > > > > On 2016/02/04 16:43:41, Wasim.Abbas wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > > > > > File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1643143002/diff/80001/src/gpu/batches/GrAADis... > > > > > src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, > > > > > &simplifiedPath); > > > > > I suppose we don't need to simplify every path. should this be > > conditional? > > > We > > > > > only added this to not have to deal with ZeroFill rule. > > > > > > > > Do we have a test case for even/odd with coincident edges? > > > > > > No, we don't have that test case for even-odd but we have it for non-zero. > > > And it will generate a wrong SDF in that case. Because we are now calculate > > > distance for every edges. > > > So I assume it will generate a wrong SDF too for coincident edges with > > even-odd > > > fill rule. > > > Simplify is needed for that case. > > > > Just to make sure I understand - with simplify the non-zero test case does > draw > > correctly? > Yes > > > Can you add a non-zero coincident test case to one of the GMs? > Yes we can add one. We trusted Simplify() will take care of such issues and we > thought its already tested? We don't have any special code path for non-zero > fill rule paths. PathOps is well tested. My concern is in a future revision of someone may decide to not call Simplify all the time. Had an earlier version of this change landed we would have had a bug that no test caught (at least I think). So it seems like a good idea to expand test coverage so that we don't regress in the future. Also, it'd be useful to demonstrate that calling Simplify really is necessary for even/odd paths.
Jim, shouldn't this hook into text rendering somewhere? https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.cpp:695: // Clear distance field for degenerate paths. On 2016/02/06 06:16:00, Joel.Liang wrote: > To avoid "return false" and then fallback to old algorithm. > I decided to clear the distance field and return true here. > The old algorithm also generate an empty distance field in this case. My understanding is that this can fail for non-empty paths. Maybe this is a bug in the old code? https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, &simplifiedPath); Should this go into GrGenerateDistanceFieldFromPath? It seems like every caller would want to do this. https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:379: if (SkPath::kEvenOdd_FillType == simplifiedPath.getFillType()) { Can this be an assert rather than a branch? https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:386: SkASSERT(false && This seems like a lot of code for a fallback we never expect to hit (and would crash in debug on the assert). Can we just setup the batch to not draw anything?
https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.cpp:695: // Clear distance field for degenerate paths. On 2016/02/06 13:22:06, bsalomon wrote: > On 2016/02/06 06:16:00, Joel.Liang wrote: > > To avoid "return false" and then fallback to old algorithm. > > I decided to clear the distance field and return true here. > > The old algorithm also generate an empty distance field in this case. > > My understanding is that this can fail for non-empty paths. Maybe this is a bug > in the old code? If this can fail for non-empty paths, then there is no way to render all paths using the new algorithm. Do we really need the "dir"(SkPathPriv::FirstDirection) parameter for the cubic to quadratic conversion? Can we just pass "SkPathPriv::kCW_FirstDirection" or "SkPathPriv::kCCW_FirstDirection" as parameter for the function "GrPathUtils::convertCubicToQuads"? https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:386: SkASSERT(false && On 2016/02/06 13:22:06, bsalomon wrote: > This seems like a lot of code for a fallback we never expect to hit (and would > crash in debug on the assert). Can we just setup the batch to not draw anything? We will need this fallback if "get_direction" may fail for non-empty paths. I'm happy to remove all this fallback code if we can handle all non-empty paths using the new algorithm.
Hi, Joel. Hope you had a nice time off. https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.cpp:695: // Clear distance field for degenerate paths. On 2016/02/15 08:36:48, Joel.Liang wrote: > On 2016/02/06 13:22:06, bsalomon wrote: > > On 2016/02/06 06:16:00, Joel.Liang wrote: > > > To avoid "return false" and then fallback to old algorithm. > > > I decided to clear the distance field and return true here. > > > The old algorithm also generate an empty distance field in this case. > > > > My understanding is that this can fail for non-empty paths. Maybe this is a > bug > > in the old code? > > If this can fail for non-empty paths, then there is no way to render all paths > using the new algorithm. > > Do we really need the "dir"(SkPathPriv::FirstDirection) parameter for the cubic > to quadratic conversion? > Can we just pass "SkPathPriv::kCW_FirstDirection" or > "SkPathPriv::kCCW_FirstDirection" as parameter for the function > "GrPathUtils::convertCubicToQuads"? Specifying dir is only required if true is passed as the third param. This interface (in GrPathUtils) is really ugly. I will land a CL that adds a variant that doesn't take the bool or direction.
Sorry about the delay. To hook this into SDF text, I think the only place where things need to change is in GrFontScaler::getPackedGlyphDFImage(). Rather than using SkGlyphCache::findImage() you can use SkGlyphCache::findPath(), and then generate the distance field from the path.
I have removed the get_direction call and the fallback code. I also add text SDF generation support as Jim said. https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/120001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.cpp:695: // Clear distance field for degenerate paths. On 2016/02/16 14:59:34, bsalomon wrote: > On 2016/02/15 08:36:48, Joel.Liang wrote: > > On 2016/02/06 13:22:06, bsalomon wrote: > > > On 2016/02/06 06:16:00, Joel.Liang wrote: > > > > To avoid "return false" and then fallback to old algorithm. > > > > I decided to clear the distance field and return true here. > > > > The old algorithm also generate an empty distance field in this case. > > > > > > My understanding is that this can fail for non-empty paths. Maybe this is a > > bug > > > in the old code? > > > > If this can fail for non-empty paths, then there is no way to render all paths > > using the new algorithm. > > > > Do we really need the "dir"(SkPathPriv::FirstDirection) parameter for the > cubic > > to quadratic conversion? > > Can we just pass "SkPathPriv::kCW_FirstDirection" or > > "SkPathPriv::kCCW_FirstDirection" as parameter for the function > > "GrPathUtils::convertCubicToQuads"? > > Specifying dir is only required if true is passed as the third param. This > interface (in GrPathUtils) is really ugly. I will land a CL that adds a variant > that doesn't take the bool or direction. ok, I'm now pass false as the third param. And will rebase the code to use the new API you introduced after you all happy with my changes. https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:377: Simplify(path, &simplifiedPath); On 2016/02/06 13:22:06, bsalomon wrote: > Should this go into GrGenerateDistanceFieldFromPath? It seems like every caller > would want to do this. Done. https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:379: if (SkPath::kEvenOdd_FillType == simplifiedPath.getFillType()) { On 2016/02/06 13:22:06, bsalomon wrote: > Can this be an assert rather than a branch? Done and moved the assertion into GrGenerateDistanceFieldFromPath. https://codereview.chromium.org/1643143002/diff/120001/src/gpu/batches/GrAADi... src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:386: SkASSERT(false && On 2016/02/15 08:36:48, Joel.Liang wrote: > On 2016/02/06 13:22:06, bsalomon wrote: > > This seems like a lot of code for a fallback we never expect to hit (and would > > crash in debug on the assert). Can we just setup the batch to not draw > anything? > > We will need this fallback if "get_direction" may fail for non-empty paths. > I'm happy to remove all this fallback code if we can handle all non-empty paths > using the new algorithm. I have removed the fallback code as now we can handle all paths using the new algorithm.
https://codereview.chromium.org/1643143002/diff/140001/src/core/SkDistanceFie... File src/core/SkDistanceFieldGen.cpp (right): https://codereview.chromium.org/1643143002/diff/140001/src/core/SkDistanceFie... src/core/SkDistanceFieldGen.cpp:320: // The distance field is constructed as unsigned char values, so that the zero value is at 128, I was looking at this to understand it a little better, so I could possibly submit it separately a little early (we have a Chrome branch coming up next week). However, it looks like (-31/32, 0] maps to 128 (this was true of the original code). I was thinking it might be better to have the values surrounding 0 map to 128, e.g. (-31/64, 31/64]. What do you think?
https://codereview.chromium.org/1643143002/diff/60001/src/core/SkDistanceFiel... File src/core/SkDistanceFieldGen.cpp (right): https://codereview.chromium.org/1643143002/diff/60001/src/core/SkDistanceFiel... src/core/SkDistanceFieldGen.cpp:320: if (dist <= -(distanceMagnitude*(1.0f-1.0f/128.0f))) { On 2016/02/04 12:12:04, Joel.Liang wrote: > On 2016/02/02 18:03:48, bsalomon wrote: > > Can you add a comment that explains this constant? > > Done. And I'd like to give a example here: > > If dist is -3.99999, the result became 0. > The correct result should be 255. > > input: > distanceMagnitude = 4.0 > dist = -3.99999 > result: > (unsigned char)((distanceMagnitude-dist)*128.0/distanceMagnitude) > => (unsigned char)((4.0-(-3.99999))*128.0/4.0) > => (unsigned char)(255.99968) > => 0 > > EDIT: I have changed the way we implement this method to more clear about the > constant. The result should be the same as the original one. Actually, the example I gave here is wrong(The input "dist" not close enough to -4.0). But the bug do existed. The problem was if we pass a value which very close to -4.0 (e.g. -3.9999997f). And then the result in float is close to 256.0 (e.g. 255.999999) But the float type in C could not represent this much significant digits. So the result became 256.0f and then unsigned char 0. https://codereview.chromium.org/1643143002/diff/140001/src/core/SkDistanceFie... File src/core/SkDistanceFieldGen.cpp (right): https://codereview.chromium.org/1643143002/diff/140001/src/core/SkDistanceFie... src/core/SkDistanceFieldGen.cpp:320: // The distance field is constructed as unsigned char values, so that the zero value is at 128, On 2016/02/19 23:36:46, jvanverth1 wrote: > I was looking at this to understand it a little better, so I could possibly > submit it separately a little early (we have a Chrome branch coming up next > week). However, it looks like (-31/32, 0] maps to 128 (this was true of the > original code). I was thinking it might be better to have the values surrounding > 0 map to 128, e.g. (-31/64, 31/64]. What do you think? Yes, to do that we could add SkScalarRoundToInt to the return statement: return (unsigned char)SkScalarRoundToInt(dist / (2 * distanceMagnitude) * 256.0f); And it's safe, because we have truncated the input by multiply 127/128. (It will truncate the result which greater than 255.0)
I have uploaded the patch set 9 to change the distance mapping. Map the range (-1/64, 1/64] to 128 instead of map (-1/32, 0] to 128.
I have rebased the code to the latest version. And also sync the SDF encoding implementation from https://codereview.chromium.org/1726763002/
lgtm, but I'd Jim to have final say.
lgtm too
The CQ bit was checked by joel.liang@arm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1643143002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/180001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by joel.liang@arm.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/1643143002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by joel.liang@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1643143002/#ps220001 (title: "Fix MSVC build error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1643143002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by joel.liang@arm.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/1643143002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I have fixed the Clang & MSVC build errors. Please have a look.
On 2016/03/01 06:18:42, Joel.Liang wrote: > I have fixed the Clang & MSVC build errors. Please have a look. In our tree, you don't need approval to resubmit, especially for build errors. But lgtm.
The CQ bit was checked by joel.liang@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1643143002/#ps240001 (title: "Fix convert double to float")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1643143002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643143002/240001
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. Currently only support even odd fill type. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. Currently only support even odd fill type. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://skia.googlesource.com/skia/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/1757913002/ by robertphillips@google.com. The reason for reverting is: This patch seems to be generating the assert: GrDistanceFieldGenFromVector.cpp:748: fatal error: ""0 && \"Winding number should be zero at the end of a scan line.\""".
Message was sent while issue was closed.
Here are some links to the failures: Mac - https://build.chromium.org/p/client.skia/builders/Test-Mac-Clang-MacMini6.2-G... Ubuntu - https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP... Windows - https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP...
Message was sent while issue was closed.
On 2016/03/02 13:42:10, robertphillips wrote: > Here are some links to the failures: > > Mac - > https://build.chromium.org/p/client.skia/builders/Test-Mac-Clang-MacMini6.2-G... > > Ubuntu - > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP... > > Windows - > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP... It also looks like at least one GM image draws incorrectly: https://gold.skia.org/img/images/ece60acf6f1fa48d28a1695f9f17dcf4.png
Message was sent while issue was closed.
On 2016/03/02 14:23:09, bsalomon wrote: > On 2016/03/02 13:42:10, robertphillips wrote: > > Here are some links to the failures: > > > > Mac - > > > https://build.chromium.org/p/client.skia/builders/Test-Mac-Clang-MacMini6.2-G... > > > > Ubuntu - > > > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP... > > > > Windows - > > > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP... > > It also looks like at least one GM image draws incorrectly: > > https://gold.skia.org/img/images/ece60acf6f1fa48d28a1695f9f17dcf4.png That test is called "nested_flipY_aa"
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. Currently only support even odd fill type. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f ==========
Hi Brian, I just uploaded the Patch Set 14 to fix the GM artifacts and the winding number assertion. Please have a look. But I can't reproduce the even-odd fill type assertion because I don't have the desk_tiger8svg.skp file. And I believe it should be a bug in the Simplify function implementation. I also tested all the SKP files I have.(Mostly downloaded from the code which use for reproduce the PLS rendering issue.) I found that the Simplify never return when I run the desk_chalkboard.skp test. I'm not familiar with the implementation of Simplify. Can someone else have a look at the Simplify function?
joel.liang@arm.com changed reviewers: + caryclark@google.com
Hi Brian, I just uploaded the patch set 15 to fix all the winding number assertion I met. I will commit the current changelist again after all the Simplify bugs have been resolved. (Note: We have this bug http://skbug.com/5131 currently.) Hi Cary, The new patch should fixed all other issues except the Simplify one. You can apply the new patch if you want to turn on some debug switch and try to run all the GM/SKPs test cases. Thanks, Joel Liang
On 2016/03/28 03:28:27, Joel.Liang wrote: > Hi Brian, > > I just uploaded the patch set 15 to fix all the winding number assertion I met. > I will commit the current changelist again after all the Simplify bugs have been > resolved. > (Note: We have this bug http://skbug.com/5131 currently.) > > Hi Cary, > > The new patch should fixed all other issues except the Simplify one. > You can apply the new patch if you want to turn on some debug switch and try to > run all the GM/SKPs test cases. > > Thanks, > Joel Liang Sounds good. I've spent a couple of days looking at the Simplify bugs; It's not an easy fix. I'll continue to work on them when I return from vacation next week.
On 2016/03/29 02:03:08, caryclark wrote: > On 2016/03/28 03:28:27, Joel.Liang wrote: > > Hi Brian, > > > > I just uploaded the patch set 15 to fix all the winding number assertion I > met. > > I will commit the current changelist again after all the Simplify bugs have > been > > resolved. > > (Note: We have this bug http://skbug.com/5131 currently.) > > > > Hi Cary, > > > > The new patch should fixed all other issues except the Simplify one. > > You can apply the new patch if you want to turn on some debug switch and try > to > > run all the GM/SKPs test cases. > > > > Thanks, > > Joel Liang > > Sounds good. I've spent a couple of days looking at the Simplify bugs; > It's not an easy fix. I'll continue to work on them when I return from > vacation next week. Cary, I have rebased my code and checked the Simplify result in the onCanDrawPath just like I mentioned earlier in email. But it will hit an assertion when I run the gm test with desk_carsvg.skp file. ../../../../src/pathops/SkOpAngle.cpp:165: fatal error: "assert(lrOpposite != trOpposite)" Can you check and fix this issue? Just apply my patch and run: ./platform_tools/android/bin/android_run_skia dm --config gpu --resourcePath /sdcard/skia_resources --skps /sdcard/skp_path --match desk_carsvg BTW: Some gm tests will failed to run on Android because of this CL https://codereview.chromium.org/2232433002/ (I have added a comment to this thread.) Thanks, Joel Liang
Could you rebase this again? When I apply the patch on ToT, I get: /c/puregit$git cl patch 1653143002 error: runtime/bin/gen_snapshot.cc: does not exist in index Failed to apply the patch
After I walked away from my desk, I realized I must have mistyped the number. My apologies for the noise
The CQ bit was checked by joel.liang@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1643143002/#ps320001 (title: "Check Simplify result for SDF renderer")
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
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
The CQ bit was checked by joel.liang@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1643143002/#ps340001 (title: "fix clang warning")
The CQ bit was unchecked by joel.liang@arm.com
The CQ bit was checked by joel.liang@arm.com
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 ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://chromiumcodereview.appspot.com/2435753002/ by benjaminwagner@google.com. The reason for reverting is: Multiple assertion failures on multiple platforms: ../../../src/gpu/GrDistanceFieldGenFromVector.cpp:806: fatal error: "assert(((col != width - 1) || (windingNumber == 0)) && "Winding number should be zero at the end of a scan line.")" https://luci-milo.appspot.com/swarming/task/31f5353caf8cc410 https://luci-milo.appspot.com/swarming/task/31f567789cbcec10 c:\b\work\skia\src\pathops\skopangle.cpp:165: fatal error: "assert(lrOpposite != trOpposite)" https://luci-milo.appspot.com/swarming/task/31f570d74f750310 .
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 ==========
The CQ bit was checked by joel.liang@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1643143002/#ps360001 (title: "Tune tolerance and use SkPath::contains")
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
Try jobs failed on following builders: Test-Android-Clang-Nexus5-GPU-Adreno330-arm-Release-GN_Android-Trybot on master.client.skia.android (JOB_FAILED, http://build.chromium.org/p/client.skia.android/builders/Test-Android-Clang-N...)
On 2016/10/25 at 09:32:50, commit-bot wrote: > Try jobs failed on following builders: > Test-Android-Clang-Nexus5-GPU-Adreno330-arm-Release-GN_Android-Trybot on master.client.skia.android (JOB_FAILED, http://build.chromium.org/p/client.skia.android/builders/Test-Android-Clang-N...) This failing bot is flaky, and is probably not your fault. Gonna run it again.
The CQ bit was checked by joel.liang@arm.com
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 ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268
Message was sent while issue was closed.
reed@google.com changed reviewers: + reed@google.com
Message was sent while issue was closed.
I think this caused a LayoutTest change/failure in the DEPS roll... https://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel...
Message was sent while issue was closed.
On 2016/10/25 15:43:25, reed1 wrote: > I think this caused a LayoutTest change/failure in the DEPS roll... > > https://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel... It also seems to have introduced a crash in Chrome. https://bugs.chromium.org/p/chromium/issues/detail?id=659611
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:360001) has been created in https://codereview.chromium.org/2447403002/ by bsalomon@google.com. The reason for reverting is: Chrome crash.
Message was sent while issue was closed.
When this relands can you add a macro that could be used to turn this off without reverting? E.g. SK_USE_LEGACY_DISTANCE_FIELDS. That way we can optionally turn this off in all Skia builds or just Chrome until all the issues are sorted out.
Message was sent while issue was closed.
On 2016/10/26 14:39:29, bsalomon wrote: > On 2016/10/25 15:43:25, reed1 wrote: > > I think this caused a LayoutTest change/failure in the DEPS roll... > > > > > https://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel... > > It also seems to have introduced a crash in Chrome. > https://bugs.chromium.org/p/chromium/issues/detail?id=659611 I do not have permission to view the chrome crash issue page. Can you describe the crash info here or give me the permission to track the issue.
Message was sent while issue was closed.
On 2016/10/26 14:56:37, Joel.Liang wrote: > On 2016/10/26 14:39:29, bsalomon wrote: > > On 2016/10/25 15:43:25, reed1 wrote: > > > I think this caused a LayoutTest change/failure in the DEPS roll... > > > > > > > > > https://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel... > > > > It also seems to have introduced a crash in Chrome. > > https://bugs.chromium.org/p/chromium/issues/detail?id=659611 > > I do not have permission to view the chrome crash issue page. > Can you describe the crash info here or give me the permission to track the > issue. Here's a bit of the stack: 0x00007ff946789ba2 (chrome_child.dll -sktypes.h:667 ) SkAutoSMalloc<1024>::~SkAutoSMalloc<1024>() 0x00007ff947022c9e (chrome_child.dll -grdistancefieldgenfromvector.cpp:826 ) GrGenerateDistanceFieldFromPath(unsigned char *,SkPath const &,SkMatrix const &,int,int,unsigned __int64) 0x00007ff946fcbe44 (chrome_child.dll -grbatchfontcache.cpp:284 ) get_packed_glyph_df_image 0x00007ff946fcbaf8 (chrome_child.dll -grbatchfontcache.cpp:361 ) GrBatchTextStrike::addGlyphToAtlas(GrDrawBatch::Target It's crashing with an access violation exception.
Message was sent while issue was closed.
egdaniel@google.com changed reviewers: + egdaniel@google.com
Message was sent while issue was closed.
This change also caused some major perf regressions. See https://perf.skia.org/#5764
Message was sent while issue was closed.
On 2016/10/26 17:54:59, egdaniel wrote: > This change also caused some major perf regressions. See > https://perf.skia.org/#5764 How can I do the performance regress test myself?
Message was sent while issue was closed.
On 2016/10/27 05:28:24, Joel.Liang wrote: > On 2016/10/26 17:54:59, egdaniel wrote: > > This change also caused some major perf regressions. See > > https://perf.skia.org/#5764 > > How can I do the performance regress test myself? nanobench --skps <path to skp file> --config gpu --match chalk should do it.
Message was sent while issue was closed.
On 2016/10/26 15:00:24, jvanverth1 wrote: > On 2016/10/26 14:56:37, Joel.Liang wrote: > > On 2016/10/26 14:39:29, bsalomon wrote: > > > On 2016/10/25 15:43:25, reed1 wrote: > > > > I think this caused a LayoutTest change/failure in the DEPS roll... > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel... > > > > > > It also seems to have introduced a crash in Chrome. > > > https://bugs.chromium.org/p/chromium/issues/detail?id=659611 > > > > I do not have permission to view the chrome crash issue page. > > Can you describe the crash info here or give me the permission to track the > > issue. > > Here's a bit of the stack: > > 0x00007ff946789ba2 (chrome_child.dll -sktypes.h:667 > ) SkAutoSMalloc<1024>::~SkAutoSMalloc<1024>() > 0x00007ff947022c9e (chrome_child.dll -grdistancefieldgenfromvector.cpp:826 > ) GrGenerateDistanceFieldFromPath(unsigned char *,SkPath const &,SkMatrix const > &,int,int,unsigned __int64) > 0x00007ff946fcbe44 (chrome_child.dll -grbatchfontcache.cpp:284 > ) get_packed_glyph_df_image > 0x00007ff946fcbaf8 (chrome_child.dll -grbatchfontcache.cpp:361 > ) GrBatchTextStrike::addGlyphToAtlas(GrDrawBatch::Target > > It's crashing with an access violation exception. Is there any SKP file can reproduce this crash? Or can someone record a SKP file for me to reproduce this issue? I think the SkAutoSMalloc class should manage its memory itself. And I have not call sk_free() to free up the memory which returned by SkAutoSMalloc::get() method.
Message was sent while issue was closed.
On 2016/11/02 10:09:48, Joel.Liang wrote: > On 2016/10/26 15:00:24, jvanverth1 wrote: > > On 2016/10/26 14:56:37, Joel.Liang wrote: > > > On 2016/10/26 14:39:29, bsalomon wrote: > > > > On 2016/10/25 15:43:25, reed1 wrote: > > > > > I think this caused a LayoutTest change/failure in the DEPS roll... > > > > > > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel... > > > > > > > > It also seems to have introduced a crash in Chrome. > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=659611 > > > > > > I do not have permission to view the chrome crash issue page. > > > Can you describe the crash info here or give me the permission to track the > > > issue. > > > > Here's a bit of the stack: > > > > 0x00007ff946789ba2 (chrome_child.dll -sktypes.h:667 > > ) SkAutoSMalloc<1024>::~SkAutoSMalloc<1024>() > > 0x00007ff947022c9e (chrome_child.dll -grdistancefieldgenfromvector.cpp:826 > > ) GrGenerateDistanceFieldFromPath(unsigned char *,SkPath const &,SkMatrix > const > > &,int,int,unsigned __int64) > > 0x00007ff946fcbe44 (chrome_child.dll -grbatchfontcache.cpp:284 > > ) get_packed_glyph_df_image > > 0x00007ff946fcbaf8 (chrome_child.dll -grbatchfontcache.cpp:361 > > ) GrBatchTextStrike::addGlyphToAtlas(GrDrawBatch::Target > > > > It's crashing with an access violation exception. > > Is there any SKP file can reproduce this crash? > Or can someone record a SKP file for me to reproduce this issue? > > I think the SkAutoSMalloc class should manage its memory itself. > And I have not call sk_free() to free up the memory which returned by > SkAutoSMalloc::get() method. It's happening on a number of webpages so I'm not sure if we have an exact SKP. You may need to build Chrome with your patch and test it. The simplified URL hotspot appears to be www.youtube.com, though web.whatsup.com, www.netflix.com, and www.facebook.com are also listed near the top.
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 ==========
Hi Cary, I just uploaded the patch set 21 to try to solve these problems: > Added a macro SK_USE_LEGACY_DISTANCE_FIELDS for temporary turn the new algorithm off. > > Adjusted conic to quadratic bezier tolerance to fix the poorly rendered circles issue. > > Removed the Simplify call in onCanDrawPath method to fix the performance issue. > And fallback to legacy algorithm if failed to simplify the path. > > Added a new tolerance for tangent line detection. > > Refined the winding number calculation methods. And passed all the correctness tests and performance tests we have. As the new nanobench can parse and render SVG files. I decided to download some SVG files from public domain to have more tests on it. Then I found some issues related to the Simplify method. I have filed a bug here: http://skbug.com/6041 Please have a look.
On 2016/12/08 10:01:44, Joel.Liang wrote: > Hi Cary, > > I just uploaded the patch set 21 to try to solve these problems: > > Added a macro SK_USE_LEGACY_DISTANCE_FIELDS for temporary turn the new > algorithm off. > > > > Adjusted conic to quadratic bezier tolerance to fix the poorly rendered > circles issue. > > > > Removed the Simplify call in onCanDrawPath method to fix the performance > issue. > > And fallback to legacy algorithm if failed to simplify the path. > > > > Added a new tolerance for tangent line detection. > > > > Refined the winding number calculation methods. > > And passed all the correctness tests and performance tests we have. > As the new nanobench can parse and render SVG files. > I decided to download some SVG files from public domain to have more tests on > it. > > Then I found some issues related to the Simplify method. > I have filed a bug here: http://skbug.com/6041 > Please have a look. Joel, how do you feel about landing this with SK_USE_LEGACY_DISTANCE_FIELDS enabled? That way we can get some compile coverage of it on the bots while you and Cary work out any remaining issues. Then when that's all fixed you can flip the flag. And before you land this you'll also need to add the flag to src/skia/config/SkUserConfig.h in Chromium so it's turned on in Chrome by default (for the moment).
On 2016/12/15 20:41:08, jvanverth1 wrote: > Joel, how do you feel about landing this with SK_USE_LEGACY_DISTANCE_FIELDS > enabled? That way we can get some compile coverage of it on the bots while you > and Cary work out any remaining issues. Then when that's all fixed you can flip > the flag. And before you land this you'll also need to add the flag to > src/skia/config/SkUserConfig.h in Chromium so it's turned on in Chrome by > default (for the moment). I'm happy to do that. And I have submitted a patch to chromium to turn on the SK_USE_LEGACY_DISTANCE_FIELDS flag. https://codereview.chromium.org/2581563004/ I'll land this CL after you reviewed the chromium patch.
The CQ bit was checked by joel.liang@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1643143002/#ps420001 (title: "rebase")
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
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release on skia.primary (JOB_FAILED, no build URL) Build-Ubuntu-Clang-arm64-Debug-Android on skia.primary (JOB_FAILED, no build URL) Build-Ubuntu-Clang-arm64-Debug-Android_FrameworkDefs on skia.primary (JOB_FAILED, no build URL) Build-Ubuntu-Clang-mips64el-Debug-Android on skia.primary (JOB_FAILED, no build URL) Build-Ubuntu-Clang-x86_64-Debug on skia.primary (JOB_FAILED, no build URL) Build-Ubuntu-GCC-x86_64-Debug-NoGPU on skia.primary (JOB_FAILED, no build URL) Build-Ubuntu-GCC-x86_64-Release on skia.primary (JOB_FAILED, no build URL) Build-Win-MSVC-x86-Debug on skia.primary (JOB_FAILED, no build URL) Build-Win-MSVC-x86_64-Release-Vulkan on skia.primary (JOB_FAILED, no build URL) Housekeeper-PerCommit-InfraTests on skia.primary (JOB_FAILED, no build URL)
The CQ bit was checked by joel.liang@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1643143002/#ps440001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1482117894447830,
"parent_rev": "57845a7264ef5183258ec23ffe77c25ec967c755", "commit_rev":
"64b70b096ac20833d9737758a4bd5f2a51078bc4"}
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:440001) has been created in https://codereview.chromium.org/2580373002/ by rmistry@google.com. The reason for reverting is: Seems to have caused lot of test bots to fail. Eg: https://luci-milo.appspot.com/swarming/task/332e3b427135f010.
Message was sent while issue was closed.
On 2016/12/19 12:32:16, rmistry wrote: > A revert of this CL (patchset #23 id:440001) has been created in > https://codereview.chromium.org/2580373002/ by mailto:rmistry@google.com. > > The reason for reverting is: Seems to have caused lot of test bots to fail. > Eg: > https://luci-milo.appspot.com/swarming/task/332e3b427135f010. Jim, I think this should introduced by your CL, but triggered by my CL. In this CL: https://skia-review.googlesource.com/5697 You should use SkScalarFloorToScalar instead of SkScalarFloorToInt at line 316,317 in file GrAADistanceFieldPathRenderer.cpp So you can fix it like this: - SkScalar dx = SkScalarFloorToInt(scaledBounds.fLeft); - SkScalar dy = SkScalarFloorToInt(scaledBounds.fTop); + SkScalar dx = SkScalarFloorToScalar(scaledBounds.fLeft); + SkScalar dy = SkScalarFloorToScalar(scaledBounds.fTop); Please confirm and fix this issue. After that, I'll try to land my CL again.
Message was sent while issue was closed.
On 2016/12/20 06:47:00, Joel.Liang wrote: > On 2016/12/19 12:32:16, rmistry wrote: > > A revert of this CL (patchset #23 id:440001) has been created in > > https://codereview.chromium.org/2580373002/ by mailto:rmistry@google.com. > > > > The reason for reverting is: Seems to have caused lot of test bots to fail. > > Eg: > > https://luci-milo.appspot.com/swarming/task/332e3b427135f010. > > Jim, I think this should introduced by your CL, but triggered by my CL. > In this CL: https://skia-review.googlesource.com/5697 > You should use SkScalarFloorToScalar instead of SkScalarFloorToInt at line > 316,317 in file GrAADistanceFieldPathRenderer.cpp > So you can fix it like this: > - SkScalar dx = SkScalarFloorToInt(scaledBounds.fLeft); > - SkScalar dy = SkScalarFloorToInt(scaledBounds.fTop); > + SkScalar dx = SkScalarFloorToScalar(scaledBounds.fLeft); > + SkScalar dy = SkScalarFloorToScalar(scaledBounds.fTop); > > Please confirm and fix this issue. > After that, I'll try to land my CL again. Sure, makes sense. I'll put that up shortly.
Message was sent while issue was closed.
On 2016/12/20 14:33:51, Jvsquare wrote: > On 2016/12/20 06:47:00, Joel.Liang wrote: > > On 2016/12/19 12:32:16, rmistry wrote: > > > A revert of this CL (patchset #23 id:440001) has been created in > > > https://codereview.chromium.org/2580373002/ by mailto:rmistry@google.com. > > > > > > The reason for reverting is: Seems to have caused lot of test bots to fail. > > > Eg: > > > https://luci-milo.appspot.com/swarming/task/332e3b427135f010. > > > > Jim, I think this should introduced by your CL, but triggered by my CL. > > In this CL: https://skia-review.googlesource.com/5697 > > You should use SkScalarFloorToScalar instead of SkScalarFloorToInt at line > > 316,317 in file GrAADistanceFieldPathRenderer.cpp > > So you can fix it like this: > > - SkScalar dx = SkScalarFloorToInt(scaledBounds.fLeft); > > - SkScalar dy = SkScalarFloorToInt(scaledBounds.fTop); > > + SkScalar dx = SkScalarFloorToScalar(scaledBounds.fLeft); > > + SkScalar dy = SkScalarFloorToScalar(scaledBounds.fTop); > > > > Please confirm and fix this issue. > > After that, I'll try to land my CL again. > > Sure, makes sense. I'll put that up shortly. That change has landed.
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 ==========
The CQ bit was checked by joel.liang@arm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1482287294447530,
"parent_rev": "253b4dd51fab530054bcf28a59341b4bd1622430", "commit_rev":
"6d2f73c364d0d823f14d1ddebc88e0bcbc8f0634"}
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/6d2f73c364d0d823f14d1ddebc88e0bcbc8f0634 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as https://skia.googlesource.com/skia/+/6d2f73c364d0d823f14d1ddebc88e0bcbc8f0634
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:440001) has been created in https://codereview.chromium.org/2597473003/ by rmistry@google.com. The reason for reverting is: Caused test failures again but this time in Win8 and Win10 bots: * https://chromium-swarm.appspot.com/task?id=33384540bf6f3710&refresh=10 * https://chromium-swarm.appspot.com/task?id=3338a3ea9f6fe510&refresh=10.
Message was sent while issue was closed.
On 2016/12/21 12:25:00, rmistry wrote: > A revert of this CL (patchset #23 id:440001) has been created in > https://codereview.chromium.org/2597473003/ by mailto:rmistry@google.com. > > The reason for reverting is: Caused test failures again but this time in Win8 > and Win10 bots: > * https://chromium-swarm.appspot.com/task?id=33384540bf6f3710&refresh=10 > * https://chromium-swarm.appspot.com/task?id=3338a3ea9f6fe510&refresh=10. How can I access to the links which you provided above? This error message popup at the bottom left of the web pages: Http response: 403 {"error": {"message": "1 is not accessible."}} And no useful message shown on that pages.
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/6d2f73c364d0d823f14d1ddebc88e0bcbc8f0634 ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/6d2f73c364d0d823f14d1ddebc88e0bcbc8f0634 ==========
On 2016/12/21 12:25:00, rmistry wrote: > A revert of this CL (patchset #23 id:440001) has been created in > https://codereview.chromium.org/2597473003/ by mailto:rmistry@google.com. > > The reason for reverting is: Caused test failures again but this time in Win8 > and Win10 bots: > * https://chromium-swarm.appspot.com/task?id=33384540bf6f3710&refresh=10 > * https://chromium-swarm.appspot.com/task?id=3338a3ea9f6fe510&refresh=10. I think it's a SkGlyph bounding issue. I suspect the path which returned from SkGlyphCache::findPath() may not bound in SkGlyph bounds when running on Windows. I've filed a bug here http://skbug.com/6074
On 2016/12/21 14:12:31, Joel.Liang wrote: > On 2016/12/21 12:25:00, rmistry wrote: > > A revert of this CL (patchset #23 id:440001) has been created in > > https://codereview.chromium.org/2597473003/ by mailto:rmistry@google.com. > > > > The reason for reverting is: Caused test failures again but this time in Win8 > > and Win10 bots: > > * https://chromium-swarm.appspot.com/task?id=33384540bf6f3710&refresh=10 > > * https://chromium-swarm.appspot.com/task?id=3338a3ea9f6fe510&refresh=10. > > How can I access to the links which you provided above? > This error message popup at the bottom left of the web pages: > Http response: 403 {"error": {"message": "1 is not accessible."}} > > And no useful message shown on that pages. Sorry, I did not see your msg earlier (was not CC'ed on the CL). These corresponding milo links should be accessible: * https://luci-milo.appspot.com/swarming/task/33384540bf6f3710 * https://luci-milo.appspot.com/swarming/task/3338a3ea9f6fe510
Not lgtm. I believe the issue is that we are generating the glyph bounds from the rasterized glyph rather than the path, which may be a little bit of work to fix. I'll take a look at it. However, this is only coming up because the new distance field code is being used, when I thought you were going to disable it for now. I have a suggestion where to disable it in the comments below. The rest of the comments are minor things I missed earlier -- many apologies. https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.cpp:1: /* Same question about src/core vs. src/gpu. https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.h (right): https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.h:1: /* Is there a reason this file is in src/gpu and not in src/core as SkDistanceFieldGenFromVector.h? https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.h:11: #include "SkDistanceFieldGen.h" This include could be in GrDistanceFieldGenFromVector.cpp instead. https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.h:13: #include "SkMatrix.h" This could be replaced by class Matrix; and put the include in GrDistanceFieldGenFromVector.h. https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.h:14: Add #define SK_USE_LEGACY_DISTANCE_FIELDS here? https://codereview.chromium.org/1643143002/diff/460001/src/gpu/ops/GrAADistan... File src/gpu/ops/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/460001/src/gpu/ops/GrAADistan... src/gpu/ops/GrAADistanceFieldPathRenderer.cpp:3: * Copyright 2016 ARM Ltd. I would feel more comfortable checking with our program manager on this additional copyright notice. Adding hcm@.
https://codereview.chromium.org/1643143002/diff/460001/src/gpu/ops/GrAADistan... File src/gpu/ops/GrAADistanceFieldPathRenderer.cpp (right): https://codereview.chromium.org/1643143002/diff/460001/src/gpu/ops/GrAADistan... src/gpu/ops/GrAADistanceFieldPathRenderer.cpp:3: * Copyright 2016 ARM Ltd. On 2017/01/04 21:12:27, jvanverth1 wrote: > I would feel more comfortable checking with our program manager on this > additional copyright notice. Adding hcm@. This is fine, sorry about the confusion.
I've done some change. Please have a look. https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.cpp (right): https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.cpp:1: /* On 2017/01/04 21:12:27, jvanverth1 wrote: > Same question about src/core vs. src/gpu. It's because we used the GrPathUtils::convertCubicToQuads() function to convert Cubic path. And I think Signed Distance Field should only available in GPU related build. Please see here for more detail: https://codereview.chromium.org/1643143002/#msg19 https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... File src/gpu/GrDistanceFieldGenFromVector.h (right): https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.h:1: /* On 2017/01/04 21:12:27, jvanverth1 wrote: > Is there a reason this file is in src/gpu and not in src/core as > SkDistanceFieldGenFromVector.h? It's because we used the GrPathUtils::convertCubicToQuads() function to convert Cubic path. And I think Signed Distance Field should only available in GPU related build. Please see here for more detail: https://codereview.chromium.org/1643143002/#msg19 https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.h:11: #include "SkDistanceFieldGen.h" On 2017/01/04 21:12:27, jvanverth1 wrote: > This include could be in GrDistanceFieldGenFromVector.cpp instead. Done. https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.h:13: #include "SkMatrix.h" On 2017/01/04 21:12:27, jvanverth1 wrote: > This could be replaced by > > class Matrix; > > and put the include in GrDistanceFieldGenFromVector.h. Done. https://codereview.chromium.org/1643143002/diff/460001/src/gpu/GrDistanceFiel... src/gpu/GrDistanceFieldGenFromVector.h:14: On 2017/01/04 21:12:27, jvanverth1 wrote: > Add #define SK_USE_LEGACY_DISTANCE_FIELDS here? Done. And I have added an #ifndef directive to surround it to avoid redefinition with the SkUserConfig.h file in Chromium.
Sorry about the delay -- we've been distracted by some winter weather here. lgtm
The CQ bit was checked by joel.liang@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1643143002/#ps480001 (title: "Temporary use legacy distance field")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 480001, "attempt_start_ts": 1484014285034060,
"parent_rev": "e57194f74d125c7b0ba767ae5357b97d102792ef", "commit_rev":
"8cbb4246e58c97e2ac51087d2708795b3b59f5e9"}
Message was sent while issue was closed.
Description was changed from ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/6d2f73c364d0d823f14d1ddebc88e0bcbc8f0634 ========== to ========== Generate Signed Distance Field directly from vector path Add SkGenerateDistanceFieldFromPath API to generate signed distance field directly from SkPath. 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/+/4de97a64e8829323a7070b623411d9f9ddb0cd0f Committed: https://skia.googlesource.com/skia/+/e8f0a7b986f1e5583c9bc162efcdd92fd6430549 Committed: https://skia.googlesource.com/skia/+/67c7c81a82b6351e9fbbf235084d7120162d9268 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/64b70b096ac20833d9737758a4bd5f2a51078bc4 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/6d2f73c364d0d823f14d1ddebc88e0bcbc8f0634 Review-Url: https://codereview.chromium.org/1643143002 Committed: https://skia.googlesource.com/skia/+/8cbb4246e58c97e2ac51087d2708795b3b59f5e9 ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001) as https://skia.googlesource.com/skia/+/8cbb4246e58c97e2ac51087d2708795b3b59f5e9 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
