Simplify reference management in SkPDF

Simplify reference management in SkPDF

Prior to this change, SkPDFObject subclasses were required
to track their resources separately from the document
structure.  (An object has a resource if it depends, via an
indirect reference, on another object).  This led to a lot
of extra code to duplicate effort.  I replace the
getResources() function with the much simpler addResources()
function.  I only define a non-trivial addResources() method
on arrays, dictionaries, and indirect object references.
All other specialized classes simply rely on their parent
class's implementation.

SkPDFObject::addResources() works by recursively walking the
directed graph of object (direct and indirect) references
and adding resources to a set.  It doesn't matter that there
are closed loops in the graph, since we check the set before
walking down a branch.

-  Add SkPDFObject::addResources() virtual function, with
   four implementations
-  Remove SkPDFObject::getResources() virtual function and
   all implementations.
-  Remove SkPDFObject::GetResourcesHelper()
-  Remove SkPDFObject::AddResourceHelper()
-  In SkPDFCatalog::findObjectIndex(), add an object to the
   catalog if it doesn't exist yet.
-  SkPDFCatalog::setSubstitute() no longer sets up resources
-  SkPDFDocument.cpp no longer needs the Streamer object
-  SkPDFDocument.cpp calls fDocCatalog->addResources to build
   the resource list.
-  SkPDFFont::addResource() removed
-  All SkPDF-::fResource sets removed (they are redundant).
-  removed SkPDFImage::addSMask() function
-  SkPDFResourceDict::getReferencedResources() removed.

Motivation: this removes quite a bit of code and makes the
objects slightly slimmer in memory.  Most importantly, this
will lead the way towards removing SkPDFObject's inheritance
from SkRefCnt, which will greatly simplify everything.

Testing: I usually test changes to the PDF backend by
comparing checksums of PDF files rendered from GMs and SKPs
before and after the change.  This change both re-orders and
re-numbers the indirect PDF objects.  I used the qpdf
program to normalize the PDFs and then compared the
normalized outputs from before and after the change; they
matched.

Committed: https://skia.googlesource.com/skia/+/bf799cd228282431e6311900dd383083f8af7164

[hal.canary, 2015-01-25 01:14:45]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-25 01:15:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870333002/1

[commit-bot: I haz the power, 2015-01-25 01:15:06]

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 2015-01-25 07:15 UTC

[hal.canary, 2015-01-25 01:15:35]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-01-25 13:53:46]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-25 13:54:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870333002/20001

[commit-bot: I haz the power, 2015-01-25 13:54:57]

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 2015-01-25 19:54 UTC

[hal.canary, 2015-01-25 13:55:11]

Patchset #1 (id:1) has been deleted

[hal.canary, 2015-01-25 13:55:18]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-01-25 18:07:05]

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

[hal.canary, 2015-01-25 18:08:25]

PTAL.

## test code ##

#!/bin/sh

: ${SKIA_SRC_DIR:=${HOME}/src/skia}
: ${SKIA_BUILD_DIR:=${HOME}/build/skia/gcc}
: ${SKIA_BUILD_TYPE:=Release}
SKIA_BUILD="${SKIA_BUILD_DIR}/${SKIA_BUILD_TYPE}"

git checkout SkPDFObject_addResources~

python "${SKIA_SRC_DIR}/tools/git-sync-deps"

SKIA_OUT="$SKIA_BUILD_DIR" "${SKIA_SRC_DIR}/gyp_skia"
ninja -C "$SKIA_BUILD" skp_to_pdf_md5 gmtoskp
"$SKIA_BUILD/gmtoskp" --writePath "${HOME}/data/gmskps" 
"$SKIA_BUILD/skp_to_pdf_md5" \
    --inputPaths "${HOME}/data/skps" "${HOME}/data/gmskps" \
        --outputDirectoryPath "/tmp/control"

git checkout SkPDFObject_addResources

SKIA_OUT="$SKIA_BUILD_DIR" "${SKIA_SRC_DIR}/gyp_skia"
ninja -C "$SKIA_BUILD" skp_to_pdf_md5
"$SKIA_BUILD/skp_to_pdf_md5" \
    --inputPaths "${HOME}/data/skps" "${HOME}/data/gmskps" \
        --outputDirectoryPath "/tmp/exper"

mkdir -p /tmp/control_normed /tmp/exper_normed
rm -f /tmp/control_normed/*.pdf /tmp/exper_normed/*.pdf

norm_pdf() {
    local SEARCH='\/ID \[<[0-9a-f]\{32\}><[0-9a-f]\{32\}>\]'
    local REPLACE='\/ID
\[<00000000000000000000000000000000><00000000000000000000000000000000>\]'
    qpdf --qdf --no-original-object-ids "$1" - | sed "s/${SEARCH}/${REPLACE}/"
}

(
    cd /tmp/control
    for file in *.pdf; do
        norm_pdf "/tmp/control/$file" > "/tmp/control_normed/${file}"
        norm_pdf "/tmp/exper/$file"   > "/tmp/exper_normed/${file}"
    done
)
diff -q -r /tmp/control_normed /tmp/exper_normed && echo 'SUCCESS!'

[mtklein, 2015-01-26 23:39:00]

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

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFCatalog.cpp...
src/pdf/SkPDFCatalog.cpp:59: int SkPDFCatalog::findObjectIndex(SkPDFObject* obj)
{
Seems like this wants a new name, lookupOrAdd?

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFGraphicStat...
File src/pdf/SkPDFGraphicState.cpp (right):

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:195: sMaskDict->insert("TR", new
SkPDFObjRef(invertFunction))->unref();
Might move the call to GetInvertFunction() right in line?

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

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
src/pdf/SkPDFTypes.h:24: typedef SkTSet<SkPDFObject*> SkPDFObjectSet;
I generally prefer writing out the full names of these things instead of
typedefing, unless you're leaving an option to easily swap between two
implementations or something.

I'm also pretty skeptical of SkTSet in general, but I see that really isn't the
point here.   It's not clear that we need to maintain sorted and insertion
order.  Might be something to look to simplify later.

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
src/pdf/SkPDFTypes.h:46: *  resouces for this object and for its resouces'
resouces....
This first sentence doesn't really make sense.  Is it

/** Adds all transitive dependencies of this object to resourceSet. */

?

It might make sense to mention why we pass catalog.  Can catalog be const*, or
const&?

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
src/pdf/SkPDFTypes.h:49: *  suitable for "leaf" objects.
Instead of writing this note, you could just put the implementation here, {}.

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
src/pdf/SkPDFTypes.h:51: virtual void addResources(SkPDFObjectSet* resourceSet,
Can this method be const ?

https://codereview.chromium.org/870333002/diff/20001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.chromium.org/870333002/diff/20001/tests/PDFPrimitivesTest....
tests/PDFPrimitivesTest.cpp:241: proxy->insert("Value", new
SkPDFInt(33))->unref();
If we're getting serious about PDF memory allocation, we might want to start
looking at tagged pointers instead of allocating ints and floats with malloc.

[hal.canary, 2015-02-09 23:35:01]

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

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFCatalog.cpp...
src/pdf/SkPDFCatalog.cpp:59: int SkPDFCatalog::findObjectIndex(SkPDFObject* obj)
{
On 2015/01/26 23:38:59, mtklein wrote:
> Seems like this wants a new name, lookupOrAdd?

I'm going to heavily refactor this class in a later CL (combine the catalog and
the resource set).  I'll hold off until then.

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFGraphicStat...
File src/pdf/SkPDFGraphicState.cpp (right):

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:195: sMaskDict->insert("TR", new
SkPDFObjRef(invertFunction))->unref();
On 2015/01/26 23:38:59, mtklein wrote:
> Might move the call to GetInvertFunction() right in line?

Done.

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

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
src/pdf/SkPDFTypes.h:24: typedef SkTSet<SkPDFObject*> SkPDFObjectSet;
On 2015/01/26 23:38:59, mtklein wrote:
> I generally prefer writing out the full names of these things instead of
> typedefing, unless you're leaving an option to easily swap between two
> implementations or something.
> 
> I'm also pretty skeptical of SkTSet in general, but I see that really isn't
the
> point here.   It's not clear that we need to maintain sorted and insertion
> order.  Might be something to look to simplify later. 

I'm not sure that I need SkTSet.  I really need an abstract interface:

    {
      bool add(SkPDFObject*); // return false if already known
    }

for addResources(), qhich I will add to SkPDFCatalog in a later CL.

Then SkTSet<SkPDFObject*> (or something like it) can be an implementation
detail of SkPDFCatalog.

Or maybe I'll just implement Map<SkPDFbject*, ObjectIndex> in a
later CL.

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
src/pdf/SkPDFTypes.h:46: *  resouces for this object and for its resouces'
resouces....
On 2015/01/26 23:38:59, mtklein wrote:
> This first sentence doesn't really make sense.  Is it
> 
> /** Adds all transitive dependencies of this object to resourceSet. */
> 
> ?
> 
> It might make sense to mention why we pass catalog.  Can catalog be const*, or
> const&?

Done.

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
src/pdf/SkPDFTypes.h:49: *  suitable for "leaf" objects.
On 2015/01/26 23:38:59, mtklein wrote:
> Instead of writing this note, you could just put the implementation here, {}.

You shouldn't put virtuals in a header file.

https://codereview.chromium.org/870333002/diff/20001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.chromium.org/870333002/diff/20001/tests/PDFPrimitivesTest....
tests/PDFPrimitivesTest.cpp:241: proxy->insert("Value", new
SkPDFInt(33))->unref();
On 2015/01/26 23:38:59, mtklein wrote:
> If we're getting serious about PDF memory allocation, we might want to start
> looking at tagged pointers instead of allocating ints and floats with malloc.

Acknowledged.

Let's do that *after* making these non-reference-counted.

[mtklein, 2015-02-09 23:47:17]

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

https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
src/pdf/SkPDFTypes.h:49: *  suitable for "leaf" objects.
On 2015/02/09 23:35:01, Hal Canary wrote:
> On 2015/01/26 23:38:59, mtklein wrote:
> > Instead of writing this note, you could just put the implementation here,
{}.
> 
> You shouldn't put virtuals in a header file.

Why not?

[hal.canary, 2015-02-09 23:56:55]

On 2015/02/09 23:47:17, mtklein wrote:
> https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h
> File src/pdf/SkPDFTypes.h (right):
> 
>
https://codereview.chromium.org/870333002/diff/20001/src/pdf/SkPDFTypes.h#new...
> src/pdf/SkPDFTypes.h:49: *  suitable for "leaf" objects.
> On 2015/02/09 23:35:01, Hal Canary wrote:
> > On 2015/01/26 23:38:59, mtklein wrote:
> > > Instead of writing this note, you could just put the implementation here,
> {}.
> > 
> > You shouldn't put virtuals in a header file.
> 
> Why not?

Quoting from Chromium's style guide: "Stop inlining virtual methods.  You can't
inline virtual methods under most circumstances, even if the method would
otherwise be inlined because it's very short. The compiler must do runtime
dispatch on any virtual method where the compiler doesn't know the object's
complete type, which rules out the majority of cases where you have an object." 
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

[mtklein, 2015-02-09 23:59:18]

> Quoting from Chromium's style guide: "Stop inlining virtual methods.  You
can't
> inline virtual methods under most circumstances, even if the method would
> otherwise be inlined because it's very short. The compiler must do runtime
> dispatch on any virtual method where the compiler doesn't know the object's
> complete type, which rules out the majority of cases where you have an
object." 
> http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

Okay, yep, we're certainly not going to inline it.  So what's the problem?

[hal.canary, 2015-02-10 00:42:20]

On 2015/02/09 23:59:18, mtklein wrote:
> > Quoting from Chromium's style guide: "Stop inlining virtual methods.  You
> can't
> > inline virtual methods under most circumstances, even if the method would
> > otherwise be inlined because it's very short. The compiler must do runtime
> > dispatch on any virtual method where the compiler doesn't know the object's
> > complete type, which rules out the majority of cases where you have an
> object." 
> > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts
> 
> Okay, yep, we're certainly not going to inline it.  So what's the problem?

"A function defined entirely inside a class definition, ... is always inline"

[mtklein, 2015-02-10 14:02:41]

> "A function defined entirely inside a class definition, ... is always inline"

Okay, so what's the problem?

[hal.canary, 2015-02-10 16:05:42]

done

[mtklein, 2015-02-10 17:25:53]

lgtm

https://codereview.chromium.org/870333002/diff/60001/src/pdf/SkPDFCatalog.cpp
File src/pdf/SkPDFCatalog.cpp (right):

https://codereview.chromium.org/870333002/diff/60001/src/pdf/SkPDFCatalog.cpp...
src/pdf/SkPDFCatalog.cpp:65: struct Rec newEntry(obj, false);
drop struct?  This is not C.

[hal.canary, 2015-02-10 17:42:34]

New patchsets have been uploaded after l-g-t-m from mtklein@google.com

[hal.canary, 2015-02-10 17:44:59]

One last round of correctness tests, then I'll land.

https://codereview.chromium.org/870333002/diff/60001/src/pdf/SkPDFCatalog.cpp
File src/pdf/SkPDFCatalog.cpp (right):

https://codereview.chromium.org/870333002/diff/60001/src/pdf/SkPDFCatalog.cpp...
src/pdf/SkPDFCatalog.cpp:65: struct Rec newEntry(obj, false);
On 2015/02/10 17:25:52, mtklein wrote:
> drop struct?  This is not C.

Done.

[hal.canary, 2015-02-10 18:12:32]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-10 18:13:31]

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

[commit-bot: I haz the power, 2015-02-10 18:18:10]

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

[commit-bot: I haz the power, 2015-02-10 18:18:11]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[hal.canary, 2015-02-10 21:17:33]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-10 21:18:13]

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

[commit-bot: I haz the power, 2015-02-10 21:32:14]

Committed patchset #5 (id:100001) as
https://skia.googlesource.com/skia/+/bf799cd228282431e6311900dd383083f8af7164

[hal.canary, 2015-02-10 21:33:15]

Note:  I changed 

   void SkPDFCatalog::emitObjectNumber(SkWStream*, SkPDFObject*) 	  

to

    int32_t SkPDFCatalog::getObjectNumber(SkPDFObject*)

to simplify unit tests.