From a1193e4b0e34a7e4e1bd33e9708d7341679f8321 Mon Sep 17 00:00:00 2001 From: scroggo Date: Wed, 21 Jan 2015 12:09:53 -0800 Subject: Make SkStream *not* ref counted. SkStream is a stateful object, so it does not make sense for it to have multiple owners. Make SkStream inherit directly from SkNoncopyable. Update methods which previously called SkStream::ref() (e.g. SkImageDecoder::buildTileIndex() and SkFrontBufferedStream::Create(), which required the existing owners to call SkStream::unref()) to take ownership of their SkStream parameters and delete when done (including on failure). Switch all SkAutoTUnrefs to SkAutoTDeletes. In some cases this means heap allocating streams that were previously stack allocated. Respect ownership rules of SkTypeface::CreateFromStream() and SkImageDecoder::buildTileIndex(). Update the comments for exceptional methods which do not affect the ownership of their SkStream parameters (e.g. SkPicture::CreateFromStream() and SkTypeface::Deserialize()) to be explicit about ownership. Remove test_stream_life, which tested that buildTileIndex() behaved correctly when SkStream was a ref counted object. The test does not make sense now that it is not. In SkPDFStream, remove the SkMemoryStream member. Instead of using it, create a new SkMemoryStream to pass to fDataStream (which is now an SkAutoTDelete). Make other pdf rasterizers behave like SkPDFDocumentToBitmap. SkPDFDocumentToBitmap delete the SkStream, so do the same in the following pdf rasterizers: SkPopplerRasterizePDF SkNativeRasterizePDF SkNoRasterizePDF Requires a change to Android, which currently treats SkStreams as ref counted objects. Review URL: https://codereview.chromium.org/849103004 --- tests/FontHostStreamTest.cpp | 4 +-- tests/FrontBufferedStreamTest.cpp | 45 +++++++++++++------------ tests/ImageDecodingTest.cpp | 70 ++------------------------------------- tests/KtxTest.cpp | 4 +-- tests/PDFPrimitivesTest.cpp | 2 +- tests/SerializationTest.cpp | 2 +- tests/StreamTest.cpp | 14 ++++---- 7 files changed, 40 insertions(+), 101 deletions(-) (limited to 'tests') diff --git a/tests/FontHostStreamTest.cpp b/tests/FontHostStreamTest.cpp index 18c74605bf..affda98cf6 100644 --- a/tests/FontHostStreamTest.cpp +++ b/tests/FontHostStreamTest.cpp @@ -96,8 +96,8 @@ DEF_TEST(FontHostStream, reporter) { } int ttcIndex; - SkAutoTUnref fontData(origTypeface->openStream(&ttcIndex)); - SkTypeface* streamTypeface = SkTypeface::CreateFromStream(fontData); + SkAutoTDelete fontData(origTypeface->openStream(&ttcIndex)); + SkTypeface* streamTypeface = SkTypeface::CreateFromStream(fontData.detach()); SkFontDescriptor desc; bool isLocalStream = false; diff --git a/tests/FrontBufferedStreamTest.cpp b/tests/FrontBufferedStreamTest.cpp index 167d1a20e8..69dade04b9 100644 --- a/tests/FrontBufferedStreamTest.cpp +++ b/tests/FrontBufferedStreamTest.cpp @@ -51,10 +51,13 @@ const char gAbcs[] = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdef // Tests reading the stream across boundaries of what has been buffered so far and what // the total buffer size is. static void test_incremental_buffering(skiatest::Reporter* reporter, size_t bufferSize) { - SkMemoryStream memStream(gAbcs, strlen(gAbcs), false); + // NOTE: For this and other tests in this file, we cheat and continue to refer to the + // wrapped stream, but that's okay because we know the wrapping stream has not been + // deleted yet (and we only call const methods in it). + SkMemoryStream* memStream = SkNEW_ARGS(SkMemoryStream, (gAbcs, strlen(gAbcs), false)); - SkAutoTUnref bufferedStream(SkFrontBufferedStream::Create(&memStream, bufferSize)); - test_hasLength(reporter, *bufferedStream.get(), memStream); + SkAutoTDelete bufferedStream(SkFrontBufferedStream::Create(memStream, bufferSize)); + test_hasLength(reporter, *bufferedStream.get(), *memStream); // First, test reading less than the max buffer size. test_read(reporter, bufferedStream, gAbcs, bufferSize / 2); @@ -79,9 +82,9 @@ static void test_incremental_buffering(skiatest::Reporter* reporter, size_t buff } static void test_perfectly_sized_buffer(skiatest::Reporter* reporter, size_t bufferSize) { - SkMemoryStream memStream(gAbcs, strlen(gAbcs), false); - SkAutoTUnref bufferedStream(SkFrontBufferedStream::Create(&memStream, bufferSize)); - test_hasLength(reporter, *bufferedStream.get(), memStream); + SkMemoryStream* memStream = SkNEW_ARGS(SkMemoryStream, (gAbcs, strlen(gAbcs), false)); + SkAutoTDelete bufferedStream(SkFrontBufferedStream::Create(memStream, bufferSize)); + test_hasLength(reporter, *bufferedStream.get(), *memStream); // Read exactly the amount that fits in the buffer. test_read(reporter, bufferedStream, gAbcs, bufferSize); @@ -98,9 +101,9 @@ static void test_perfectly_sized_buffer(skiatest::Reporter* reporter, size_t buf } static void test_skipping(skiatest::Reporter* reporter, size_t bufferSize) { - SkMemoryStream memStream(gAbcs, strlen(gAbcs), false); - SkAutoTUnref bufferedStream(SkFrontBufferedStream::Create(&memStream, bufferSize)); - test_hasLength(reporter, *bufferedStream.get(), memStream); + SkMemoryStream* memStream = SkNEW_ARGS(SkMemoryStream, (gAbcs, strlen(gAbcs), false)); + SkAutoTDelete bufferedStream(SkFrontBufferedStream::Create(memStream, bufferSize)); + test_hasLength(reporter, *bufferedStream.get(), *memStream); // Skip half the buffer. bufferedStream->skip(bufferSize / 2); @@ -149,11 +152,11 @@ private: // does not invalidate the buffer. static void test_read_beyond_buffer(skiatest::Reporter* reporter, size_t bufferSize) { // Use a stream that behaves like Android's stream. - AndroidLikeMemoryStream memStream((void*)gAbcs, bufferSize, false); + AndroidLikeMemoryStream* memStream = SkNEW_ARGS(AndroidLikeMemoryStream, ((void*)gAbcs, bufferSize, false)); // Create a buffer that matches the length of the stream. - SkAutoTUnref bufferedStream(SkFrontBufferedStream::Create(&memStream, bufferSize)); - test_hasLength(reporter, *bufferedStream.get(), memStream); + SkAutoTDelete bufferedStream(SkFrontBufferedStream::Create(memStream, bufferSize)); + test_hasLength(reporter, *bufferedStream.get(), *memStream); // Attempt to read one more than the bufferSize test_read(reporter, bufferedStream.get(), gAbcs, bufferSize + 1); @@ -197,22 +200,22 @@ private: static void test_length_combos(skiatest::Reporter* reporter, size_t bufferSize) { for (int hasLen = 0; hasLen <= 1; hasLen++) { for (int hasPos = 0; hasPos <= 1; hasPos++) { - LengthOptionalStream stream(SkToBool(hasLen), SkToBool(hasPos)); - SkAutoTUnref buffered(SkFrontBufferedStream::Create(&stream, bufferSize)); - test_hasLength(reporter, *buffered.get(), stream); + LengthOptionalStream* stream = SkNEW_ARGS(LengthOptionalStream, (SkToBool(hasLen), SkToBool(hasPos))); + SkAutoTDelete buffered(SkFrontBufferedStream::Create(stream, bufferSize)); + test_hasLength(reporter, *buffered.get(), *stream); } } } // Test using a stream with an initial offset. static void test_initial_offset(skiatest::Reporter* reporter, size_t bufferSize) { - SkMemoryStream memStream(gAbcs, strlen(gAbcs), false); + SkMemoryStream* memStream = SkNEW_ARGS(SkMemoryStream, (gAbcs, strlen(gAbcs), false)); // Skip a few characters into the memStream, so that bufferedStream represents an offset into // the stream it wraps. const size_t arbitraryOffset = 17; - memStream.skip(arbitraryOffset); - SkAutoTUnref bufferedStream(SkFrontBufferedStream::Create(&memStream, bufferSize)); + memStream->skip(arbitraryOffset); + SkAutoTDelete bufferedStream(SkFrontBufferedStream::Create(memStream, bufferSize)); // Since SkMemoryStream has a length and a position, bufferedStream must also. REPORTER_ASSERT(reporter, bufferedStream->hasLength()); @@ -283,13 +286,13 @@ private: }; DEF_TEST(ShortFrontBufferedStream, reporter) { - FailingStream failingStream; - SkAutoTUnref stream(SkFrontBufferedStream::Create(&failingStream, 64)); + FailingStream* failingStream = SkNEW(FailingStream); + SkAutoTDelete stream(SkFrontBufferedStream::Create(failingStream, 64)); SkBitmap bm; // The return value of DecodeStream is not important. We are just using DecodeStream because // it simulates a bug. DecodeStream will read the stream, then rewind, then attempt to read // again. FrontBufferedStream::read should not continue to read its underlying stream beyond // its end. SkImageDecoder::DecodeStream(stream, &bm); - REPORTER_ASSERT(reporter, !failingStream.readAfterEnd()); + REPORTER_ASSERT(reporter, !failingStream->readAfterEnd()); } diff --git a/tests/ImageDecodingTest.cpp b/tests/ImageDecodingTest.cpp index 78d6b6c35f..b94e29c11d 100644 --- a/tests/ImageDecodingTest.cpp +++ b/tests/ImageDecodingTest.cpp @@ -324,76 +324,13 @@ DEF_TEST(ImageDecoding_unpremul, reporter) { #endif // SK_BUILD_FOR_UNIX/ANDROID skbug.com/2388 #ifdef SK_DEBUG -// Create a stream containing a bitmap encoded to Type type. -static SkMemoryStream* create_image_stream(SkImageEncoder::Type type) { - SkBitmap bm; - const int size = 50; - bm.allocN32Pixels(size, size); - SkCanvas canvas(bm); - SkPoint points[2] = { - { SkIntToScalar(0), SkIntToScalar(0) }, - { SkIntToScalar(size), SkIntToScalar(size) } - }; - SkColor colors[2] = { SK_ColorWHITE, SK_ColorBLUE }; - SkShader* shader = SkGradientShader::CreateLinear(points, colors, NULL, - SK_ARRAY_COUNT(colors), - SkShader::kClamp_TileMode); - SkPaint paint; - paint.setShader(shader)->unref(); - canvas.drawPaint(paint); - // Now encode it to a stream. - SkAutoTUnref data(SkImageEncoder::EncodeData(bm, type, 100)); - if (NULL == data.get()) { - return NULL; - } - return SkNEW_ARGS(SkMemoryStream, (data.get())); -} - -// For every format that supports tile based decoding, ensure that -// calling decodeSubset will not fail if the caller has unreffed the -// stream provided in buildTileIndex. -// Only runs in debug mode since we are testing for a crash. -static void test_stream_life() { - const SkImageEncoder::Type gTypes[] = { -#ifdef SK_BUILD_FOR_ANDROID - SkImageEncoder::kJPEG_Type, - SkImageEncoder::kPNG_Type, -#endif - SkImageEncoder::kWEBP_Type, - }; - for (size_t i = 0; i < SK_ARRAY_COUNT(gTypes); ++i) { - // SkDebugf("encoding to %i\n", i); - SkAutoTUnref stream(create_image_stream(gTypes[i])); - if (NULL == stream.get()) { - SkDebugf("no stream\n"); - continue; - } - SkAutoTDelete decoder(SkImageDecoder::Factory(stream)); - if (NULL == decoder.get()) { - SkDebugf("no decoder\n"); - continue; - } - int width, height; - if (!decoder->buildTileIndex(stream.get(), &width, &height)) { - SkDebugf("could not build a tile index\n"); - continue; - } - // Now unref the stream to make sure it survives - stream.reset(NULL); - SkBitmap bm; - decoder->decodeSubset(&bm, SkIRect::MakeWH(width, height), kN32_SkColorType); - } -} - // Test inside SkScaledBitmapSampler.cpp extern void test_row_proc_choice(); - #endif // SK_DEBUG DEF_TEST(ImageDecoding, reporter) { test_unpremul(reporter); #ifdef SK_DEBUG - test_stream_life(); test_row_proc_choice(); #endif } @@ -488,7 +425,6 @@ static SkPixelRef* install_pixel_ref(SkBitmap* bitmap, SkASSERT(bitmap != NULL); SkASSERT(stream != NULL); SkASSERT(stream->rewind()); - SkASSERT(stream->unique()); SkColorType colorType = bitmap->colorType(); SkDecodingImageGenerator::Options opts(sampleSize, ditherImage, colorType); if (SkInstallDiscardablePixelRef( @@ -503,7 +439,7 @@ static SkPixelRef* install_pixel_ref(SkBitmap* bitmap, */ DEF_TEST(ImprovedBitmapFactory, reporter) { SkString pngFilename = GetResourcePath("randPixels.png"); - SkAutoTUnref stream(SkStream::NewFromFile(pngFilename.c_str())); + SkAutoTDelete stream(SkStream::NewFromFile(pngFilename.c_str())); if (sk_exists(pngFilename.c_str())) { SkBitmap bm; SkAssertResult(bm.setInfo(SkImageInfo::MakeN32Premul(1, 1))); @@ -698,7 +634,7 @@ DEF_TEST(ImageDecoderOptions, reporter) { SkAutoDataUnref encodedData(SkData::NewFromFileName(path.c_str())); REPORTER_ASSERT(reporter, encodedData.get() != NULL); - SkAutoTUnref encodedStream( + SkAutoTDelete encodedStream( SkStream::NewFromFile(path.c_str())); REPORTER_ASSERT(reporter, encodedStream.get() != NULL); @@ -782,7 +718,7 @@ private: DEF_TEST(ImageDecoding_JpegOverwrite, r) { SkString resourceDir = GetResourcePath(); SkString path = SkOSPath::Join(resourceDir.c_str(), "randPixels.jpg"); - SkAutoTUnref stream( + SkAutoTDelete stream( SkStream::NewFromFile(path.c_str())); if (!stream.get()) { SkDebugf("\nPath '%s' missing.\n", path.c_str()); diff --git a/tests/KtxTest.cpp b/tests/KtxTest.cpp index 23c1d70900..53caabdd91 100644 --- a/tests/KtxTest.cpp +++ b/tests/KtxTest.cpp @@ -55,7 +55,7 @@ DEF_TEST(KtxReadWrite, reporter) { SkAutoDataUnref encodedData(SkImageEncoder::EncodeData(bm8888, SkImageEncoder::kKTX_Type, 0)); REPORTER_ASSERT(reporter, encodedData); - SkAutoTUnref stream(SkNEW_ARGS(SkMemoryStream, (encodedData))); + SkAutoTDelete stream(SkNEW_ARGS(SkMemoryStream, (encodedData))); REPORTER_ASSERT(reporter, stream); SkBitmap decodedBitmap; @@ -107,7 +107,7 @@ DEF_TEST(KtxReadUnpremul, reporter) { 0xFF, 0xFF, 0xFF, 0x80, // Pixel 3 0xFF, 0xFF, 0xFF, 0x80};// Pixel 4 - SkAutoTUnref stream( + SkAutoTDelete stream( SkNEW_ARGS(SkMemoryStream, (kHalfWhiteKTX, sizeof(kHalfWhiteKTX)))); REPORTER_ASSERT(reporter, stream); diff --git a/tests/PDFPrimitivesTest.cpp b/tests/PDFPrimitivesTest.cpp index 3a4c6f6ab0..3610dd9581 100644 --- a/tests/PDFPrimitivesTest.cpp +++ b/tests/PDFPrimitivesTest.cpp @@ -119,7 +119,7 @@ static void SimpleCheckObjectOutput(skiatest::Reporter* reporter, static void TestPDFStream(skiatest::Reporter* reporter) { char streamBytes[] = "Test\nFoo\tBar"; - SkAutoTUnref streamData(new SkMemoryStream( + SkAutoTDelete streamData(new SkMemoryStream( streamBytes, strlen(streamBytes), true)); SkAutoTUnref stream(new SkPDFStream(streamData.get())); SimpleCheckObjectOutput( diff --git a/tests/SerializationTest.cpp b/tests/SerializationTest.cpp index cd5bc60574..f6a8a7d5fc 100644 --- a/tests/SerializationTest.cpp +++ b/tests/SerializationTest.cpp @@ -350,7 +350,7 @@ static void TestPictureTypefaceSerialization(skiatest::Reporter* reporter) { // Serlialize picture and create its clone from stream. SkDynamicMemoryWStream stream; picture->serialize(&stream); - SkAutoTUnref inputStream(stream.detachAsStream()); + SkAutoTDelete inputStream(stream.detachAsStream()); SkAutoTUnref loadedPicture(SkPicture::CreateFromStream(inputStream.get())); // Draw both original and clone picture and compare bitmaps -- they should be identical. diff --git a/tests/StreamTest.cpp b/tests/StreamTest.cpp index ab0af14088..ff22ecf554 100644 --- a/tests/StreamTest.cpp +++ b/tests/StreamTest.cpp @@ -58,7 +58,7 @@ static void test_filestreams(skiatest::Reporter* reporter, const char* tmpDir) { REPORTER_ASSERT(reporter, stream.isValid()); test_loop_stream(reporter, &stream, s, 26, 100); - SkAutoTUnref stream2(stream.duplicate()); + SkAutoTDelete stream2(stream.duplicate()); test_loop_stream(reporter, stream2.get(), s, 26, 100); } @@ -68,7 +68,7 @@ static void test_filestreams(skiatest::Reporter* reporter, const char* tmpDir) { REPORTER_ASSERT(reporter, stream.isValid()); test_loop_stream(reporter, &stream, s, 26, 100); - SkAutoTUnref stream2(stream.duplicate()); + SkAutoTDelete stream2(stream.duplicate()); test_loop_stream(reporter, stream2.get(), s, 26, 100); } } @@ -91,15 +91,15 @@ static void TestWStream(skiatest::Reporter* reporter) { } { - SkAutoTUnref stream(ds.detachAsStream()); + SkAutoTDelete stream(ds.detachAsStream()); REPORTER_ASSERT(reporter, 100 * 26 == stream->getLength()); REPORTER_ASSERT(reporter, ds.getOffset() == 0); test_loop_stream(reporter, stream.get(), s, 26, 100); - SkAutoTUnref stream2(stream->duplicate()); + SkAutoTDelete stream2(stream->duplicate()); test_loop_stream(reporter, stream2.get(), s, 26, 100); - SkAutoTUnref stream3(stream->fork()); + SkAutoTDelete stream3(stream->fork()); REPORTER_ASSERT(reporter, stream3->isAtEnd()); char tmp; size_t bytes = stream->read(&tmp, 1); @@ -121,11 +121,11 @@ static void TestWStream(skiatest::Reporter* reporter) { { // Test that this works after a copyToData. - SkAutoTUnref stream(ds.detachAsStream()); + SkAutoTDelete stream(ds.detachAsStream()); REPORTER_ASSERT(reporter, ds.getOffset() == 0); test_loop_stream(reporter, stream.get(), s, 26, 100); - SkAutoTUnref stream2(stream->duplicate()); + SkAutoTDelete stream2(stream->duplicate()); test_loop_stream(reporter, stream2.get(), s, 26, 100); } delete[] dst; -- cgit v1.2.3