Switch SkPDFStream's internal storage from SkStream to SkData

Switch SkPDFStream's internal storage from SkStream to SkData

Motivation: This makes SkPDFStream thread-safe for two threads
serializing it at once, since a SkStream has an internal position.

Updated SkPDFFont, SkPDFGraphicState, and SkPDFPage's use of
SkPDFStream to use the SkData constructor rather than the SkStream
constructor (saving a memcpy).

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

Committed: https://skia.googlesource.com/skia/+/67ec1f8eecfb48bc0a6ba04c0057f103c1c9696f

[hal.canary, 2014-06-25 15:37:20]

ptal (fix for a subset of threading bugs)

[mtklein, 2014-06-25 15:53:17]

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:15: #include "../images/SkStreamHelpers.h"  //
CopyStreamToData
Weird.  Can't just #include "SkStreamHelpers.h" ?

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:33: setData(SkSafeRef(pdfStream.fData.get()));
Looks like we're reffing inside and outside setData?  Let's just do that once
(inside setData)?

Add this->?

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:101: return (fData.get()) ? fData->size() : 0;
Pointless parens?

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.h#newcode83
src/pdf/SkPDFStream.h:83: size_t dataSize();
move this up with getData?

[djsollen, 2014-06-25 17:35:59]

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:15: #include "../images/SkStreamHelpers.h"  //
CopyStreamToData
On 2014/06/25 15:53:17, mtklein wrote:
> Weird.  Can't just #include "SkStreamHelpers.h" ?

looks like the pdf gyp will need to be updated to include src/images in the
include path.

[hal.canary, 2014-06-25 18:00:30]

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:15: #include "../images/SkStreamHelpers.h"  //
CopyStreamToData
On 2014/06/25 17:35:59, djsollen wrote:
> On 2014/06/25 15:53:17, mtklein wrote:
> > Weird.  Can't just #include "SkStreamHelpers.h" ?
> 
> looks like the pdf gyp will need to be updated to include
> src/images in the include path.

I meant to fix that before uploading.

Done.

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:33: setData(SkSafeRef(pdfStream.fData.get()));
On 2014/06/25 15:53:17, mtklein wrote:
> Looks like we're reffing inside and outside setData?  Let's just do that once
> (inside setData)?
> 
> Add this->?

Done.

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.cpp#newc...
src/pdf/SkPDFStream.cpp:101: return (fData.get()) ? fData->size() : 0;
On 2014/06/25 15:53:17, mtklein wrote:
> Pointless parens?

Done.

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.chromium.org/340783013/diff/1/src/pdf/SkPDFStream.h#newcode83
src/pdf/SkPDFStream.h:83: size_t dataSize();
On 2014/06/25 15:53:17, mtklein wrote:
> move this up with getData?

Done.

[mtklein, 2014-06-25 18:12:31]

https://codereview.chromium.org/340783013/diff/40001/gyp/pdf.gyp
File gyp/pdf.gyp (right):

https://codereview.chromium.org/340783013/diff/40001/gyp/pdf.gyp#newcode21
gyp/pdf.gyp:21: '../src/images', # needed to get SkStreamHelpers.h
sort?

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:153: static SkData* handleType1Stream(SkStream* srcStream,
size_t* headerLen,
-> handle_type1_stream

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:206: memcpy(result, src + 6, *headerLen);
Just pass buffer.get() into each of these?

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:208: memcpy(result + (*headerLen + *dataLen),
This whole area is really hard to follow.   Diagram and some structuring to make
it clear that what we're doing here with 6s is intentional?

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:220: SkAutoMalloc buffer(length);
Just start with SkAutoTMalloc<char> if you want chars?

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:560: return new SkPDFStream(cmapData.get());
Remind me why this is not

return new SkPDFStream(cmap.copyToData());

?

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFImage.cpp#n...
src/pdf/SkPDFImage.cpp:601: setData(data.get());
this-> everywhere possible?

[hal.canary, 2014-06-25 18:56:26]

https://codereview.chromium.org/340783013/diff/40001/gyp/pdf.gyp
File gyp/pdf.gyp (right):

https://codereview.chromium.org/340783013/diff/40001/gyp/pdf.gyp#newcode21
gyp/pdf.gyp:21: '../src/images', # needed to get SkStreamHelpers.h
On 2014/06/25 18:12:31, mtklein wrote:
> sort?

Done.

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:153: static SkData* handleType1Stream(SkStream* srcStream,
size_t* headerLen,
On 2014/06/25 18:12:31, mtklein wrote:
> -> handle_type1_stream

Done.

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:220: SkAutoMalloc buffer(length);
On 2014/06/25 18:12:31, mtklein wrote:
> Just start with SkAutoTMalloc<char> if you want chars?

SkAutoTMalloc uses SK_MALLOC_TEMP, which is not what we want.

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:560: return new SkPDFStream(cmapData.get());
On 2014/06/25 18:12:31, mtklein wrote:
> Remind me why this is not
> 
> return new SkPDFStream(cmap.copyToData());
> 
> ?

The SkPDFStream constructor refs the data.

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFImage.cpp#n...
src/pdf/SkPDFImage.cpp:601: setData(data.get());
On 2014/06/25 18:12:31, mtklein wrote:
> this-> everywhere possible?

Done.

[mtklein, 2014-06-26 13:21:59]

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:220: SkAutoMalloc buffer(length);
On 2014/06/25 18:56:26, Hal Canary wrote:
> On 2014/06/25 18:12:31, mtklein wrote:
> > Just start with SkAutoTMalloc<char> if you want chars?
> 
> SkAutoTMalloc uses SK_MALLOC_TEMP, which is not what we want.

Let's ignore that.  SK_MALLOC_TEMP doesn't do anything, and it looks like a
copy-and-paste bug that SkAutoTMalloc sets it.  (SkAutoSTMalloc makes a little
sense that it sets it, but really I think this is a dangerously designed feature
if the implication is we might use alloca instead of malloc...)

https://codereview.chromium.org/340783013/diff/40001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:560: return new SkPDFStream(cmapData.get());
On 2014/06/25 18:56:26, Hal Canary wrote:
> On 2014/06/25 18:12:31, mtklein wrote:
> > Remind me why this is not
> > 
> > return new SkPDFStream(cmap.copyToData());
> > 
> > ?
> 
> The SkPDFStream constructor refs the data.

Add a comment?

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:202: static const int pfbSectionHeaderLength = 6;
kPFBSectionHeaderLength?  I started panicking that this was some sort of Windows
pointer for a second there.

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:208: const uint8_t* const srcHeader = src +
pfbSectionHeaderLength;
I admire your attention to constness, but it's probably not telling us much that
the pointers aren't going to change.  We can see all the code where they're live
here at a glance.

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:208: const uint8_t* const srcHeader = src +
pfbSectionHeaderLength;
// There is a six-byte section header before header and data (but not trailer)
that we're not going to copy.

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

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:125: SkData::NewWithProc(psInvert,
strlen(psInvert), NULL, NULL));
NewWithCString?

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.cpp#...
src/pdf/SkPDFStream.cpp:105: SkAutoMutexAcquire lock(fMutex);  // multiple
threads could be calling emit
Why not lock in emitObject then?

I'd recommend you put all thoughts of performance to the side for now.  We want
correctness, and preferably obvious correctness.  We can improve things
performance-wise from there, if we care.  Just lock the hell out of this if it's
at all uncertain.

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.h#ne...
src/pdf/SkPDFStream.h:27: class SkPDFStream : public SkPDFDict {
Now that there's a mutex involved, we need to document what's thread safe and
what's not.  Can we tack const onto any of these methods?

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.h#ne...
src/pdf/SkPDFStream.h:93: SkMutex fMutex;
It's nice to note which fields this guards.  // Guards all fields  is fine if
that's true.

[hal.canary, 2014-06-26 18:42:02]

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:202: static const int pfbSectionHeaderLength = 6;
On 2014/06/26 13:21:59, mtklein wrote:
> kPFBSectionHeaderLength?  I started panicking that this was some sort of
Windows
> pointer for a second there.

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:208: const uint8_t* const srcHeader = src +
pfbSectionHeaderLength;
On 2014/06/26 13:21:59, mtklein wrote:
> // There is a six-byte section header before header and data (but not trailer)
> that we're not going to copy.

Done.

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFFont.cpp#ne...
src/pdf/SkPDFFont.cpp:208: const uint8_t* const srcHeader = src +
pfbSectionHeaderLength;
On 2014/06/26 13:21:59, mtklein wrote:
> I admire your attention to constness, but it's probably not telling us much
that
> the pointers aren't going to change.  We can see all the code where they're
live
> here at a glance.

Done.

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

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:125: SkData::NewWithProc(psInvert,
strlen(psInvert), NULL, NULL));
On 2014/06/26 13:21:59, mtklein wrote:
> NewWithCString?

I was avoiding a copy.  Also, NewWithCString includes the trailing nul
character, which we don't want.  Switched to NewWithCopy for clarity, added a
comment.

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.cpp#...
src/pdf/SkPDFStream.cpp:105: SkAutoMutexAcquire lock(fMutex);  // multiple
threads could be calling emit
On 2014/06/26 13:21:59, mtklein wrote:
> Why not lock in emitObject then?
> 
> I'd recommend you put all thoughts of performance to the side for now.  We
want
> correctness, and preferably obvious correctness.  We can improve things
> performance-wise from there, if we care.  Just lock the hell out of this if
it's
> at all uncertain.

Done.

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.h#ne...
src/pdf/SkPDFStream.h:27: class SkPDFStream : public SkPDFDict {
On 2014/06/26 13:21:59, mtklein wrote:
> Now that there's a mutex involved, we need to document what's thread safe and
> what's not.  Can we tack const onto any of these methods?

nothing is const.  I note that the emitObject and getOutputSize public methods
are thread-safe.

https://codereview.chromium.org/340783013/diff/80001/src/pdf/SkPDFStream.h#ne...
src/pdf/SkPDFStream.h:93: SkMutex fMutex;
On 2014/06/26 13:21:59, mtklein wrote:
> It's nice to note which fields this guards.  // Guards all fields  is fine if
> that's true.

Done.

[mtklein, 2014-06-26 19:28:37]

lgtm

https://codereview.chromium.org/340783013/diff/120001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/340783013/diff/120001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:206: SkAutoMalloc buffer(length);
AutoTMalloc<uint8_t>?

https://codereview.chromium.org/340783013/diff/120001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:234: SkAutoMalloc buffer(length);
AutoTMalloc<char>?

[hal.canary, 2014-06-26 20:19:24]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-06-26 20:19:43]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/340783013/140001

[commit-bot: I haz the power, 2014-06-26 21:00:36]

Change committed as c1dfa14b645ae274780f026dd86c9b633fbdad06

[rmistry, 2014-06-26 21:30:45]

A revert of this CL has been created in
https://codereview.chromium.org/354043005/ by rmistry@google.com.

The reason for reverting is: Causes canary failures.

[rmistry, 2014-06-26 21:32:08]

Log of failures:
http://108.170.220.120:10117/builders/Canary-Chrome-Win7-Ninja-x86-SharedLib_...

[hal.canary, 2014-06-27 13:52:18]

On 2014/06/26 21:32:08, rmistry wrote:
> Log of failures:
>
http://108.170.220.120:10117/builders/Canary-Chrome-Win7-Ninja-x86-SharedLib_...

I moved src/images/SkStreamHelpers.h to src/core/SkStreamPriv.h since those
functions are used in three different directories.  Now I hope the Canary will
build green.

[mtklein, 2014-06-27 13:57:31]

Seems reasonable.

You know, this is the sort of thing where more descriptive patch titles than
AnotherPatchSet would be helpful.

https://codereview.chromium.org/340783013/diff/160001/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/340783013/diff/160001/src/pdf/SkPDFStream.cpp...
src/pdf/SkPDFStream.cpp:96: SkAssertResult(stream->rewind());
Is change this intentional?

[hal.canary, 2014-06-27 14:09:39]

On 2014/06/27 13:57:31, mtklein wrote:
> Seems reasonable.
> 
> You know, this is the sort of thing where more descriptive patch titles than
> AnotherPatchSet would be helpful.
> 
> https://codereview.chromium.org/340783013/diff/160001/src/pdf/SkPDFStream.cpp
> File src/pdf/SkPDFStream.cpp (right):
> 
>
https://codereview.chromium.org/340783013/diff/160001/src/pdf/SkPDFStream.cpp...
> src/pdf/SkPDFStream.cpp:96: SkAssertResult(stream->rewind());
> Is change this intentional?

Yes.  I doubt it matters but I like the same side-effects to happen in Debug and
Release.

[hal.canary, 2014-06-27 17:22:40]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-06-27 17:23:45]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/340783013/160001

[commit-bot: I haz the power, 2014-06-27 17:38:09]

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

[commit-bot: I haz the power, 2014-06-27 17:38:10]

Failed to apply patch for src/pdf/SkPDFStream.h:
While running git apply --index -p1;
  error: patch failed: src/pdf/SkPDFStream.h:41
  error: src/pdf/SkPDFStream.h: patch does not apply

Patch:       src/pdf/SkPDFStream.h
Index: src/pdf/SkPDFStream.h
diff --git a/src/pdf/SkPDFStream.h b/src/pdf/SkPDFStream.h
index
6371bc187d14d9c90ed463d7734cfa67776f4d2e..f847a2952bace940929d2b10d68f143b6f369ba3
100644
--- a/src/pdf/SkPDFStream.h
+++ b/src/pdf/SkPDFStream.h
@@ -41,7 +41,8 @@ public:
     explicit SkPDFStream(const SkPDFStream& pdfStream);
     virtual ~SkPDFStream();
 
-    // The SkPDFObject interface.
+    // The SkPDFObject interface.  These two methods use a mutex to
+    // allow multiple threads to call at the same time.
     virtual void emitObject(SkWStream* stream, SkPDFCatalog* catalog,
                             bool indirect);
     virtual size_t getOutputSize(SkPDFCatalog* catalog, bool indirect);
@@ -67,22 +68,22 @@ protected:
         fSubstitute.reset(stream);
     }
 
-    SkPDFStream* getSubstitute() {
+    SkPDFStream* getSubstitute() const {
         return fSubstitute.get();
     }
 
     void setData(SkData* data);
     void setData(SkStream* stream);
 
-    SkStream* getData() {
-        return fData.get();
-    }
+    size_t dataSize() const;
+
+    SkData* getData() const { return fData.get(); }
 
     void setState(State state) {
         fState = state;
     }
 
-    State getState() {
+    State getState() const {
         return fState;
     }
 
@@ -90,8 +91,10 @@ private:
     // Indicates what form (or if) the stream has been requested.
     State fState;
 
-    // TODO(vandebo): Use SkData (after removing deprecated constructor).
-    SkAutoTUnref<SkStream> fData;
+    // Mutex guards fState, fData, and fSubstitute in public interface.
+    SkMutex fMutex;
+
+    SkAutoTUnref<SkData> fData;
     SkAutoTUnref<SkPDFStream> fSubstitute;
 
     typedef SkPDFDict INHERITED;

[hal.canary, 2014-06-27 17:46:35]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-06-27 17:46:46]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/340783013/180001

[commit-bot: I haz the power, 2014-06-27 18:36:26]

Change committed as 67ec1f8eecfb48bc0a6ba04c0057f103c1c9696f