[PDF] Restrict scalars to the range that PDF understands.

[PDF] Restrict scalars to the range that PDF understands.

* Add a config flag to ignore the restrictions
* Apply restriction to both SkPDFScalar and scalars used in content streams.
* +/- 32,767 for the integer part.
* +/1 1/65536 for the fraction part.

Committed: http://code.google.com/p/skia/source/detail?r=882

[reed1, 2011-02-28 13:55:53]

http://www.thbimage.com/pdf/PDF_real_numbers_out_of_range_24080da6_8650_4ff4_...

My read is that modern PS readers support large floats, so perhaps we don't need
this check (assuming we fix SkString to call sprintf).

[Steve VanDeBogart, 2011-02-28 17:56:38]

On 2011/02/28 13:55:53, reed1 wrote:
>
http://www.thbimage.com/pdf/PDF_real_numbers_out_of_range_24080da6_8650_4ff4_...
> 
> My read is that modern PS readers support large floats, so perhaps we don't
need
> this check (assuming we fix SkString to call sprintf).

We are targeting PDF/A compliance, so that link agrees with this CL.

[reed1, 2011-02-28 17:59:35]

Then can we consider a build-time or runtime flag. If we are compliant, but
can't print a set of web pages, I'm not sure we'll be happy.

On Mon, Feb 28, 2011 at 12:56 PM, <vandebo@chromium.org> wrote:

> On 2011/02/28 13:55:53, reed1 wrote:
>
>
>
http://www.thbimage.com/pdf/PDF_real_numbers_out_of_range_24080da6_8650_4ff4_...
>
>  My read is that modern PS readers support large floats, so perhaps we
>>
> don't need
>
>> this check (assuming we fix SkString to call sprintf).
>>
>
> We are targeting PDF/A compliance, so that link agrees with this CL.
>
>
> http://codereview.appspot.com/4240050/
>

[Steve VanDeBogart, 2011-03-01 00:02:41]

http://codereview.appspot.com/4240050/diff/5/src/core/SkString.cpp
File src/core/SkString.cpp (left):

http://codereview.appspot.com/4240050/diff/5/src/core/SkString.cpp#oldcode142
src/core/SkString.cpp:142: return string + SNPRINTF(string,
SkStrAppendScalar_MaxSize, "%g", value);
%g is the same as %.1g according to
http://www.cplusplus.com/reference/clibrary/cstdio/printf/
%.16g seems to behave better.

[reed1, 2011-03-01 17:49:57]

http://codereview.appspot.com/4240050/diff/5/include/config/SkUserConfig.h
File include/config/SkUserConfig.h (right):

http://codereview.appspot.com/4240050/diff/5/include/config/SkUserConfig.h#ne...
include/config/SkUserConfig.h:129: //#define SK_ALLOW_LARGE_PDF_SCALARS
Nice!

http://codereview.appspot.com/4240050/diff/5/src/core/SkString.cpp
File src/core/SkString.cpp (left):

http://codereview.appspot.com/4240050/diff/5/src/core/SkString.cpp#oldcode142
src/core/SkString.cpp:142: return string + SNPRINTF(string,
SkStrAppendScalar_MaxSize, "%g", value);
On 2011/03/01 00:02:41, Steve VanDeBogart wrote:
> %g is the same as %.1g according to
> http://www.cplusplus.com/reference/clibrary/cstdio/printf/
> %.16g seems to behave better.

My understanding is floats have at best 8 significant digits. I have submitted a
change that uses .8g and ups the SkStrAppendScalar_MaxSize to 14 to accomodate
this.

http://codereview.appspot.com/4240050/diff/5/src/pdf/SkPDFTypes.cpp
File src/pdf/SkPDFTypes.cpp (right):

http://codereview.appspot.com/4240050/diff/5/src/pdf/SkPDFTypes.cpp#newcode101
src/pdf/SkPDFTypes.cpp:101: if (fValue > 32767 || fValue < -32767) {
did you need the (int) cast? If so we should fix skia's headers.

[Steve VanDeBogart, 2011-03-01 22:04:55]

http://codereview.appspot.com/4240050/diff/5/src/pdf/SkPDFTypes.cpp
File src/pdf/SkPDFTypes.cpp (right):

http://codereview.appspot.com/4240050/diff/5/src/pdf/SkPDFTypes.cpp#newcode101
src/pdf/SkPDFTypes.cpp:101: if (fValue > 32767 || fValue < -32767) {
On 2011/03/01 17:49:57, reed1 wrote:
> did you need the (int) cast? If so we should fix skia's headers.

Looks like no, I removed the cast.

[Steve VanDeBogart, 2011-03-03 00:06:09]

So there were two problems with the previous patch set, PDF doesn't understand
scientific notation and it didn't account for scalars used in pdf content
streams.  This patch set deals with both of those issues.  Sorry to drag this
out so much.  I realize that the -> float code is kind of ugly.  I'm open to
suggestions.

[reed1, 2011-03-03 12:33:05]

I'm fine with the changes to SkString.[h,cpp]. Perhaps we can check those in
separately (first).

[Steve VanDeBogart, 2011-03-03 18:23:09]

On 2011/03/03 12:33:05, reed1 wrote:
> I'm fine with the changes to SkString.[h,cpp]. Perhaps we can check those in
> separately (first).

Done (r880).

Do you have objections to the rest?

[reed1, 2011-03-03 18:31:25]

No, other than I didn't realize we couldn't use scientific notation inside
pdf. Is there no binary or hexy way to store floats? (just curious).

On Thu, Mar 3, 2011 at 1:23 PM, <vandebo@chromium.org> wrote:

> On 2011/03/03 12:33:05, reed1 wrote:
>
>> I'm fine with the changes to SkString.[h,cpp]. Perhaps we can check
>>
> those in
>
>> separately (first).
>>
>
> Done (r880).
>
> Do you have objections to the rest?
>
>
>
> http://codereview.appspot.com/4240050/
>

[Steve VanDeBogart, 2011-03-03 18:42:56]

On 2011/03/03 18:31:25, reed1 wrote:
> No, other than I didn't realize we couldn't use scientific notation inside
> pdf. Is there no binary or hexy way to store floats? (just curious).

Nope, all the pdf (1.4) spec says on the topic is: "Real objects approximate
mathematical real numbers, but with limited range and precision; they are
typically represented in fixed-point, rather than floating-point, form. The
range and precision of numbers are limited by the internal representations...
...
Note: PDF does not support the PostScript syntax for numbers with nondecimal
radices (such as 16#FFFE) or in exponential format (such as 6.02E23)."

[reed1, 2011-03-03 18:46:12]

OK. I tried making some PDFs on the mac with fractional values, and I just
see hex looking data, with no decimal numbers. Not sure what that means.

On Thu, Mar 3, 2011 at 1:42 PM, <vandebo@chromium.org> wrote:

> On 2011/03/03 18:31:25, reed1 wrote:
>
>> No, other than I didn't realize we couldn't use scientific notation
>>
> inside
>
>> pdf. Is there no binary or hexy way to store floats? (just curious).
>>
>
> Nope, all the pdf (1.4) spec says on the topic is: "Real objects
> approximate mathematical real numbers, but with limited range and
> precision; they are typically represented in fixed-point, rather than
> floating-point, form. The range and precision of numbers are limited by
> the internal representations...
> ...
> Note: PDF does not support the PostScript syntax for numbers with
> nondecimal radices (such as 16#FFFE) or in exponential format (such as
> 6.02E23)."
>
>
> http://codereview.appspot.com/4240050/
>
> --
> You received this message because you are subscribed to the Google Groups
> "skia-review" group.
> To post to this group, send email to skia-review@googlegroups.com.
> To unsubscribe from this group, send email to
> skia-review+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/skia-review?hl=en.
>
>

[Steve VanDeBogart, 2011-03-03 18:49:43]

Happy to look at the output.

On Thu, Mar 3, 2011 at 10:45 AM, Mike Reed <reed@google.com> wrote:

> OK. I tried making some PDFs on the mac with fractional values, and I just
> see hex looking data, with no decimal numbers. Not sure what that means.
>
> On Thu, Mar 3, 2011 at 1:42 PM, <vandebo@chromium.org> wrote:
>
>> On 2011/03/03 18:31:25, reed1 wrote:
>>
>>> No, other than I didn't realize we couldn't use scientific notation
>>>
>> inside
>>
>>> pdf. Is there no binary or hexy way to store floats? (just curious).
>>>
>>
>> Nope, all the pdf (1.4) spec says on the topic is: "Real objects
>> approximate mathematical real numbers, but with limited range and
>> precision; they are typically represented in fixed-point, rather than
>> floating-point, form. The range and precision of numbers are limited by
>> the internal representations...
>> ...
>> Note: PDF does not support the PostScript syntax for numbers with
>> nondecimal radices (such as 16#FFFE) or in exponential format (such as
>> 6.02E23)."
>>
>>
>> http://codereview.appspot.com/4240050/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "skia-review" group.
>> To post to this group, send email to skia-review@googlegroups.com.
>> To unsubscribe from this group, send email to
>> skia-review+unsubscribe@googlegroups.com.
>> For more options, visit this group at
>> http://groups.google.com/group/skia-review?hl=en.
>>
>>
>

[Chris Guillory, 2011-03-03 19:19:20]

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp
File src/pdf/SkPDFTypes.cpp (right):

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode118
src/pdf/SkPDFTypes.cpp:118: #endif  // SK_SCALAR_IS_FIXED
Can we make this an #else with "SK_SCALAR_IS_FLOAT" code to follow? Or to we not
prefer nested #if declarations.

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode120
src/pdf/SkPDFTypes.cpp:120: #if !defined(SK_ALLOW_LARGE_PDF_SCALARS)
Nit/Optional: Place defined(SK_ALLOW_LARGE_PDF_SCALARS) code first?

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode128
src/pdf/SkPDFTypes.cpp:128: string->append(buffer, end - buffer);
Should we have SkString::appendFixed?

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode130
src/pdf/SkPDFTypes.cpp:130: #endif  // !SK_ALLOW_LARGE_PDF_SCALARS
Also can we make this an else with "SK_ALLOW_LARGE_PDF_SCALARS" code to follow.

[Steve VanDeBogart, 2011-03-03 19:35:36]

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp
File src/pdf/SkPDFTypes.cpp (right):

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode118
src/pdf/SkPDFTypes.cpp:118: #endif  // SK_SCALAR_IS_FIXED
On 2011/03/03 19:19:20, Chris Guillory wrote:
> Can we make this an #else with "SK_SCALAR_IS_FLOAT" code to follow? Or to we
not
> prefer nested #if declarations.

Initially I had it with nested #ifs, but found this formulation easier to read
and understand.

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode120
src/pdf/SkPDFTypes.cpp:120: #if !defined(SK_ALLOW_LARGE_PDF_SCALARS)
On 2011/03/03 19:19:20, Chris Guillory wrote:
> Nit/Optional: Place defined(SK_ALLOW_LARGE_PDF_SCALARS) code first?

Large pdf scalars will be off by default.  Do you still think I should swap
them?

http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode128
src/pdf/SkPDFTypes.cpp:128: string->append(buffer, end - buffer);
On 2011/03/03 19:19:20, Chris Guillory wrote:
> Should we have SkString::appendFixed?

The string API is already pretty big.  Mike, what do you think, is it worthwhile
to add a family of Fixed methods?

[Chris Guillory, 2011-03-03 20:17:12]

On 2011/03/03 19:35:36, Steve VanDeBogart wrote:
> http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp
> File src/pdf/SkPDFTypes.cpp (right):
> 
> http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode118
> src/pdf/SkPDFTypes.cpp:118: #endif  // SK_SCALAR_IS_FIXED
> On 2011/03/03 19:19:20, Chris Guillory wrote:
> > Can we make this an #else with "SK_SCALAR_IS_FLOAT" code to follow? Or to we
> not
> > prefer nested #if declarations.
> 
> Initially I had it with nested #ifs, but found this formulation easier to read
> and understand.
I support its easier to understand once you know that either SK_SCALAR_IS_FIXED
or SK_SCALAR_IS_FLOAT will be defined.
> 
> http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode120
> src/pdf/SkPDFTypes.cpp:120: #if !defined(SK_ALLOW_LARGE_PDF_SCALARS)
> On 2011/03/03 19:19:20, Chris Guillory wrote:
> > Nit/Optional: Place defined(SK_ALLOW_LARGE_PDF_SCALARS) code first?
> 
> Large pdf scalars will be off by default.  Do you still think I should swap
> them?
Not a big deal. Just had to think twice.
> 
> http://codereview.appspot.com/4240050/diff/6/src/pdf/SkPDFTypes.cpp#newcode128
> src/pdf/SkPDFTypes.cpp:128: string->append(buffer, end - buffer);
> On 2011/03/03 19:19:20, Chris Guillory wrote:
> > Should we have SkString::appendFixed?
> 
> The string API is already pretty big.  Mike, what do you think, is it
worthwhile
> to add a family of Fixed methods?

[reed1, 2011-03-03 20:37:24]

I vote to not add more APIs to SkString just yet.

[Chris Guillory, 2011-03-03 21:13:27]

LGTM.