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
R=mtklein@google.com, djsollen@google.com

Author: halcanary@google.com

Review URL: https://codereview.chromium.org/340783013

diff --git a/gyp/pdf.gyp b/gyp/pdf.gyp
index e5923e7..ca4d52e 100644 (file)
--- a/gyp/pdf.gyp
+++ b/gyp/pdf.gyp
@@ -18,6 +18,7 @@
         '../src/core', # needed to get SkGlyphCache.h and SkTextFormatParams.h
         '../src/pdf',
         '../src/utils', # needed to get SkBitSet.h
+        '../src/images', # needed to get SkStreamHelpers.h
       ],
       'sources': [
         'pdf.gypi', # Makes the gypi appear in IDEs (but does not modify the build).

diff --git a/src/pdf/SkPDFFont.cpp b/src/pdf/SkPDFFont.cpp
index 014b328..aa4b4b5 100644 (file)
--- a/src/pdf/SkPDFFont.cpp
+++ b/src/pdf/SkPDFFont.cpp
@@ -150,8 +150,8 @@ int8_t hexToBin(uint8_t c) {
     return -1;
 }
 
-SkStream* handleType1Stream(SkStream* srcStream, size_t* headerLen,
-                            size_t* dataLen, size_t* trailerLen) {
+static SkData* handle_type1_stream(SkStream* srcStream, size_t* headerLen,
+                                   size_t* dataLen, size_t* trailerLen) {
     // srcStream may be backed by a file or a unseekable fd, so we may not be
     // able to use skip(), rewind(), or getMemoryBase().  read()ing through
     // the input only once is doable, but very ugly. Furthermore, it'd be nice
@@ -199,26 +199,43 @@ SkStream* handleType1Stream(SkStream* srcStream, size_t* headerLen,
     SkAutoDataUnref aud(data);
 
     if (parsePFB(src, srcLen, headerLen, dataLen, trailerLen)) {
-        SkMemoryStream* result =
-            new SkMemoryStream(*headerLen + *dataLen + *trailerLen);
-        memcpy((char*)result->getAtPos(), src + 6, *headerLen);
-        result->seek(*headerLen);
-        memcpy((char*)result->getAtPos(), src + 6 + *headerLen + 6, *dataLen);
-        result->seek(*headerLen + *dataLen);
-        memcpy((char*)result->getAtPos(), src + 6 + *headerLen + 6 + *dataLen,
-               *trailerLen);
-        result->rewind();
-        return result;
+        static const int kPFBSectionHeaderLength = 6;
+        const size_t length = *headerLen + *dataLen + *trailerLen;
+        SkASSERT(length > 0);
+        SkASSERT(length + (2 * kPFBSectionHeaderLength) <= srcLen);
+
+        SkAutoTMalloc<uint8_t> buffer(length);
+
+        const uint8_t* const srcHeader = src + kPFBSectionHeaderLength;
+        // There is a six-byte section header before header and data
+        // (but not trailer) that we're not going to copy.
+        const uint8_t* const srcData
+            = srcHeader + *headerLen + kPFBSectionHeaderLength;
+        const uint8_t* const srcTrailer = srcData + *headerLen;
+
+        uint8_t* const resultHeader = buffer.get();
+        uint8_t* const resultData = resultHeader + *headerLen;
+        uint8_t* const resultTrailer = resultData + *dataLen;
+
+        SkASSERT(resultTrailer + *trailerLen == resultHeader + length);
+
+        memcpy(resultHeader,  srcHeader,  *headerLen);
+        memcpy(resultData,    srcData,    *dataLen);
+        memcpy(resultTrailer, srcTrailer, *trailerLen);
+
+        return SkData::NewFromMalloc(buffer.detach(), length);
     }
 
     // A PFA has to be converted for PDF.
     size_t hexDataLen;
     if (parsePFA((const char*)src, srcLen, headerLen, &hexDataLen, dataLen,
                  trailerLen)) {
-        SkMemoryStream* result =
-            new SkMemoryStream(*headerLen + *dataLen + *trailerLen);
-        memcpy((char*)result->getAtPos(), src, *headerLen);
-        result->seek(*headerLen);
+        const size_t length = *headerLen + *dataLen + *trailerLen;
+        SkASSERT(length > 0);
+        SkAutoTMalloc<uint8_t> buffer(length);
+
+        memcpy(buffer.get(), src, *headerLen);
+        uint8_t* const resultData = &(buffer[*headerLen]);
 
         const uint8_t* hexData = src + *headerLen;
         const uint8_t* trailer = hexData + hexDataLen;
@@ -236,21 +253,19 @@ SkStream* handleType1Stream(SkStream* srcStream, size_t* headerLen,
             } else {
                 dataByte |= curNibble;
                 highNibble = true;
-                ((char *)result->getAtPos())[outputOffset++] = dataByte;
+                resultData[outputOffset++] = dataByte;
             }
         }
         if (!highNibble) {
-            ((char *)result->getAtPos())[outputOffset++] = dataByte;
+            resultData[outputOffset++] = dataByte;
         }
         SkASSERT(outputOffset == *dataLen);
-        result->seek(*headerLen + outputOffset);
 
-        memcpy((char *)result->getAtPos(), src + *headerLen + hexDataLen,
-               *trailerLen);
-        result->rewind();
-        return result;
-    }
+        uint8_t* const resultTrailer = &(buffer[*headerLen + outputOffset]);
+        memcpy(resultTrailer, src + *headerLen + hexDataLen, *trailerLen);
 
+        return SkData::NewFromMalloc(buffer.detach(), length);
+    }
     return NULL;
 }
 
@@ -556,9 +571,8 @@ static SkPDFStream* generate_tounicode_cmap(
     append_cmap_sections(glyphToUnicode, subset, &cmap, multiByteGlyphs,
                          firstGlyphID, lastGlyphID);
     append_cmap_footer(&cmap);
-    SkAutoTUnref<SkMemoryStream> cmapStream(new SkMemoryStream());
-    cmapStream->setData(cmap.copyToData())->unref();
-    return new SkPDFStream(cmapStream.get());
+    SkAutoTUnref<SkData> cmapData(cmap.copyToData());
+    return new SkPDFStream(cmapData.get());
 }
 
 #if defined (SK_SFNTLY_SUBSETTER)
@@ -574,6 +588,7 @@ static size_t get_subset_font_stream(const char* fontName,
                                      SkPDFStream** fontStream) {
     int ttcIndex;
     SkAutoTUnref<SkStream> fontData(typeface->openStream(&ttcIndex));
+    SkASSERT(fontData.get());
 
     size_t fontSize = fontData->getLength();
 
@@ -1295,7 +1310,7 @@ bool SkPDFType1Font::addFontDescriptor(int16_t defaultWidth) {
     size_t data SK_INIT_TO_AVOID_WARNING;
     size_t trailer SK_INIT_TO_AVOID_WARNING;
     SkAutoTUnref<SkStream> rawFontData(typeface()->openStream(&ttcIndex));
-    SkStream* fontData = handleType1Stream(rawFontData.get(), &header, &data,
+    SkData* fontData = handle_type1_stream(rawFontData.get(), &header, &data,
                                            &trailer);
     if (fontData == NULL) {
         return false;

diff --git a/src/pdf/SkPDFGraphicState.cpp b/src/pdf/SkPDFGraphicState.cpp
index 1b49534..fa3baaf 100644 (file)
--- a/src/pdf/SkPDFGraphicState.cpp
+++ b/src/pdf/SkPDFGraphicState.cpp
@@ -5,10 +5,10 @@
  * found in the LICENSE file.
  */
 
+#include "SkData.h"
 #include "SkPDFFormXObject.h"
 #include "SkPDFGraphicState.h"
 #include "SkPDFUtils.h"
-#include "SkStream.h"
 #include "SkTypes.h"
 
 static const char* blend_mode_from_xfermode(SkXfermode::Mode mode) {
@@ -121,8 +121,9 @@ SkPDFObject* SkPDFGraphicState::GetInvertFunction() {
         domainAndRange->appendInt(1);
 
         static const char psInvert[] = "{1 exch sub}";
-        SkAutoTUnref<SkMemoryStream> psInvertStream(
-            new SkMemoryStream(&psInvert, strlen(psInvert), true));
+        // Do not copy the trailing '\0' into the SkData.
+        SkAutoTUnref<SkData> psInvertStream(
+                SkData::NewWithCopy(psInvert, strlen(psInvert)));
 
         invertFunction = new SkPDFStream(psInvertStream.get());
         invertFunction->insertInt("FunctionType", 4);

diff --git a/src/pdf/SkPDFImage.cpp b/src/pdf/SkPDFImage.cpp
index 77fd84e..7e17f98 100644 (file)
--- a/src/pdf/SkPDFImage.cpp
+++ b/src/pdf/SkPDFImage.cpp
@@ -512,7 +512,7 @@ SkPDFImage::SkPDFImage(SkStream* stream,
     }
 
     if (stream != NULL) {
-        setData(stream);
+        this->setData(stream);
         fStreamValid = true;
     } else {
         fStreamValid = false;
@@ -598,13 +598,11 @@ bool SkPDFImage::populate(SkPDFCatalog* catalog) {
             SkAutoTUnref<SkData> data(fEncoder(&pixelRefOffset, subset));
             if (data.get() && data->size() < get_uncompressed_size(fBitmap,
                                                                    fSrcRect)) {
-                SkAutoTUnref<SkStream> stream(SkNEW_ARGS(SkMemoryStream,
-                                                         (data)));
-                setData(stream.get());
+                this->setData(data.get());
 
                 insertName("Filter", "DCTDecode");
                 insertInt("ColorTransform", kNoColorTransform);
-                insertInt("Length", getData()->getLength());
+                insertInt("Length", this->dataSize());
                 setState(kCompressed_State);
                 return true;
             }
@@ -613,7 +611,7 @@ bool SkPDFImage::populate(SkPDFCatalog* catalog) {
         if (!fStreamValid) {
             SkAutoTUnref<SkStream> stream(
                     extract_image_data(fBitmap, fSrcRect, fIsAlpha, NULL));
-            setData(stream);
+            this->setData(stream);
             fStreamValid = true;
         }
         return INHERITED::populate(catalog);

diff --git a/src/pdf/SkPDFPage.cpp b/src/pdf/SkPDFPage.cpp
index 8961d2f..cfb6790 100644 (file)
--- a/src/pdf/SkPDFPage.cpp
+++ b/src/pdf/SkPDFPage.cpp
@@ -7,11 +7,11 @@
  */
 
 
+#include "SkData.h"
 #include "SkPDFCatalog.h"
 #include "SkPDFDevice.h"
 #include "SkPDFPage.h"
 #include "SkPDFResourceDict.h"
-#include "SkStream.h"
 
 SkPDFPage::SkPDFPage(SkPDFDevice* content)
     : SkPDFDict("Page"),
@@ -36,7 +36,7 @@ void SkPDFPage::finalizePage(SkPDFCatalog* catalog, bool firstPage,
             }
         }
 
-        SkAutoTUnref<SkStream> content(fDevice->content());
+        SkAutoTUnref<SkData> content(fDevice->copyContentToData());
         fContentStream.reset(new SkPDFStream(content.get()));
         insert("Contents", new SkPDFObjRef(fContentStream.get()))->unref();
     }

diff --git a/src/pdf/SkPDFStream.cpp b/src/pdf/SkPDFStream.cpp
index e570976..319433f 100644 (file)
--- a/src/pdf/SkPDFStream.cpp
+++ b/src/pdf/SkPDFStream.cpp
@@ -12,6 +12,7 @@
 #include "SkPDFCatalog.h"
 #include "SkPDFStream.h"
 #include "SkStream.h"
+#include "SkStreamHelpers.h"  // CopyStreamToData
 
 static bool skip_compression(SkPDFCatalog* catalog) {
     return SkToBool(catalog->getDocumentFlags() &
@@ -19,17 +20,17 @@ static bool skip_compression(SkPDFCatalog* catalog) {
 }
 
 SkPDFStream::SkPDFStream(SkStream* stream) : fState(kUnused_State) {
-    setData(stream);
+    this->setData(stream);
 }
 
 SkPDFStream::SkPDFStream(SkData* data) : fState(kUnused_State) {
-    setData(data);
+    this->setData(data);
 }
 
 SkPDFStream::SkPDFStream(const SkPDFStream& pdfStream)
         : SkPDFDict(),
           fState(kUnused_State) {
-    setData(pdfStream.fData.get());
+    this->setData(pdfStream.fData.get());
     bool removeLength = true;
     // Don't uncompress an already compressed stream, but we could.
     if (pdfStream.fState == kCompressed_State) {
@@ -55,14 +56,16 @@ void SkPDFStream::emitObject(SkWStream* stream, SkPDFCatalog* catalog,
     if (indirect) {
         return emitIndirectObject(stream, catalog);
     }
+    SkAutoMutexAcquire lock(fMutex);  // multiple threads could be calling emit
     if (!this->populate(catalog)) {
         return fSubstitute->emitObject(stream, catalog, indirect);
     }
 
     this->INHERITED::emitObject(stream, catalog, false);
     stream->writeText(" stream\n");
-    stream->writeStream(fData.get(), fData->getLength());
-    fData->rewind();
+    if (fData.get()) {
+        stream->write(fData->data(), fData->size());
+    }
     stream->writeText("\nendstream");
 }
 
@@ -70,30 +73,34 @@ size_t SkPDFStream::getOutputSize(SkPDFCatalog* catalog, bool indirect) {
     if (indirect) {
         return getIndirectOutputSize(catalog);
     }
+    SkAutoMutexAcquire lock(fMutex);  // multiple threads could be calling emit
     if (!this->populate(catalog)) {
         return fSubstitute->getOutputSize(catalog, indirect);
     }
 
     return this->INHERITED::getOutputSize(catalog, false) +
-        strlen(" stream\n\nendstream") + fData->getLength();
+        strlen(" stream\n\nendstream") + this->dataSize();
 }
 
 SkPDFStream::SkPDFStream() : fState(kUnused_State) {}
 
 void SkPDFStream::setData(SkData* data) {
-    SkMemoryStream* stream = new SkMemoryStream;
-    stream->setData(data);
-    fData.reset(stream);  // Transfer ownership.
+    fData.reset(SkSafeRef(data));
 }
 
 void SkPDFStream::setData(SkStream* stream) {
     // Code assumes that the stream starts at the beginning and is rewindable.
     if (stream) {
         SkASSERT(stream->getPosition() == 0);
+        fData.reset(CopyStreamToData(stream));
         SkASSERT(stream->rewind());
+    } else {
+        fData.reset(NULL);
     }
-    fData.reset(stream);
-    SkSafeRef(stream);
+}
+
+size_t SkPDFStream::dataSize() const {
+    return fData.get() ? fData->size() : 0;
 }
 
 bool SkPDFStream::populate(SkPDFCatalog* catalog) {
@@ -102,17 +109,15 @@ bool SkPDFStream::populate(SkPDFCatalog* catalog) {
             SkDynamicMemoryWStream compressedData;
 
             SkAssertResult(SkFlate::Deflate(fData.get(), &compressedData));
-            if (compressedData.getOffset() < fData->getLength()) {
-                SkMemoryStream* stream = new SkMemoryStream;
-                stream->setData(compressedData.copyToData())->unref();
-                fData.reset(stream);  // Transfer ownership.
+            if (compressedData.getOffset() < this->dataSize()) {
+                fData.reset(compressedData.copyToData());
                 insertName("Filter", "FlateDecode");
             }
             fState = kCompressed_State;
         } else {
             fState = kNoCompression_State;
         }
-        insertInt("Length", fData->getLength());
+        insertInt("Length", this->dataSize());
     } else if (fState == kNoCompression_State && !skip_compression(catalog) &&
                SkFlate::HaveFlate()) {
         if (!fSubstitute.get()) {

diff --git a/src/pdf/SkPDFStream.h b/src/pdf/SkPDFStream.h
index 6371bc1..f847a29 100644 (file)
--- 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;