Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1720863003:
SkPDF: fix scalar serialization (Closed)
|
Created:
4 years, 10 months ago by hal.canary Modified:
4 years, 10 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: fix scalar serialization
BUG=skia:4868
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1720863003
Committed: https://skia.googlesource.com/skia/+/8e9f5e39d774198a5a5d9345bc9f863e855c593b
Patch Set 1 #
Total comments: 12
Patch Set 2 : 2016-02-22 (Monday) 18:29:59 EST #Patch Set 3 : refactor, clean #
Total comments: 3
Patch Set 4 : 2016-02-24 (Wednesday) 16:44:08 EST #
Messages
Total messages: 38 (20 generated)
Description was changed from ========== SkPDF: fix scalar serialization BUG=skia:4868 ========== to ========== SkPDF: fix scalar serialization BUG=skia:4868 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
halcanary@google.com changed reviewers: + tomhudson@google.com
Read the spec (<http://goo.gl/0SCswJ>, §3.2.2 and §C.1) and tell me if you approve.
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/patch-status/1720863003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720863003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
I'm not sure I understand either the claim in the bug or the spec, and I
certainly don't understand this CL yet. Early thoughts:
From C.1:
real ±32,767 Largest and smallest real values (approximate).
±1/65,536 Nonzero real values closest to 0 (approximate); equal to ±10−38.
Values closer than these are automatically converted to 0.
5 Number of significant decimal digits of precision in fractional
part (approximate).
The previous support for LARGE_PDF_SCALARS doesn't seem consistent with that.
This really seems like it's set up for a 32b fixed-point model without any other
subtleties?
Because of the spec, the problematic behavior which motivated the bug seems to
be WAI, even if not great?
Your CL seems to be doing more than a minimal response to the bug. Is the
special treatment of infinity just because we were ignoring it before and this
seems "more reasonable"? I'm not spotting a discussion of infinity in the spec.
https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp
File src/pdf/SkPDFUtils.cpp (right):
https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc...
src/pdf/SkPDFUtils.cpp:283: const size_t maxSignificantDigits = 9; // any less
loses precision
With a 32b signed fixed, can't we have 10 significant digits, 5 on each side of
the decimal point?
https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc...
src/pdf/SkPDFUtils.cpp:284: SkASSERT(intPart >= 0.0 && fracPart >= 0.0);
What happened to negative numbers? std::modf() "[D]ecomposes given floating
point value x into integral and fractional parts, each having the same type and
sign as x."
https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc...
src/pdf/SkPDFUtils.cpp:286: stream->writeText("0"); // Unsupported number in
PDF
Why is 0 unsupported?
https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc...
src/pdf/SkPDFUtils.cpp:289: if (intPart > 0.0) {
This code looks like it was hard to write; it should not be so hard to read.
https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc...
src/pdf/SkPDFUtils.cpp:290: char reversed[311];
What's with the magic number 311? No idea how you came up with it.
https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc...
src/pdf/SkPDFUtils.cpp:291: size_t reveredIndex = 0;
Why is this index holy or particularly esteemed? (spelling?)
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/patch-status/1720863003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720863003/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-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp File src/pdf/SkPDFUtils.cpp (right): https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc... src/pdf/SkPDFUtils.cpp:283: const size_t maxSignificantDigits = 9; // any less loses precision On 2016/02/22 at 20:13:46, tomhudson wrote: > With a 32b signed fixed, can't we have 10 significant digits, 5 on each side of the decimal point? Ah, but we don't have a fixed-point input. SkScalar is float. https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc... src/pdf/SkPDFUtils.cpp:284: SkASSERT(intPart >= 0.0 && fracPart >= 0.0); On 2016/02/22 at 20:13:46, tomhudson wrote: > What happened to negative numbers? std::modf() "[D]ecomposes given floating point value x into integral and fractional parts, each having the same type and sign as x." I already dealt with negative number. https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc... src/pdf/SkPDFUtils.cpp:286: stream->writeText("0"); // Unsupported number in PDF On 2016/02/22 at 20:13:46, tomhudson wrote: > Why is 0 unsupported? cut-and-paste error. https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc... src/pdf/SkPDFUtils.cpp:290: char reversed[311]; On 2016/02/22 at 20:13:46, tomhudson wrote: > What's with the magic number 311? No idea how you came up with it. that's a holdover from the double code. I did some experimentation and found the correct number for single-precision and then documented why I use that one. https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc... src/pdf/SkPDFUtils.cpp:291: size_t reveredIndex = 0; On 2016/02/22 at 20:13:46, tomhudson wrote: > Why is this index holy or particularly esteemed? (spelling?) done
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/patch-status/1720863003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720863003/40001
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 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/patch-status/1720863003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720863003/60001
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-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720863003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720863003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by 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/patch-status/1720863003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720863003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
Thanks. LGTM https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp File src/pdf/SkPDFUtils.cpp (right): https://codereview.chromium.org/1720863003/diff/1/src/pdf/SkPDFUtils.cpp#newc... src/pdf/SkPDFUtils.cpp:284: SkASSERT(intPart >= 0.0 && fracPart >= 0.0); On 2016/02/22 22:45:38, Hal Canary wrote: > On 2016/02/22 at 20:13:46, tomhudson wrote: > > What happened to negative numbers? std::modf() "[D]ecomposes given floating > point value x into integral and fractional parts, each having the same type and > sign as x." > > I already dealt with negative number. Acknowledged. https://codereview.chromium.org/1720863003/diff/100001/gm/skbug_4868.cpp File gm/skbug_4868.cpp (right): https://codereview.chromium.org/1720863003/diff/100001/gm/skbug_4868.cpp#newc... gm/skbug_4868.cpp:10: DEF_SIMPLE_GM(skbug_4868, canvas, 32, 32) { It's pretty clear that there was misunderstanding in the bug; therefore, the bug # doesn't serve as sufficient documentation of your intent here. What was going wrong, and *why*? Why these particular scales? Why the weird clear() color? https://codereview.chromium.org/1720863003/diff/100001/src/pdf/SkPDFUtils.cpp File src/pdf/SkPDFUtils.cpp (right): https://codereview.chromium.org/1720863003/diff/100001/src/pdf/SkPDFUtils.cpp... src/pdf/SkPDFUtils.cpp:265: /** Write a string into result, includeing a terminating '\0' (for nit: including https://codereview.chromium.org/1720863003/diff/100001/src/pdf/SkPDFUtils.h File src/pdf/SkPDFUtils.h (right): https://codereview.chromium.org/1720863003/diff/100001/src/pdf/SkPDFUtils.h#n... src/pdf/SkPDFUtils.h:65: static const size_t kMaximumFloatDecimalLength = 3 + 9 - FLT_MIN_10_EXP; Thank you.
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com Link to the patchset: https://codereview.chromium.org/1720863003/#ps120001 (title: "2016-02-24 (Wednesday) 16:44:08 EST")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720863003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720863003/120001
Message was sent while issue was closed.
Description was changed from ========== SkPDF: fix scalar serialization BUG=skia:4868 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkPDF: fix scalar serialization BUG=skia:4868 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/8e9f5e39d774198a5a5d9345bc9f863e855c593b ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://skia.googlesource.com/skia/+/8e9f5e39d774198a5a5d9345bc9f863e855c593b |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
