From 9d02b264b72be64702e43cb797b7899a8a6926ca Mon Sep 17 00:00:00 2001 From: bsalomon Date: Mon, 1 Feb 2016 12:49:30 -0800 Subject: [PATCH] Fix GL readback code to handle rowbytes correctly for non-32bit formats Update tests to exercise more rowbytes. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1645043006 Review URL: https://codereview.chromium.org/1645043006 --- src/gpu/gl/GrGLGpu.cpp | 11 +-- tests/ReadPixelsTest.cpp | 21 ++++-- tests/ReadWriteAlphaTest.cpp | 167 +++++++++++++++++++++++-------------------- 3 files changed, 111 insertions(+), 88 deletions(-) diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp index 6a61f31..3b7ccd3 100644 --- a/src/gpu/gl/GrGLGpu.cpp +++ b/src/gpu/gl/GrGLGpu.cpp @@ -2296,7 +2296,8 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, GrGLIRect readRect; readRect.setRelativeTo(glvp, left, top, width, height, renderTarget->origin()); - size_t tightRowBytes = GrBytesPerPixel(config) * width; + size_t bytesPerPixel = GrBytesPerPixel(config); + size_t tightRowBytes = bytesPerPixel * width; size_t readDstRowBytes = tightRowBytes; void* readDst = buffer; @@ -2305,10 +2306,9 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, // a scratch buffer. SkAutoSMalloc<32 * sizeof(GrColor)> scratch; if (rowBytes != tightRowBytes) { - if (this->glCaps().packRowLengthSupport()) { - SkASSERT(!(rowBytes % sizeof(GrColor))); + if (this->glCaps().packRowLengthSupport() && !(rowBytes % bytesPerPixel)) { GL_CALL(PixelStorei(GR_GL_PACK_ROW_LENGTH, - static_cast(rowBytes / sizeof(GrColor)))); + static_cast(rowBytes / bytesPerPixel))); readDstRowBytes = rowBytes; } else { scratch.reset(tightRowBytes * height); @@ -2353,7 +2353,8 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, } } } else { - SkASSERT(readDst != buffer); SkASSERT(rowBytes != tightRowBytes); + SkASSERT(readDst != buffer); + SkASSERT(rowBytes != tightRowBytes); // copy from readDst to buffer while flipping y // const int halfY = height >> 1; const char* src = reinterpret_cast(readDst); diff --git a/tests/ReadPixelsTest.cpp b/tests/ReadPixelsTest.cpp index 0ab25ca..28a0dc1 100644 --- a/tests/ReadPixelsTest.cpp +++ b/tests/ReadPixelsTest.cpp @@ -224,8 +224,10 @@ enum BitmapInit { kNoPixels_BitmapInit = kFirstBitmapInit, kTight_BitmapInit, kRowBytes_BitmapInit, + kRowBytesOdd_BitmapInit, - kBitmapInitCnt + kLastAligned_BitmapInit = kRowBytes_BitmapInit, + kLast_BitmapInit = kRowBytesOdd_BitmapInit }; static BitmapInit nextBMI(BitmapInit bmi) { @@ -246,13 +248,16 @@ static void init_bitmap(SkBitmap* bitmap, const SkIRect& rect, BitmapInit init, case kRowBytes_BitmapInit: rowBytes = (info.width() + 16) * sizeof(SkPMColor); break; + case kRowBytesOdd_BitmapInit: + rowBytes = (info.width() * sizeof(SkPMColor)) + 3; + break; default: SkASSERT(0); break; } if (alloc) { - bitmap->allocPixels(info); + bitmap->allocPixels(info, rowBytes); } else { bitmap->setInfo(info, rowBytes); } @@ -314,12 +319,13 @@ const SkIRect gReadPixelsTestRects[] = { SkIRect::MakeLTRB(3 * DEV_W / 4, -10, DEV_W + 10, DEV_H + 10), }; -static void test_readpixels(skiatest::Reporter* reporter, SkSurface* surface) { +static void test_readpixels(skiatest::Reporter* reporter, SkSurface* surface, + BitmapInit lastBitmapInit) { SkCanvas* canvas = surface->getCanvas(); fill_src_canvas(canvas); for (size_t rect = 0; rect < SK_ARRAY_COUNT(gReadPixelsTestRects); ++rect) { const SkIRect& srcRect = gReadPixelsTestRects[rect]; - for (BitmapInit bmi = kFirstBitmapInit; bmi < kBitmapInitCnt; bmi = nextBMI(bmi)) { + for (BitmapInit bmi = kFirstBitmapInit; bmi <= lastBitmapInit; bmi = nextBMI(bmi)) { for (size_t c = 0; c < SK_ARRAY_COUNT(gReadPixelsConfigs); ++c) { SkBitmap bmp; init_bitmap(&bmp, srcRect, bmi, @@ -372,7 +378,8 @@ static void test_readpixels(skiatest::Reporter* reporter, SkSurface* surface) { DEF_TEST(ReadPixels, reporter) { const SkImageInfo info = SkImageInfo::MakeN32Premul(DEV_W, DEV_H); SkAutoTUnref surface(SkSurface::NewRaster(info)); - test_readpixels(reporter, surface); + // SW readback fails a premul check when reading back to an unaligned rowbytes. + test_readpixels(reporter, surface, kLastAligned_BitmapInit); } #if SK_SUPPORT_GPU DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadPixels_Gpu, reporter, context) { @@ -387,7 +394,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadPixels_Gpu, reporter, context) { context->textureProvider()->createTexture(desc, false)); SkAutoTUnref surface(SkSurface::NewRenderTargetDirect(surfaceTexture->asRenderTarget())); desc.fFlags = kNone_GrSurfaceFlags; - test_readpixels(reporter, surface); + test_readpixels(reporter, surface, kLast_BitmapInit); } } #endif @@ -397,7 +404,7 @@ static void test_readpixels_texture(skiatest::Reporter* reporter, GrTexture* tex fill_src_texture(texture); for (size_t rect = 0; rect < SK_ARRAY_COUNT(gReadPixelsTestRects); ++rect) { const SkIRect& srcRect = gReadPixelsTestRects[rect]; - for (BitmapInit bmi = kFirstBitmapInit; bmi < kBitmapInitCnt; bmi = nextBMI(bmi)) { + for (BitmapInit bmi = kFirstBitmapInit; bmi <= kLast_BitmapInit; bmi = nextBMI(bmi)) { for (size_t c = 0; c < SK_ARRAY_COUNT(gReadPixelsConfigs); ++c) { SkBitmap bmp; init_bitmap(&bmp, srcRect, bmi, diff --git a/tests/ReadWriteAlphaTest.cpp b/tests/ReadWriteAlphaTest.cpp index 0a84417..4374d9c 100644 --- a/tests/ReadWriteAlphaTest.cpp +++ b/tests/ReadWriteAlphaTest.cpp @@ -17,13 +17,28 @@ static const int X_SIZE = 13; static const int Y_SIZE = 13; -DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, context) { - unsigned char alphaData[X_SIZE][Y_SIZE]; +static void validate_alpha_data(skiatest::Reporter* reporter, int w, int h, const uint8_t* actual, + size_t actualRowBytes, const uint8_t* expected, SkString extraMsg) { + for (int y = 0; y < h; ++y) { + for (int x = 0; x < w; ++x) { + uint8_t a = actual[y * actualRowBytes + x]; + uint8_t e = expected[y * w + x]; + if (e != a) { + ERRORF(reporter, + "Failed alpha readback. Expected: 0x%02x, Got: 0x%02x at (%d,%d), %s", + e, a, x, y, extraMsg.c_str()); + return; + } + } + } +} - memset(alphaData, 0, X_SIZE * Y_SIZE); +DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, context) { + unsigned char alphaData[X_SIZE * Y_SIZE]; bool match; - unsigned char readback[X_SIZE][Y_SIZE]; + + static const size_t kRowBytes[] = {0, X_SIZE, X_SIZE + 1, 2 * X_SIZE - 1}; for (int rt = 0; rt < 2; ++rt) { GrSurfaceDesc desc; @@ -35,6 +50,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, context) { desc.fHeight = Y_SIZE; // We are initializing the texture with zeros here + memset(alphaData, 0, X_SIZE * Y_SIZE); SkAutoTUnref texture( context->textureProvider()->createTexture(desc, false, alphaData, 0)); if (!texture) { @@ -47,61 +63,59 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, context) { // create a distinctive texture for (int y = 0; y < Y_SIZE; ++y) { for (int x = 0; x < X_SIZE; ++x) { - alphaData[x][y] = y*X_SIZE+x; - } - } - - // upload the texture - texture->writePixels(0, 0, desc.fWidth, desc.fHeight, desc.fConfig, - alphaData, 0); - - // clear readback to something non-zero so we can detect readback failures - memset(readback, 0x1, X_SIZE * Y_SIZE); - - // read the texture back - texture->readPixels(0, 0, desc.fWidth, desc.fHeight, desc.fConfig, - readback, 0); - - // make sure the original & read back versions match - match = true; - - for (int y = 0; y < Y_SIZE && match; ++y) { - for (int x = 0; x < X_SIZE && match; ++x) { - if (alphaData[x][y] != readback[x][y]) { - SkDebugf("Failed alpha readback. Expected: 0x%02x, " - "Got: 0x%02x at (%d,%d), rt: %d", alphaData[x][y], readback[x][y], x, - y, rt); - match = false; - } + alphaData[y * X_SIZE + x] = y*X_SIZE+x; } } - // Now try writing on the single channel texture (if we could create as a RT). - if (texture->asRenderTarget()) { - SkSurfaceProps props(SkSurfaceProps::kLegacyFontHost_InitType); - SkAutoTUnref device(SkGpuDevice::Create( - texture->asRenderTarget(), &props, SkGpuDevice::kUninit_InitContents)); - SkCanvas canvas(device); - - SkPaint paint; + for (auto rowBytes : kRowBytes) { + // upload the texture (do per-rowbytes iteration because we may overwrite below). + texture->writePixels(0, 0, desc.fWidth, desc.fHeight, desc.fConfig, + alphaData, 0); - const SkRect rect = SkRect::MakeLTRB(-10, -10, X_SIZE + 10, Y_SIZE + 10); - - paint.setColor(SK_ColorWHITE); - - canvas.drawRect(rect, paint); - - texture->readPixels(0, 0, desc.fWidth, desc.fHeight, desc.fConfig, readback, 0); - - match = true; + size_t nonZeroRowBytes = rowBytes ? rowBytes : X_SIZE; + SkAutoTDeleteArray readback(new uint8_t[nonZeroRowBytes * Y_SIZE]); + // clear readback to something non-zero so we can detect readback failures + memset(readback.get(), 0x1, nonZeroRowBytes * Y_SIZE); - for (int y = 0; y < Y_SIZE && match; ++y) { - for (int x = 0; x < X_SIZE && match; ++x) { - if (0xFF != readback[x][y]) { - ERRORF(reporter, - "Failed alpha readback after clear. Expected: 0xFF, Got: 0x%02x at " - "(%d,%d)", readback[x][y], x, y); - match = false; + // read the texture back + texture->readPixels(0, 0, desc.fWidth, desc.fHeight, desc.fConfig, + readback.get(), rowBytes); + + // make sure the original & read back versions match + SkString msg; + msg.printf("rt:%d, rb:%zd", rt, rowBytes); + validate_alpha_data(reporter, X_SIZE, Y_SIZE, readback.get(), nonZeroRowBytes, + alphaData, msg); + + // Now try writing on the single channel texture (if we could create as a RT). + if (texture->asRenderTarget()) { + SkSurfaceProps props(SkSurfaceProps::kLegacyFontHost_InitType); + SkAutoTUnref device(SkGpuDevice::Create( + texture->asRenderTarget(), &props, SkGpuDevice::kUninit_InitContents)); + SkCanvas canvas(device); + + SkPaint paint; + + const SkRect rect = SkRect::MakeLTRB(-10, -10, X_SIZE + 10, Y_SIZE + 10); + + paint.setColor(SK_ColorWHITE); + + canvas.drawRect(rect, paint); + + memset(readback.get(), 0x1, nonZeroRowBytes * Y_SIZE); + texture->readPixels(0, 0, desc.fWidth, desc.fHeight, desc.fConfig, readback.get(), + rowBytes); + + match = true; + for (int y = 0; y < Y_SIZE && match; ++y) { + for (int x = 0; x < X_SIZE && match; ++x) { + uint8_t rbValue = readback.get()[y * nonZeroRowBytes + x]; + if (0xFF != rbValue) { + ERRORF(reporter, + "Failed alpha readback after clear. Expected: 0xFF, Got: 0x%02x" + " at (%d,%d), rb:%zd", rbValue, x, y, rowBytes); + match = false; + } } } } @@ -114,6 +128,12 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, context) { kSRGBA_8888_GrPixelConfig }; + for (int y = 0; y < Y_SIZE; ++y) { + for (int x = 0; x < X_SIZE; ++x) { + alphaData[y * X_SIZE + x] = y*X_SIZE+x; + } + } + // Attempt to read back just alpha from a RGBA/BGRA texture. Once with a texture-only src and // once with a render target. for (auto cfg : kRGBAConfigs) { @@ -124,11 +144,11 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, context) { desc.fWidth = X_SIZE; desc.fHeight = Y_SIZE; - uint32_t rgbaData[X_SIZE][Y_SIZE]; + uint32_t rgbaData[X_SIZE * Y_SIZE]; // Make the alpha channel of the rgba texture come from alphaData. for (int y = 0; y < Y_SIZE; ++y) { for (int x = 0; x < X_SIZE; ++x) { - rgbaData[x][y] = GrColorPackRGBA(6, 7, 8, alphaData[x][y]); + rgbaData[y * X_SIZE + x] = GrColorPackRGBA(6, 7, 8, alphaData[y * X_SIZE + x]); } } SkAutoTUnref texture( @@ -141,27 +161,22 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, context) { continue; } - // clear readback to something non-zero so we can detect readback failures - memset(readback, 0x0, X_SIZE * Y_SIZE); + for (auto rowBytes : kRowBytes) { + size_t nonZeroRowBytes = rowBytes ? rowBytes : X_SIZE; - // read the texture back - texture->readPixels(0, 0, desc.fWidth, desc.fHeight, kAlpha_8_GrPixelConfig, readback, - 0); - - match = true; - - for (int y = 0; y < Y_SIZE && match; ++y) { - for (int x = 0; x < X_SIZE && match; ++x) { - if (alphaData[x][y] != readback[x][y]) { - texture->readPixels(0, 0, desc.fWidth, desc.fHeight, - kAlpha_8_GrPixelConfig, readback, 0); - ERRORF(reporter, - "Failed alpha readback from cfg %d. Expected: 0x%02x, Got: 0x%02x at" - " (%d,%d), rt:%d", desc.fConfig, alphaData[x][y], readback[x][y], x, - y, rt); - match = false; - } - } + SkAutoTDeleteArray readback(new uint8_t[nonZeroRowBytes * Y_SIZE]); + // Clear so we don't accidentally see values from previous iteration. + memset(readback.get(), 0x0, nonZeroRowBytes * Y_SIZE); + + // read the texture back + texture->readPixels(0, 0, desc.fWidth, desc.fHeight, kAlpha_8_GrPixelConfig, + readback.get(), rowBytes); + + // make sure the original & read back versions match + SkString msg; + msg.printf("rt:%d, rb:%zd", rt, rowBytes); + validate_alpha_data(reporter, X_SIZE, Y_SIZE, readback.get(), nonZeroRowBytes, + alphaData, msg); } } } -- 2.7.4