SkPDF: Optionally output PDF/A-2b archive format.

SkPDF: Optionally output PDF/A-2b  archive format.

Note: this format does not yet pass validation tests.

Add skia_pdf_generate_pdfa GYP flag.  Default to off for now.
PDF/A files are not reproducable, so they make correctness
testing harder.

Turn the Metadata struct into te SkPDFMetadata struct.  This
splits out a lot of functionality around both kinds of metadata.

When PDF/A is used, add an ID entry to the trailer.

Add SkPDFObjNumMap::addObjectRecursively.

Test with

    GYP_DEFINES=skia_pdf_generate_pdfa=1 bin/sync-and-gyp
    ninja -C out/Release dm
    out/Release/dm --config pdf --src skp gm -w /tmp/dm

With skia_pdf_generate_pdfa=0, all PDFs generated from GMs and
SKPs are identical.  With skia_pdf_generate_pdfa=1, all PDFs
generated from GMs and SKPs render identically in Pdfium.

BUG=skia:3110
Committed: https://skia.googlesource.com/skia/+/34422610ac22adceeabb66023120f27b96cae953

[hal.canary, 2015-10-08 19:39:31]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-08 19:39:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394263003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394263003/1

[commit-bot: I haz the power, 2015-10-08 19:46:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-08 19:46:50]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2015-10-08 21:50:57]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-08 21:51:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394263003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394263003/20001

[hal.canary, 2015-10-08 21:51:28]

halcanary@google.com changed reviewers:
+ tomhudson@google.com

[hal.canary, 2015-10-08 21:51:29]

PTAL.  I'd like to land this before I go to NYC.

[commit-bot: I haz the power, 2015-10-08 21:57:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-08 21:57:39]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2015-10-09 15:31:37]

Looking good

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF.cpp
File src/doc/SkDocument_PDF.cpp (right):

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:42: trailerDict.insertObjRef("Info", info);
You made info optional?

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:45: trailerDict.insertObject("ID", id);
Both info and id are noted "take ownership", but for one you insert a ref and
the other you insert the object. Consistent?
(Confession: I haven't got your memory model internalized right now, although
you've explained it very well in the past. No mention at
https://skia.org/dev/design/pdftheory?)

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:197: id.reset(SkPDFMetadata::CreatePdfId(uuid,
uuid));
CreatePdfId(uuid, uuid) calls the first one "doc" and the second one "instance".
If you're passing the same UUID for both parameters, it's really not clear how
they're being used.

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:205: "sRGB IEC61966-2.1");
I thought we don't actually support sRGB yet (and Mike doesn't want to invest
headcount in it in the short-term?)

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:205: "sRGB IEC61966-2.1");
Creating these intents even when we aren't writing a PDFA? Because it's so small
it doesn't much matter?

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.cpp
File src/pdf/SkPDFMetadata.cpp (right):

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:14: const char uuidNamespace[] = "org.skia.pdf\n";
The require newline-termination of the namespace? really? Eww.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:36: digest.data[8] = (digest.data[6] & 0x3F) | 0x80;
Where do these magic numbers come from, the spec?

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:116: // TODO: When all compilers support this, remove
windows-specific code.
Nit: how far are we from being able to get rid of the Windows-specific version?
I thought we had C++11 everywhere now?

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:156: } while (false)
Nit: if you already have a for() statement with everything scoped inside, is the
do-while() necessary?

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:192: // Do not compress this.
WHY, OH, WHY?

(And why is compression even a topic? Is it going on somewhere nearby
unobviously? (I could easily have missed it if it isn't in code that is part of
this review.))

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:208: extra += 3;  // strlen("&lt;") - strlen("<")
Nit: not obvious why we don't also have to escape &gt to preserve correctness

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:258: // the integer value 1 for pdfaid:part and
Yet you write '2' and 'B' below in templateString?

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.h
File src/pdf/SkPDFMetadata.h (right):

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.h...
src/pdf/SkPDFMetadata.h:24: UUID uuid() const;
Would it make sense to name this generateUUID() or something?

You're setting uuid = foo.uuid() and then passing uuid to functions and it's all
very dense; I've seen that before but is it really Skia style?

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFTypes.h
File src/pdf/SkPDFTypes.h (right):

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFTypes.h#ne...
src/pdf/SkPDFTypes.h:349: /** Add the passed object to the catalog, as well as
all it's dependencies.
no apostrophe!

[hal.canary, 2015-10-09 15:49:11]

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF.cpp
File src/doc/SkDocument_PDF.cpp (right):

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:197: id.reset(SkPDFMetadata::CreatePdfId(uuid,
uuid));
On 2015/10/09 15:31:37, tomhudson wrote:
> CreatePdfId(uuid, uuid) calls the first one "doc" and the second one
"instance".
> If you're passing the same UUID for both parameters, it's really not clear how
> they're being used.

    // We use the same UUID for Document ID and Instance ID since this
    // is the first revision of this document (and Skia does not
    // support revising existing PDF documents).

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:205: "sRGB IEC61966-2.1");
On 2015/10/09 15:31:37, tomhudson wrote:
> Creating these intents even when we aren't writing a PDFA? Because it's so
small
> it doesn't much matter?

1. I want to save 139 bytes.
2. Nobody cares outside of the PDF/A request.

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:205: "sRGB IEC61966-2.1");
On 2015/10/09 15:31:37, tomhudson wrote:
> I thought we don't actually support sRGB yet (and Mike doesn't want to invest
> headcount in it in the short-term?)

HTML, CSS, and SVG all specify that they use sRGB.  So the colors that come in
from those sources are already sRGB.  We just don't *blend* in a non-linear way.

[hal.canary, 2015-10-09 19:13:27]

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF.cpp
File src/doc/SkDocument_PDF.cpp (right):

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:42: trailerDict.insertObjRef("Info", info);
On 2015/10/09 15:31:37, tomhudson wrote:
> You made info optional?

Done.

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:45: trailerDict.insertObject("ID", id);
On 2015/10/09 15:31:37, tomhudson wrote:
> Both info and id are noted "take ownership", but for one you insert a ref and
> the other you insert the object. Consistent?
> (Confession: I haven't got your memory model internalized right now, although
> you've explained it very well in the past. No mention at
> https://skia.org/dev/design/pdftheory)

a ref just means "don't inline this object"

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.cpp
File src/pdf/SkPDFMetadata.cpp (right):

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:14: const char uuidNamespace[] = "org.skia.pdf\n";
On 2015/10/09 15:31:37, tomhudson wrote:
> The require newline-termination of the namespace? really? Eww.

    // The main requirement is for the UUID to be unique; the exact
    // format of the data that will be hashed is not important.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:36: digest.data[8] = (digest.data[6] & 0x3F) | 0x80;
On 2015/10/09 15:31:37, tomhudson wrote:
> Where do these magic numbers come from, the spec?

    // See RFC 4122, page 6-7.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:116: // TODO: When all compilers support this, remove
windows-specific code.
On 2015/10/09 15:31:37, tomhudson wrote:
> Nit: how far are we from being able to get rid of the Windows-specific
version?
> I thought we had C++11 everywhere now?

We have C++11 *semantics* everywhere.  The version of the C standard library I
have on my windows workstation has vsnprintf, but it does not behave according
to the standard.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:156: } while (false)
On 2015/10/09 15:31:37, tomhudson wrote:
> Nit: if you already have a for() statement with everything scoped inside, is
the
> do-while() necessary?

It means nothing, but allows me to put a semi-colon after
  HEXIFY(data, ptr, gHex, 4)

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:192: // Do not compress this.
On 2015/10/09 15:31:37, tomhudson wrote:
> WHY, OH, WHY?

The standard requires that a program that does not understand PDF can grep for
"<?xpacket" and extracť the entire XML.

> (And why is compression even a topic? Is it going on somewhere nearby
> unobviously? (I could easily have missed it if it isn't in code that is
>  part of this review.))

If I wanted to compress, I'd use SkPDFStream.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:208: extra += 3;  // strlen("&lt;") - strlen("<")
On 2015/10/09 15:31:37, tomhudson wrote:
> Nit: not obvious why we don't also have to escape &gt to preserve correctness

    <dc:description>
      <rdf:Alt>
        <rdf:li>
          My Document is called <<, aka "very much less than"
        </rdf:li>
      </rdf:Alt>
    </dc:description>

is not valid XML.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:258: // the integer value 1 for pdfaid:part and
On 2015/10/09 15:31:37, tomhudson wrote:
> Yet you write '2' and 'B' below in templateString?

The spec was confusing me.  But I think I have it correct in the code now. 
Comment removed.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFTypes.h
File src/pdf/SkPDFTypes.h (right):

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFTypes.h#ne...
src/pdf/SkPDFTypes.h:349: /** Add the passed object to the catalog, as well as
all it's dependencies.
On 2015/10/09 15:31:37, tomhudson wrote:
> no apostrophe!

Done.  Im so embarrassed.

[hal.canary, 2015-10-09 19:15:20]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-09 19:15:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394263003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394263003/60001

[tomhudson, 2015-10-09 19:21:37]

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF.cpp
File src/doc/SkDocument_PDF.cpp (right):

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:197: id.reset(SkPDFMetadata::CreatePdfId(uuid,
uuid));
On 2015/10/09 15:49:11, Hal Canary wrote:
> On 2015/10/09 15:31:37, tomhudson wrote:
> > CreatePdfId(uuid, uuid) calls the first one "doc" and the second one
> "instance".
> > If you're passing the same UUID for both parameters, it's really not clear
how
> > they're being used.
> 
>     // We use the same UUID for Document ID and Instance ID since this
>     // is the first revision of this document (and Skia does not
>     // support revising existing PDF documents).

Acknowledged.

https://codereview.chromium.org/1394263003/diff/20001/src/doc/SkDocument_PDF....
src/doc/SkDocument_PDF.cpp:205: "sRGB IEC61966-2.1");
On 2015/10/09 15:49:11, Hal Canary wrote:
> On 2015/10/09 15:31:37, tomhudson wrote:
> > Creating these intents even when we aren't writing a PDFA? Because it's so
> small
> > it doesn't much matter?
> 
> 1. I want to save 139 bytes.
> 2. Nobody cares outside of the PDF/A request.

Never mind, my question was wrong - this is inside an #ifdef.

HOWEVER, the #ifdef/#endif aren't 0-aligned.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.cpp
File src/pdf/SkPDFMetadata.cpp (right):

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:36: digest.data[8] = (digest.data[6] & 0x3F) | 0x80;
On 2015/10/09 19:13:27, Hal Canary wrote:
> On 2015/10/09 15:31:37, tomhudson wrote:
> > Where do these magic numbers come from, the spec?
> 
>     // See RFC 4122, page 6-7.

Acknowledged.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:116: // TODO: When all compilers support this, remove
windows-specific code.
On 2015/10/09 19:13:27, Hal Canary wrote:
> We have C++11 *semantics* everywhere.  The version of the C standard library I
> have on my windows workstation has vsnprintf, but it does not behave according
> to the standard.

Oh, ouch.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:156: } while (false)
On 2015/10/09 19:13:27, Hal Canary wrote:
> On 2015/10/09 15:31:37, tomhudson wrote:
> > Nit: if you already have a for() statement with everything scoped inside, is
> the
> > do-while() necessary?
> 
> It means nothing, but allows me to put a semi-colon after
>   HEXIFY(data, ptr, gHex, 4)

Acknowledged.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:192: // Do not compress this.
Just trying to understand why the comment was there and how a future maintainer
is supposed to be driven to act/understand based on it.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:208: extra += 3;  // strlen("&lt;") - strlen("<")
I think the key question is: XML never has user-generated text inside tags <>?
(The way HTML can have <foo bar="baz">)
Or, we assume that this is valid XML.

[commit-bot: I haz the power, 2015-10-09 19:22:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-09 19:22:22]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2015-10-09 19:28:49]

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.cpp
File src/pdf/SkPDFMetadata.cpp (right):

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:192: // Do not compress this.
On 2015/10/09 19:21:37, tomhudson wrote:
> Just trying to understand why the comment was there and how a future
maintainer
> is supposed to be driven to act/understand based on it.

Comment fixed:

    // Do not compress this.  The standard requires that a
    // program that does not understand PDF can grep for
    // "<?xpacket" and extracť the entire XML.

https://codereview.chromium.org/1394263003/diff/20001/src/pdf/SkPDFMetadata.c...
src/pdf/SkPDFMetadata.cpp:208: extra += 3;  // strlen("&lt;") - strlen("<")
On 2015/10/09 19:21:37, tomhudson wrote:
> I think the key question is: XML never has user-generated text inside tags <>?
> (The way HTML can have <foo bar="baz">)
> Or, we assume that this is valid XML.
> 

We don't use that aspect of XML here  everywhere we have user-provided or
client-provided text, it is in the form:

    <tag>user-provided plain text</tag>

[tomhudson, 2015-10-09 19:55:59]

lgtm

[hal.canary, 2015-10-09 20:02:55]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-10-09 20:03:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394263003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394263003/80001

[commit-bot: I haz the power, 2015-10-09 20:10:03]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/939c0fe51f157104758bcb268643c8b6d317a530

[bungeman-skia, 2015-10-09 20:45:37]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1398193002/ by bungeman@google.com.

The reason for reverting is: SkMD5 is not really part of the Skia library. This
is breaking the roll by using it, since Chromium doesn't build it..

[hal.canary, 2015-10-12 17:03:32]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-10-12 17:03:32]

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/1394263003/#ps100001
(title: "2015-10-12 (Monday) 11:49:14 EDT")

[commit-bot: I haz the power, 2015-10-12 17:03:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394263003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394263003/100001

[tomhudson, 2015-10-12 17:04:28]

Post-revert fixup LGTM.

[commit-bot: I haz the power, 2015-10-12 17:11:23]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/34422610ac22adceeabb66023120f27b96cae953