PDF: Now threadsafe!

PDF: Now threadsafe!

The PDF canvas is now just as threadsafe as any other Skia canvas.

DM updated to thread PDF tests.

SkDocument_PDF now owns SkPDFCanon, and pointers to that canon are
passed around to all classes that need access to the canon.

BUG=skia:2683
Committed: https://skia.googlesource.com/skia/+/792c80f5a7b66e75d42664ccb298f31962c6654c

[hal.canary, 2015-02-19 23:07:34]

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

[hal.canary, 2015-02-19 23:07:35]

ptal

[hal.canary, 2015-02-19 23:13:26]

Patchset #1 (id:1) has been deleted

[hal.canary, 2015-02-19 23:13:34]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-19 23:13:46]

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

[commit-bot: I haz the power, 2015-02-19 23:13:47]

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-02-20 05:13 UTC

[mtklein, 2015-02-19 23:50:38]

super cool

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

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFDevice.h#ne...
src/pdf/SkPDFDevice.h:75: static SkPDFDevice* CreateUnflipped(SkISize pageSize,
This seems like 3 CLs:

1)
The PDF canvas is now just as threadsafe as any other Skia canvas.

DM updated to thread PDF tests.

SkDocument_PDF now owns SkPDFCanon, and pointers to that canon are
passed around to all classes that need access to the canon.

2)
SkPDFDevice now has factory function that matches what cllers need.

3)
SkPDFDeviceFlattener is removed.  There are no clients.

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

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:116: SkASSERT((fSMask || fCanon) && !(fSMask &&
fCanon));
My head asplode.  Can we write this as

if (fCanon) {
   SkASSERT(!fSMask);
   fCanon->removeGraphicState(this);
}

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

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:945: SkMatrix unflip;
Now unused?

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:952: const SkScalar dpi = 72.0f; /*TODO: get this
right*/
Is that, pull it from the device instead of making up a constant here?

[hal.canary, 2015-02-20 01:22:28]

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

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFDevice.h#ne...
src/pdf/SkPDFDevice.h:75: static SkPDFDevice* CreateUnflipped(SkISize pageSize,
On 2015/02/19 23:50:38, mtklein wrote:
> This seems like 3 CLs:
> 
> 1)
> The PDF canvas is now just as threadsafe as any other Skia canvas.
> 
> DM updated to thread PDF tests.
> 
> SkDocument_PDF now owns SkPDFCanon, and pointers to that canon are
> passed around to all classes that need access to the canon.
> 
> 2)
> SkPDFDevice now has factory function that matches what cllers need.
> 
> 3)
> SkPDFDeviceFlattener is removed.  There are no clients.

Will do.

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

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:116: SkASSERT((fSMask || fCanon) && !(fSMask &&
fCanon));
On 2015/02/19 23:50:38, mtklein wrote:
> My head asplode.  Can we write this as
> 
> if (fCanon) {
>    SkASSERT(!fSMask);
>    fCanon->removeGraphicState(this);
> }
Not exactly.  Is this more readable?

  SkASSERT((fSMask == NULL) != (fCanon == NULL));

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

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:945: SkMatrix unflip;
On 2015/02/19 23:50:38, mtklein wrote:
> Now unused?

Done.

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:952: const SkScalar dpi = 72.0f; /*TODO: get this
right*/
On 2015/02/19 23:50:38, mtklein wrote:
> Is that, pull it from the device instead of making up a constant here?

I meant to do that.  Done

[hal.canary, 2015-02-20 14:28:55]

rebased and ready to land.  Please review.

[hal.canary, 2015-02-20 14:43:16]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-20 14:44:13]

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

[commit-bot: I haz the power, 2015-02-20 14:44:14]

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-02-20 20:44 UTC

[mtklein, 2015-02-20 14:55:41]

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

https://codereview.chromium.org/944643002/diff/20001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:116: SkASSERT((fSMask || fCanon) && !(fSMask &&
fCanon));
On 2015/02/20 01:22:28, Hal Canary wrote:
> On 2015/02/19 23:50:38, mtklein wrote:
> > My head asplode.  Can we write this as
> > 
> > if (fCanon) {
> >    SkASSERT(!fSMask);
> >    fCanon->removeGraphicState(this);
> > }
> Not exactly.  Is this more readable?
> 
>   SkASSERT((fSMask == NULL) != (fCanon == NULL));
> 

Yes that was actually my first thought too, until I realized fSMask is a bool. 
Then I couldn't figure out any way to write it that wasn't either unclear or
misleading.

Why don't we just drop the fSMask checks altogether here?  Seems like mixing
that in is just sort of confusing things.

if (fCanon) {
    fCanon->removeGraphicState(this);
}

[mtklein, 2015-02-20 15:05:27]

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

https://codereview.chromium.org/944643002/diff/60001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:277: // TODO(halcanary): SkPDFBitmap::Create should take
a SkPDFCanon* parameter.
done!

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

https://codereview.chromium.org/944643002/diff/60001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:116: // Exactly one of the two is NULL.
Let's at least just remove the comment.  The assert seems fine and clearer
without it.

[hal.canary, 2015-02-20 15:07:08]

On 2015/02/20 15:05:27, mtklein wrote:
> https://codereview.chromium.org/944643002/diff/60001/src/pdf/SkPDFBitmap.cpp
> File src/pdf/SkPDFBitmap.cpp (right):
> 
>
https://codereview.chromium.org/944643002/diff/60001/src/pdf/SkPDFBitmap.cpp#...
> src/pdf/SkPDFBitmap.cpp:277: // TODO(halcanary): SkPDFBitmap::Create should
take
> a SkPDFCanon* parameter.
> done!
> 
>
https://codereview.chromium.org/944643002/diff/60001/src/pdf/SkPDFGraphicStat...
> File src/pdf/SkPDFGraphicState.cpp (right):
> 
>
https://codereview.chromium.org/944643002/diff/60001/src/pdf/SkPDFGraphicStat...
> src/pdf/SkPDFGraphicState.cpp:116: // Exactly one of the two is NULL.
> Let's at least just remove the comment.  The assert seems fine and clearer
> without it.

I just realized that that bool only existed to say that the object was not in
the canon.  I thought it was something more.  Removing it altogether.

[hal.canary, 2015-02-20 15:08:09]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-20 15:08:17]

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

[commit-bot: I haz the power, 2015-02-20 15:08:18]

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-02-20 21:08 UTC

[hal.canary, 2015-02-20 15:09:17]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-20 15:09:27]

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

[commit-bot: I haz the power, 2015-02-20 15:09:28]

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-02-20 21:09 UTC

[hal.canary, 2015-02-20 15:10:01]

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

https://codereview.chromium.org/944643002/diff/60001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:277: // TODO(halcanary): SkPDFBitmap::Create should take
a SkPDFCanon* parameter.
On 2015/02/20 15:05:27, mtklein wrote:
> done!

Done.

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

https://codereview.chromium.org/944643002/diff/60001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:116: // Exactly one of the two is NULL.
On 2015/02/20 15:05:27, mtklein wrote:
> Let's at least just remove the comment.  The assert seems fine and clearer
> without it.

Acknowledged.

[mtklein, 2015-02-20 15:10:09]

On 2015/02/20 15:09:28, I haz the power (commit-bot) wrote:
> 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-02-20 21:09 UTC

lgtm

[hal.canary, 2015-02-20 15:15:24]

On 2015/02/20 15:10:09, mtklein wrote:
> lgtm

On my local machine:

  $  ps -C dm -o %cpu,%mem,args
  %CPU %MEM COMMAND
  2545  2.4 ./dm --config pdf --src gm skp

[mtklein, 2015-02-20 15:17:55]

On 2015/02/20 15:15:24, Hal Canary wrote:
> On 2015/02/20 15:10:09, mtklein wrote:
> > lgtm
> 
> On my local machine:
> 
>   $  ps -C dm -o %cpu,%mem,args
>   %CPU %MEM COMMAND
>   2545  2.4 ./dm --config pdf --src gm skp

Is that 25 CPUs, 2.4G RAM?  SGTM

[commit-bot: I haz the power, 2015-02-20 15:21:10]

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