Support GL_RGB textures and render targets.

Bug= skia:7533

Change-Id: Iba30e90dbf2574368b773bb5cf2ebd5219559717
Reviewed-on: https://skia-review.googlesource.com/108188
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>

1 files changed, 137 insertions, 45 deletions

diff --git a/tests/ReadPixelsTest.cpp b/tests/ReadPixelsTest.cpp
index ccb2ced4fd..867f766d17 100644
--- a/tests/ReadPixelsTest.cpp
+++ b/tests/ReadPixelsTest.cpp
@@ -67,6 +67,7 @@ static SkPMColor get_dst_bmp_init_color(int x, int y, int w) {
     return SkPackARGB32(0xff, r, g , b);
 }
 
+// TODO: Make this consider both ATs
 static SkPMColor convert_to_pmcolor(SkColorType ct, SkAlphaType at, const uint32_t* addr,
                                     bool* doUnpremul) {
     *doUnpremul = (kUnpremul_SkAlphaType == at);
@@ -80,10 +81,12 @@ static SkPMColor convert_to_pmcolor(SkColorType ct, SkAlphaType at, const uint32
             r = static_cast<U8CPU>(c[2]);
             a = static_cast<U8CPU>(c[3]);
             break;
+        case kRGB_888x_SkColorType:  // fallthrough
         case kRGBA_8888_SkColorType:
             r = static_cast<U8CPU>(c[0]);
             g = static_cast<U8CPU>(c[1]);
             b = static_cast<U8CPU>(c[2]);
+            // We set this even when for kRGB_888x because our caller will validate that it is 0xff.
             a = static_cast<U8CPU>(c[3]);
             break;
         default:
@@ -165,16 +168,13 @@ static bool check_read_pixel(SkPMColor a, SkPMColor b, bool didPremulConversion)
 // checks the bitmap contains correct pixels after the readPixels
 // if the bitmap was prefilled with pixels it checks that these weren't
 // overwritten in the area outside the readPixels.
-static bool check_read(skiatest::Reporter* reporter,
-                       const SkBitmap& bitmap,
-                       int x, int y,
-                       bool checkCanvasPixels,
-                       bool checkBitmapPixels,
-                       SkColorType ct,
-                       SkAlphaType at) {
-    SkASSERT(ct == bitmap.colorType() && at == bitmap.alphaType());
+static bool check_read(skiatest::Reporter* reporter, const SkBitmap& bitmap, int x, int y,
+                       bool checkSurfacePixels, bool checkBitmapPixels,
+                       SkAlphaType surfaceAlphaType) {
+    SkAlphaType bmpAT = bitmap.alphaType();
+    SkColorType bmpCT = bitmap.colorType();
     SkASSERT(!bitmap.isNull());
-    SkASSERT(checkCanvasPixels || checkBitmapPixels);
+    SkASSERT(checkSurfacePixels || checkBitmapPixels);
 
     int bw = bitmap.width();
     int bh = bitmap.height();
@@ -184,7 +184,7 @@ static bool check_read(skiatest::Reporter* reporter,
     if (!clippedSrcRect.intersect(srcRect)) {
         clippedSrcRect.setEmpty();
     }
-    if (kAlpha_8_SkColorType == ct) {
+    if (kAlpha_8_SkColorType == bmpCT) {
         for (int by = 0; by < bh; ++by) {
             for (int bx = 0; bx < bw; ++bx) {
                 int devx = bx + srcRect.fLeft;
@@ -192,11 +192,14 @@ static bool check_read(skiatest::Reporter* reporter,
                 const uint8_t* alpha = bitmap.getAddr8(bx, by);
 
                 if (clippedSrcRect.contains(devx, devy)) {
-                    if (checkCanvasPixels) {
-                        uint8_t canvasAlpha = SkGetPackedA32(get_src_color(devx, devy));
-                        if (canvasAlpha != *alpha) {
-                            ERRORF(reporter, "Expected readback alpha (%d, %d) value 0x%02x, got 0x%02x. ",
-                                   bx, by, canvasAlpha, *alpha);
+                    if (checkSurfacePixels) {
+                        uint8_t surfaceAlpha = (surfaceAlphaType == kOpaque_SkAlphaType)
+                                                       ? 0xFF
+                                                       : SkGetPackedA32(get_src_color(devx, devy));
+                        if (surfaceAlpha != *alpha) {
+                            ERRORF(reporter,
+                                   "Expected readback alpha (%d, %d) value 0x%02x, got 0x%02x. ",
+                                   bx, by, surfaceAlpha, *alpha);
                             return false;
                         }
                     }
@@ -220,13 +223,18 @@ static bool check_read(skiatest::Reporter* reporter,
             const uint32_t* pixel = bitmap.getAddr32(bx, by);
 
             if (clippedSrcRect.contains(devx, devy)) {
-                if (checkCanvasPixels) {
-                    SkPMColor canvasPixel = get_src_color(devx, devy);
+                if (checkSurfacePixels) {
+                    SkPMColor surfacePMColor = get_src_color(devx, devy);
+                    if (kOpaque_SkAlphaType == surfaceAlphaType || kOpaque_SkAlphaType == bmpAT) {
+                        surfacePMColor |= 0xFF000000;
+                    }
                     bool didPremul;
-                    SkPMColor pmPixel = convert_to_pmcolor(ct, at, pixel, &didPremul);
-                    if (!check_read_pixel(pmPixel, canvasPixel, didPremul)) {
-                        ERRORF(reporter, "Expected readback pixel (%d, %d) value 0x%08x, got 0x%08x. "
-                               "Readback was unpremul: %d", bx, by, canvasPixel, pmPixel, didPremul);
+                    SkPMColor pmPixel = convert_to_pmcolor(bmpCT, bmpAT, pixel, &didPremul);
+                    if (!check_read_pixel(pmPixel, surfacePMColor, didPremul)) {
+                        ERRORF(reporter,
+                               "Expected readback pixel (%d, %d) value 0x%08x, got 0x%08x. "
+                               "Readback was unpremul: %d",
+                               bx, by, surfacePMColor, pmPixel, didPremul);
                         return false;
                     }
                 }
@@ -288,11 +296,12 @@ static const struct {
     SkColorType fColorType;
     SkAlphaType fAlphaType;
 } gReadPixelsConfigs[] = {
-    { kRGBA_8888_SkColorType,   kPremul_SkAlphaType },
-    { kRGBA_8888_SkColorType,   kUnpremul_SkAlphaType },
-    { kBGRA_8888_SkColorType,   kPremul_SkAlphaType },
-    { kBGRA_8888_SkColorType,   kUnpremul_SkAlphaType },
-    { kAlpha_8_SkColorType,     kPremul_SkAlphaType },
+        {kRGBA_8888_SkColorType, kPremul_SkAlphaType},
+        {kRGBA_8888_SkColorType, kUnpremul_SkAlphaType},
+        {kRGB_888x_SkColorType, kOpaque_SkAlphaType},
+        {kBGRA_8888_SkColorType, kPremul_SkAlphaType},
+        {kBGRA_8888_SkColorType, kUnpremul_SkAlphaType},
+        {kAlpha_8_SkColorType, kPremul_SkAlphaType},
 };
 const SkIRect gReadPixelsTestRects[] = {
     // entire thing
@@ -341,8 +350,58 @@ const SkIRect gReadPixelsTestRects[] = {
     SkIRect::MakeLTRB(3 * DEV_W / 4, -10, DEV_W + 10, DEV_H + 10),
 };
 
+enum class ReadSuccessExpectation {
+    kNo,
+    kMaybe,
+    kYes,
+};
+
+bool check_success_expectation(ReadSuccessExpectation expectation, bool actualSuccess) {
+    switch (expectation) {
+        case ReadSuccessExpectation::kMaybe:
+            return true;
+        case ReadSuccessExpectation::kNo:
+            return !actualSuccess;
+        case ReadSuccessExpectation::kYes:
+            return actualSuccess;
+    }
+    return false;
+}
+
+ReadSuccessExpectation read_should_succeed(const SkIRect& srcRect, const SkImageInfo& dstInfo,
+                                           const SkImageInfo& srcInfo, bool isGPU) {
+    if (!SkIRect::Intersects(srcRect, DEV_RECT)) {
+        return ReadSuccessExpectation::kNo;
+    }
+    if (!SkImageInfoValidConversion(dstInfo, srcInfo)) {
+        return ReadSuccessExpectation::kNo;
+    }
+    if (!isGPU) {
+        return ReadSuccessExpectation::kYes;
+    }
+    // This serves more as documentation of what currently works on the GPU rather than desired
+    // expectations. Once we make GrSurfaceContext color/alpha type aware and clean up some read
+    // pixels code we will make more scenarios work.
+
+    // The GPU code current only does the premul->unpremul conversion, not the reverse.
+    if (srcInfo.alphaType() == kUnpremul_SkAlphaType &&
+        dstInfo.alphaType() == kPremul_SkAlphaType) {
+        return ReadSuccessExpectation::kNo;
+    }
+    // We don't currently require reading alpha-only surfaces to succeed because of some pessimistic
+    // caps decisions and alpha/red complexity in GL.
+    if (SkColorTypeIsAlphaOnly(srcInfo.colorType())) {
+        return ReadSuccessExpectation::kMaybe;
+    }
+    if (!SkColorTypeIsAlwaysOpaque(srcInfo.colorType()) &&
+        SkColorTypeIsAlwaysOpaque(dstInfo.colorType())) {
+        return ReadSuccessExpectation::kNo;
+    }
+    return ReadSuccessExpectation::kYes;
+}
+
 static void test_readpixels(skiatest::Reporter* reporter, const sk_sp<SkSurface>& surface,
-                            BitmapInit lastBitmapInit) {
+                            const SkImageInfo& surfaceInfo, BitmapInit lastBitmapInit) {
     SkCanvas* canvas = surface->getCanvas();
     fill_src_canvas(canvas);
     for (size_t rect = 0; rect < SK_ARRAY_COUNT(gReadPixelsTestRects); ++rect) {
@@ -363,18 +422,21 @@ static void test_readpixels(skiatest::Reporter* reporter, const sk_sp<SkSurface>
                 bool success = surface->readPixels(bmp, srcRect.fLeft, srcRect.fTop);
                 uint32_t idAfter = surface->generationID();
 
-                // we expect to succeed when the read isn't fully clipped
-                // out.
-                bool expectSuccess = SkIRect::Intersects(srcRect, DEV_RECT);
+                // we expect to succeed when the read isn't fully clipped out and the infos are
+                // compatible.
+                bool isGPU = SkToBool(surface->getCanvas()->getGrContext());
+                auto expectSuccess = read_should_succeed(srcRect, bmp.info(), surfaceInfo, isGPU);
                 // determine whether we expected the read to succeed.
-                REPORTER_ASSERT(reporter, success == expectSuccess);
+                REPORTER_ASSERT(reporter, check_success_expectation(expectSuccess, success),
+                                "Read succeed=%d unexpectedly, src ct/at: %d/%d, dst ct/at: %d/%d",
+                                success, surfaceInfo.colorType(), surfaceInfo.alphaType(),
+                                bmp.info().colorType(), bmp.info().alphaType());
                 // read pixels should never change the gen id
                 REPORTER_ASSERT(reporter, idBefore == idAfter);
 
                 if (success || startsWithPixels) {
-                    check_read(reporter, bmp, srcRect.fLeft, srcRect.fTop,
-                               success, startsWithPixels,
-                               gReadPixelsConfigs[c].fColorType, gReadPixelsConfigs[c].fAlphaType);
+                    check_read(reporter, bmp, srcRect.fLeft, srcRect.fTop, success,
+                               startsWithPixels, surfaceInfo.alphaType());
                 } else {
                     // if we had no pixels beforehand and the readPixels
                     // failed then our bitmap should still not have pixels
@@ -384,11 +446,12 @@ static void test_readpixels(skiatest::Reporter* reporter, const sk_sp<SkSurface>
         }
     }
 }
+
 DEF_TEST(ReadPixels, reporter) {
     const SkImageInfo info = SkImageInfo::MakeN32Premul(DEV_W, DEV_H);
     auto surface(SkSurface::MakeRaster(info));
     // SW readback fails a premul check when reading back to an unaligned rowbytes.
-    test_readpixels(reporter, surface, kLastAligned_BitmapInit);
+    test_readpixels(reporter, surface, info, kLastAligned_BitmapInit);
 }
 #if SK_SUPPORT_GPU
 DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadPixels_Gpu, reporter, ctxInfo) {
@@ -399,18 +462,29 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadPixels_Gpu, reporter, ctxInfo) {
         return;
     }
 
-    const SkImageInfo ii = SkImageInfo::MakeN32Premul(DEV_W, DEV_H);
-    for (auto& origin : {kBottomLeft_GrSurfaceOrigin, kTopLeft_GrSurfaceOrigin}) {
-        sk_sp<SkSurface> surface(SkSurface::MakeRenderTarget(ctxInfo.grContext(), SkBudgeted::kNo,
-                                                             ii, 0, origin, nullptr));
-        test_readpixels(reporter, surface, kLast_BitmapInit);
+    static const SkImageInfo kImageInfos[] = {
+            SkImageInfo::Make(DEV_W, DEV_H, kRGBA_8888_SkColorType, kPremul_SkAlphaType),
+            SkImageInfo::Make(DEV_W, DEV_H, kBGRA_8888_SkColorType, kPremul_SkAlphaType),
+            SkImageInfo::Make(DEV_W, DEV_H, kRGB_888x_SkColorType, kOpaque_SkAlphaType),
+            SkImageInfo::Make(DEV_W, DEV_H, kAlpha_8_SkColorType, kPremul_SkAlphaType),
+    };
+    for (const auto& ii : kImageInfos) {
+        for (auto& origin : {kBottomLeft_GrSurfaceOrigin, kTopLeft_GrSurfaceOrigin}) {
+            sk_sp<SkSurface> surface(SkSurface::MakeRenderTarget(
+                    ctxInfo.grContext(), SkBudgeted::kNo, ii, 0, origin, nullptr));
+            if (!surface) {
+                continue;
+            }
+            test_readpixels(reporter, surface, ii, kLast_BitmapInit);
+        }
     }
 }
 #endif
 
 #if SK_SUPPORT_GPU
 static void test_readpixels_texture(skiatest::Reporter* reporter,
-                                    sk_sp<GrSurfaceContext> sContext) {
+                                    sk_sp<GrSurfaceContext> sContext,
+                                    const SkImageInfo& surfaceInfo) {
     for (size_t rect = 0; rect < SK_ARRAY_COUNT(gReadPixelsTestRects); ++rect) {
         const SkIRect& srcRect = gReadPixelsTestRects[rect];
         for (BitmapInit bmi = kFirstBitmapInit; bmi <= kLast_BitmapInit; bmi = nextBMI(bmi)) {
@@ -426,15 +500,32 @@ static void test_readpixels_texture(skiatest::Reporter* reporter,
                 if (startsWithPixels) {
                     fill_dst_bmp_with_init_data(&bmp);
                     uint32_t flags = 0;
+                    // TODO: These two hacks can go away when the surface context knows the alpha
+                    // type.
+                    // Tell the read to perform an unpremul step since it doesn't know alpha type.
                     if (gReadPixelsConfigs[c].fAlphaType == kUnpremul_SkAlphaType) {
                         flags = GrContextPriv::kUnpremul_PixelOpsFlag;
                     }
+                    // The surface context doesn't know that the src is opaque. We don't support
+                    // converting non-opaque data to opaque during a read.
+                    if (bmp.alphaType() == kOpaque_SkAlphaType &&
+                        surfaceInfo.alphaType() != kOpaque_SkAlphaType) {
+                        continue;
+                    }
                     bool success = sContext->readPixels(bmp.info(), bmp.getPixels(),
                                                         bmp.rowBytes(),
                                                         srcRect.fLeft, srcRect.fTop, flags);
-                    check_read(reporter, bmp, srcRect.fLeft, srcRect.fTop,
-                               success, true,
-                               gReadPixelsConfigs[c].fColorType, gReadPixelsConfigs[c].fAlphaType);
+                    auto expectSuccess =
+                            read_should_succeed(srcRect, bmp.info(), surfaceInfo, true);
+                    REPORTER_ASSERT(
+                            reporter, check_success_expectation(expectSuccess, success),
+                            "Read succeed=%d unexpectedly, src ct/at: %d/%d, dst ct/at: %d/%d",
+                            success, surfaceInfo.colorType(), surfaceInfo.alphaType(),
+                            bmp.info().colorType(), bmp.info().alphaType());
+                    if (success) {
+                        check_read(reporter, bmp, srcRect.fLeft, srcRect.fTop, success, true,
+                                   surfaceInfo.alphaType());
+                    }
                 }
             }
         }
@@ -460,7 +551,8 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadPixels_Texture, reporter, ctxInfo) {
                     bmp.rowBytes());
             sk_sp<GrSurfaceContext> sContext = context->contextPriv().makeWrappedSurfaceContext(
                                                                                 std::move(proxy));
-            test_readpixels_texture(reporter, std::move(sContext));
+            auto info = SkImageInfo::Make(DEV_W, DEV_H, kN32_SkColorType, kPremul_SkAlphaType);
+            test_readpixels_texture(reporter, std::move(sContext), info);
         }
     }
 }