Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2146103004:
SkPDF: re-work SkPDFUtils::FloatToDecimal (Closed)
|
Created:
4 years, 5 months ago by hal.canary Modified:
4 years, 5 months ago Reviewers:
tomhudson CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: re-work SkPDFUtils::FloatToDecimal
* do a lot less floating-point math by converting to
an integer as early as possible [faster].
* round rather than truncate.
* use 8 significant digits rather than 9 when possible.
* remove trailing zeros in fractions.
before:
0.12 ! PDFScalar nonrendering
after:
0.07 ! PDFScalar nonrendering
Accuracy guaranteed by existing unit test.
Example diffs:
-/Shading <</Function <</C0 [.321568638 .333333343 .321568638]
+/Shading <</Function <</C0 [.32156864 .33333334 .32156864]
-/C1 [.258823543 .270588248 .258823543]
+/C1 [.25882354 .27058825 .25882354]
-1 0 0 -1 20 120.394500 Tm
+1 0 0 -1 20 120.394501 Tm
-1 0 0 -1 20 184.789001 Tm
+1 0 0 -1 20 184.789 Tm
-291.503997 0 l
+291.504 0 l
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2146103004
Committed: https://skia.googlesource.com/skia/+/56586b6f3dc824431958946ac8c27f0c0b564a3d
Patch Set 1 #Patch Set 2 : 2016-07-14 (Thursday) 15:50:08 EDT #
Total comments: 14
Patch Set 3 : 2016-07-16 (Saturday) 22:33:13 EDT #Patch Set 4 : 2016-07-25 (Monday) 15:22:36 EDT #Messages
Total messages: 39 (32 generated)
Description was changed from
==========
SkPDF: re-work t SkPDFUtils::FloatToDecimal
* do a lot less floating-point math by converting to
an integer as early as possible [faster].
* round rather than truncate.
* use 8 significant digits rather than 9 when possible.
* remove trailing zeros in fractions.
before:
0.12 ! PDFScalar nonrendering
after:
0.07 ! PDFScalar nonrendering
Accuracy guaranteed by existing unit test.
Example diffs:
-/Shading <</Function <</C0 [.321568638 .333333343 .321568638]
+/Shading <</Function <</C0 [.32156864 .33333334 .32156864]
-/C1 [.258823543 .270588248 .258823543]
+/C1 [.25882354 .27058825 .25882354]
-1 0 0 -1 20 120.394500 Tm
+1 0 0 -1 20 120.394501 Tm
-1 0 0 -1 20 184.789001 Tm
+1 0 0 -1 20 184.789 Tm
-291.503997 0 l
+291.504 0 l
BUG=skia:
==========
to
==========
SkPDF: re-work t SkPDFUtils::FloatToDecimal
* do a lot less floating-point math by converting to
an integer as early as possible [faster].
* round rather than truncate.
* use 8 significant digits rather than 9 when possible.
* remove trailing zeros in fractions.
before:
0.12 ! PDFScalar nonrendering
after:
0.07 ! PDFScalar nonrendering
Accuracy guaranteed by existing unit test.
Example diffs:
-/Shading <</Function <</C0 [.321568638 .333333343 .321568638]
+/Shading <</Function <</C0 [.32156864 .33333334 .32156864]
-/C1 [.258823543 .270588248 .258823543]
+/C1 [.25882354 .27058825 .25882354]
-1 0 0 -1 20 120.394500 Tm
+1 0 0 -1 20 120.394501 Tm
-1 0 0 -1 20 184.789001 Tm
+1 0 0 -1 20 184.789 Tm
-291.503997 0 l
+291.504 0 l
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2146103004
==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
halcanary@google.com changed reviewers: + tomhudson@google.com
PTAL
Description was changed from
==========
SkPDF: re-work t SkPDFUtils::FloatToDecimal
* do a lot less floating-point math by converting to
an integer as early as possible [faster].
* round rather than truncate.
* use 8 significant digits rather than 9 when possible.
* remove trailing zeros in fractions.
before:
0.12 ! PDFScalar nonrendering
after:
0.07 ! PDFScalar nonrendering
Accuracy guaranteed by existing unit test.
Example diffs:
-/Shading <</Function <</C0 [.321568638 .333333343 .321568638]
+/Shading <</Function <</C0 [.32156864 .33333334 .32156864]
-/C1 [.258823543 .270588248 .258823543]
+/C1 [.25882354 .27058825 .25882354]
-1 0 0 -1 20 120.394500 Tm
+1 0 0 -1 20 120.394501 Tm
-1 0 0 -1 20 184.789001 Tm
+1 0 0 -1 20 184.789 Tm
-291.503997 0 l
+291.504 0 l
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2146103004
==========
to
==========
SkPDF: re-work SkPDFUtils::FloatToDecimal
* do a lot less floating-point math by converting to
an integer as early as possible [faster].
* round rather than truncate.
* use 8 significant digits rather than 9 when possible.
* remove trailing zeros in fractions.
before:
0.12 ! PDFScalar nonrendering
after:
0.07 ! PDFScalar nonrendering
Accuracy guaranteed by existing unit test.
Example diffs:
-/Shading <</Function <</C0 [.321568638 .333333343 .321568638]
+/Shading <</Function <</C0 [.32156864 .33333334 .32156864]
-/C1 [.258823543 .270588248 .258823543]
+/C1 [.25882354 .27058825 .25882354]
-1 0 0 -1 20 120.394500 Tm
+1 0 0 -1 20 120.394501 Tm
-1 0 0 -1 20 184.789001 Tm
+1 0 0 -1 20 184.789 Tm
-291.503997 0 l
+291.504 0 l
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2146103004
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm out for the next week, so you'll want another reviewer if you want to land it soon; these are my notes so far. https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp File src/pdf/SkPDFUtils.cpp (right): https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:261: // Return pow(10.0, e), optimized for common cases. Huh, this is really faster? https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:345: int binaryExponents; Bikeshed: Why is this name plural? https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:346: (void)frexp(value, &binaryExponents); std::frexp()? Or is that not our style? https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:347: static const double kLog2 = 0.3010299956639812; // log10(2.0); constexpr? https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:348: int decimalExponents = (int)floor(kLog2 * binaryExponents); Bikeshed as above https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:350: double power = pow10(-decShift); (OK, convinced myself that in the common case pow10() really will be getting a positive argument) https://codereview.chromium.org/2146103004/diff/20001/tests/PDFPrimitivesTest... File tests/PDFPrimitivesTest.cpp (right): https://codereview.chromium.org/2146103004/diff/20001/tests/PDFPrimitivesTest... tests/PDFPrimitivesTest.cpp:213: ASSERT_EMIT_EQ(reporter, smallestScalar, ".0000152587891"); One digit change? How did this ever work? Or is there something subtle here that isn't obvious from reading the code?
https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp File src/pdf/SkPDFUtils.cpp (right): https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:261: // Return pow(10.0, e), optimized for common cases. On 2016/07/15 20:45:41, tomhudson wrote: > Huh, this is really faster? yep. https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:345: int binaryExponents; On 2016/07/15 20:45:41, tomhudson wrote: > Bikeshed: Why is this name plural? Done. https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:346: (void)frexp(value, &binaryExponents); On 2016/07/15 20:45:41, tomhudson wrote: > std::frexp()? Or is that not our style? We do it both ways. the C++ish way seems to be gaining ground. I'm changing it to std:: https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:347: static const double kLog2 = 0.3010299956639812; // log10(2.0); On 2016/07/15 20:45:41, tomhudson wrote: > constexpr? std::log10 isn't constexpr. https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:348: int decimalExponents = (int)floor(kLog2 * binaryExponents); On 2016/07/15 20:45:41, tomhudson wrote: > Bikeshed as above Done. https://codereview.chromium.org/2146103004/diff/20001/src/pdf/SkPDFUtils.cpp#... src/pdf/SkPDFUtils.cpp:350: double power = pow10(-decShift); On 2016/07/15 20:45:41, tomhudson wrote: > (OK, convinced myself that in the common case pow10() really will be getting a > positive argument) Done. https://codereview.chromium.org/2146103004/diff/20001/tests/PDFPrimitivesTest... File tests/PDFPrimitivesTest.cpp (right): https://codereview.chromium.org/2146103004/diff/20001/tests/PDFPrimitivesTest... tests/PDFPrimitivesTest.cpp:213: ASSERT_EMIT_EQ(reporter, smallestScalar, ".0000152587891"); Both values round back to the original float value, which is somewhere in between them: Before: .0000152587890 After: .0000152587891 Exact: .0000152587890625 assert(1.0f / 65536 == (float)atof(".0000152587890")); assert(1.0f / 65536 == (float)atof(".0000152587891"));
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by halcanary@google.com
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:120001) has been deleted
Tom, want to look at this again?
lgtm
The CQ bit was checked by halcanary@google.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
==========
SkPDF: re-work SkPDFUtils::FloatToDecimal
* do a lot less floating-point math by converting to
an integer as early as possible [faster].
* round rather than truncate.
* use 8 significant digits rather than 9 when possible.
* remove trailing zeros in fractions.
before:
0.12 ! PDFScalar nonrendering
after:
0.07 ! PDFScalar nonrendering
Accuracy guaranteed by existing unit test.
Example diffs:
-/Shading <</Function <</C0 [.321568638 .333333343 .321568638]
+/Shading <</Function <</C0 [.32156864 .33333334 .32156864]
-/C1 [.258823543 .270588248 .258823543]
+/C1 [.25882354 .27058825 .25882354]
-1 0 0 -1 20 120.394500 Tm
+1 0 0 -1 20 120.394501 Tm
-1 0 0 -1 20 184.789001 Tm
+1 0 0 -1 20 184.789 Tm
-291.503997 0 l
+291.504 0 l
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2146103004
==========
to
==========
SkPDF: re-work SkPDFUtils::FloatToDecimal
* do a lot less floating-point math by converting to
an integer as early as possible [faster].
* round rather than truncate.
* use 8 significant digits rather than 9 when possible.
* remove trailing zeros in fractions.
before:
0.12 ! PDFScalar nonrendering
after:
0.07 ! PDFScalar nonrendering
Accuracy guaranteed by existing unit test.
Example diffs:
-/Shading <</Function <</C0 [.321568638 .333333343 .321568638]
+/Shading <</Function <</C0 [.32156864 .33333334 .32156864]
-/C1 [.258823543 .270588248 .258823543]
+/C1 [.25882354 .27058825 .25882354]
-1 0 0 -1 20 120.394500 Tm
+1 0 0 -1 20 120.394501 Tm
-1 0 0 -1 20 184.789001 Tm
+1 0 0 -1 20 184.789 Tm
-291.503997 0 l
+291.504 0 l
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2146103004
Committed:
https://skia.googlesource.com/skia/+/56586b6f3dc824431958946ac8c27f0c0b564a3d
==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://skia.googlesource.com/skia/+/56586b6f3dc824431958946ac8c27f0c0b564a3d |
