From e216948d63958e75a2321096bda5598bd1608711 Mon Sep 17 00:00:00 2001 From: Derek Sollenberger Date: Tue, 20 Nov 2018 10:57:20 -0500 Subject: [PATCH] Cleanup of Bitmap.h entry points. All Bitmap constructors have been made private and the only way to create one is through the allocate or createFrom factories. SkColorSpace is now explicitly passed in to all the factories and is no longer assumed to be sRGB. Test: atest CtsGraphicsTestCases Change-Id: I92c1c5c59df6de7fdd90e9504a2c2717cb854588 --- core/jni/android/graphics/Bitmap.cpp | 18 ++-- core/jni/android/graphics/Graphics.cpp | 30 ------- core/jni/android/graphics/GraphicsJNI.h | 3 - core/jni/android_view_ThreadedRenderer.cpp | 10 +-- libs/hwui/HardwareBitmapUploader.cpp | 3 +- libs/hwui/hwui/Bitmap.cpp | 89 ++++++++++++------- libs/hwui/hwui/Bitmap.h | 44 +++++---- .../hwui/pipeline/skia/SkiaVulkanPipeline.cpp | 2 +- .../scenes/HwBitmapInCompositeShader.cpp | 2 +- libs/hwui/utils/Color.h | 3 +- 10 files changed, 102 insertions(+), 102 deletions(-) diff --git a/core/jni/android/graphics/Bitmap.cpp b/core/jni/android/graphics/Bitmap.cpp index c32de0a5737e0..12ca78a7ce92e 100755 --- a/core/jni/android/graphics/Bitmap.cpp +++ b/core/jni/android/graphics/Bitmap.cpp @@ -725,9 +725,10 @@ static jobject Bitmap_createFromParcel(JNIEnv* env, jobject, jobject parcel) { return NULL; } - // Map the pixels in place and take ownership of the ashmem region. - nativeBitmap = sk_sp(GraphicsJNI::mapAshmemBitmap(env, bitmap.get(), - dupFd, const_cast(blob.data()), size, !isMutable)); + // Map the pixels in place and take ownership of the ashmem region. We must also respect the + // rowBytes value already set on the bitmap instead of attempting to compute our own. + nativeBitmap = Bitmap::createFrom(bitmap->info(), bitmap->rowBytes(), dupFd, + const_cast(blob.data()), size, !isMutable); if (!nativeBitmap) { close(dupFd); blob.release(); @@ -1097,21 +1098,20 @@ static jobject Bitmap_copyPreserveInternalConfig(JNIEnv* env, jobject, jlong bit SkBitmap src; hwuiBitmap.getSkBitmap(&src); - SkBitmap result; - HeapAllocator allocator; - if (!bitmapCopyTo(&result, hwuiBitmap.info().colorType(), src, &allocator)) { + if (src.pixelRef() == nullptr) { doThrowRE(env, "Could not copy a hardware bitmap."); return NULL; } - return createBitmap(env, allocator.getStorageObjAndReset(), getPremulBitmapCreateFlags(false)); + + sk_sp bitmap = Bitmap::createFrom(src.info(), *src.pixelRef()); + return createBitmap(env, bitmap.release(), getPremulBitmapCreateFlags(false)); } static jobject Bitmap_createHardwareBitmap(JNIEnv* env, jobject, jobject graphicBuffer) { sp buffer(graphicBufferForJavaObject(env, graphicBuffer)); - // Bitmap::createFrom currently assumes SRGB color space for RGBA images. // To support any color space, we need to pass an additional ColorSpace argument to // java Bitmap.createHardwareBitmap. - sk_sp bitmap = Bitmap::createFrom(buffer); + sk_sp bitmap = Bitmap::createFrom(buffer, SkColorSpace::MakeSRGB()); if (!bitmap.get()) { ALOGW("failed to create hardware bitmap from graphic buffer"); return NULL; diff --git a/core/jni/android/graphics/Graphics.cpp b/core/jni/android/graphics/Graphics.cpp index 26af15e79e2dc..67d0c8aced618 100644 --- a/core/jni/android/graphics/Graphics.cpp +++ b/core/jni/android/graphics/Graphics.cpp @@ -424,36 +424,6 @@ jobject GraphicsJNI::createRegion(JNIEnv* env, SkRegion* region) /////////////////////////////////////////////////////////////////////////////// -android::Bitmap* GraphicsJNI::mapAshmemBitmap(JNIEnv* env, SkBitmap* bitmap, - int fd, void* addr, size_t size, bool readOnly) { - const SkImageInfo& info = bitmap->info(); - if (info.colorType() == kUnknown_SkColorType) { - doThrowIAE(env, "unknown bitmap configuration"); - return nullptr; - } - - if (!addr) { - // Map existing ashmem region if not already mapped. - int flags = readOnly ? (PROT_READ) : (PROT_READ | PROT_WRITE); - size = ashmem_get_size_region(fd); - addr = mmap(NULL, size, flags, MAP_SHARED, fd, 0); - if (addr == MAP_FAILED) { - return nullptr; - } - } - - // we must respect the rowBytes value already set on the bitmap instead of - // attempting to compute our own. - const size_t rowBytes = bitmap->rowBytes(); - - auto wrapper = new android::Bitmap(addr, fd, size, info, rowBytes); - wrapper->getSkBitmap(bitmap); - if (readOnly) { - bitmap->pixelRef()->setImmutable(); - } - return wrapper; -} - SkColorSpaceTransferFn GraphicsJNI::getNativeTransferParameters(JNIEnv* env, jobject transferParams) { SkColorSpaceTransferFn p; p.fA = (float) env->GetDoubleField(transferParams, gTransferParams_aFieldID); diff --git a/core/jni/android/graphics/GraphicsJNI.h b/core/jni/android/graphics/GraphicsJNI.h index cee3c46dd67f3..b0bd68336e08e 100644 --- a/core/jni/android/graphics/GraphicsJNI.h +++ b/core/jni/android/graphics/GraphicsJNI.h @@ -85,9 +85,6 @@ public: static jobject createBitmapRegionDecoder(JNIEnv* env, SkBitmapRegionDecoder* bitmap); - static android::Bitmap* mapAshmemBitmap(JNIEnv* env, SkBitmap* bitmap, - int fd, void* addr, size_t size, bool readOnly); - /** * Given a bitmap we natively allocate a memory block to store the contents * of that bitmap. The memory is then attached to the bitmap via an diff --git a/core/jni/android_view_ThreadedRenderer.cpp b/core/jni/android_view_ThreadedRenderer.cpp index 702741eb813cd..5a8ab3c1bdc4a 100644 --- a/core/jni/android_view_ThreadedRenderer.cpp +++ b/core/jni/android_view_ThreadedRenderer.cpp @@ -31,8 +31,6 @@ #include #include -#include -#include #include #include @@ -58,6 +56,7 @@ #include #include #include +#include namespace android { @@ -1011,10 +1010,9 @@ static jobject android_view_ThreadedRenderer_createHardwareBitmapFromRenderNode( buffer->getWidth(), buffer->getHeight(), width, height); // Continue I guess? } - sk_sp bitmap = Bitmap::createFrom(buffer); - // Bitmap::createFrom currently can only attach to a GraphicBuffer with PIXEL_FORMAT_RGBA_8888 - // format and SRGB color space. - // To support any color space, we could extract it from BufferItem and pass it to Bitmap. + + sk_sp cs = uirenderer::DataSpaceToColorSpace(bufferItem.mDataSpace); + sk_sp bitmap = Bitmap::createFrom(buffer, cs); return bitmap::createBitmap(env, bitmap.release(), android::bitmap::kBitmapCreateFlag_Premultiplied); } diff --git a/libs/hwui/HardwareBitmapUploader.cpp b/libs/hwui/HardwareBitmapUploader.cpp index a97c12cad9fd4..b9860ada18fcc 100644 --- a/libs/hwui/HardwareBitmapUploader.cpp +++ b/libs/hwui/HardwareBitmapUploader.cpp @@ -253,7 +253,8 @@ sk_sp HardwareBitmapUploader::allocateHardwareBitmap(const SkBitmap& sou eglDestroySyncKHR(display, fence); } - return sk_sp(new Bitmap(buffer.get(), bitmap.info(), Bitmap::computePalette(bitmap))); + return Bitmap::createFrom(buffer.get(), bitmap.refColorSpace(), bitmap.alphaType(), + Bitmap::computePalette(bitmap)); } } // namespace android::uirenderer diff --git a/libs/hwui/hwui/Bitmap.cpp b/libs/hwui/hwui/Bitmap.cpp index 6c77f9ee18451..6e0258c9ecb2e 100644 --- a/libs/hwui/hwui/Bitmap.cpp +++ b/libs/hwui/hwui/Bitmap.cpp @@ -75,31 +75,6 @@ sk_sp Bitmap::allocateAshmemBitmap(SkBitmap* bitmap) { return allocateBitmap(bitmap, &Bitmap::allocateAshmemBitmap); } -static sk_sp allocateHeapBitmap(size_t size, const SkImageInfo& info, size_t rowBytes) { - void* addr = calloc(size, 1); - if (!addr) { - return nullptr; - } - return sk_sp(new Bitmap(addr, size, info, rowBytes)); -} - -sk_sp Bitmap::allocateHardwareBitmap(SkBitmap& bitmap) { - return uirenderer::HardwareBitmapUploader::allocateHardwareBitmap(bitmap); -} - -sk_sp Bitmap::allocateHeapBitmap(SkBitmap* bitmap) { - return allocateBitmap(bitmap, &android::allocateHeapBitmap); -} - -sk_sp Bitmap::allocateHeapBitmap(const SkImageInfo& info) { - size_t size; - if (!computeAllocationSize(info.minRowBytes(), info.height(), &size)) { - LOG_ALWAYS_FATAL("trying to allocate too large bitmap"); - return nullptr; - } - return android::allocateHeapBitmap(size, info, info.minRowBytes()); -} - sk_sp Bitmap::allocateAshmemBitmap(size_t size, const SkImageInfo& info, size_t rowBytes) { // Create new ashmem region with read/write priv int fd = ashmem_create_region("bitmap", size); @@ -121,6 +96,31 @@ sk_sp Bitmap::allocateAshmemBitmap(size_t size, const SkImageInfo& info, return sk_sp(new Bitmap(addr, fd, size, info, rowBytes)); } +sk_sp Bitmap::allocateHardwareBitmap(const SkBitmap& bitmap) { + return uirenderer::HardwareBitmapUploader::allocateHardwareBitmap(bitmap); +} + +sk_sp Bitmap::allocateHeapBitmap(SkBitmap* bitmap) { + return allocateBitmap(bitmap, &Bitmap::allocateHeapBitmap); +} + +sk_sp Bitmap::allocateHeapBitmap(const SkImageInfo& info) { + size_t size; + if (!computeAllocationSize(info.minRowBytes(), info.height(), &size)) { + LOG_ALWAYS_FATAL("trying to allocate too large bitmap"); + return nullptr; + } + return allocateHeapBitmap(size, info, info.minRowBytes()); +} + +sk_sp Bitmap::allocateHeapBitmap(size_t size, const SkImageInfo& info, size_t rowBytes) { + void* addr = calloc(size, 1); + if (!addr) { + return nullptr; + } + return sk_sp(new Bitmap(addr, size, info, rowBytes)); +} + void FreePixelRef(void* addr, void* context) { auto pixelRef = (SkPixelRef*)context; pixelRef->unref(); @@ -132,17 +132,38 @@ sk_sp Bitmap::createFrom(const SkImageInfo& info, SkPixelRef& pixelRef) pixelRef.rowBytes())); } -sk_sp Bitmap::createFrom(sp graphicBuffer) { - return createFrom(graphicBuffer, SkColorSpace::MakeSRGB()); + +sk_sp Bitmap::createFrom(sp graphicBuffer, sk_sp colorSpace, + SkAlphaType alphaType, BitmapPalette palette) { + // As we will be effectively texture-sampling the buffer (using either EGL or Vulkan), we can + // view the format as RGBA8888. + SkImageInfo info = SkImageInfo::Make(graphicBuffer->getWidth(), graphicBuffer->getHeight(), + kRGBA_8888_SkColorType, alphaType, colorSpace); + return sk_sp(new Bitmap(graphicBuffer.get(), info, palette)); } -sk_sp Bitmap::createFrom(sp graphicBuffer, sk_sp colorSpace) { - // As we will be effectively texture-sampling the buffer (using either EGL or Vulkan), we can - // view the colorspace as RGBA8888. - SkImageInfo info = SkImageInfo::Make(graphicBuffer->getWidth(), graphicBuffer->getHeight(), - kRGBA_8888_SkColorType, kPremul_SkAlphaType, - colorSpace); - return sk_sp(new Bitmap(graphicBuffer.get(), info)); +sk_sp Bitmap::createFrom(const SkImageInfo& info, size_t rowBytes, int fd, void* addr, + size_t size, bool readOnly) { + if (info.colorType() == kUnknown_SkColorType) { + LOG_ALWAYS_FATAL("unknown bitmap configuration"); + return nullptr; + } + + if (!addr) { + // Map existing ashmem region if not already mapped. + int flags = readOnly ? (PROT_READ) : (PROT_READ | PROT_WRITE); + size = ashmem_get_size_region(fd); + addr = mmap(NULL, size, flags, MAP_SHARED, fd, 0); + if (addr == MAP_FAILED) { + return nullptr; + } + } + + sk_sp bitmap(new Bitmap(addr, fd, size, info, rowBytes)); + if (readOnly) { + bitmap->setImmutable(); + } + return bitmap; } void Bitmap::setColorSpace(sk_sp colorSpace) { diff --git a/libs/hwui/hwui/Bitmap.h b/libs/hwui/hwui/Bitmap.h index d446377ec1d9d..2138040d9690e 100644 --- a/libs/hwui/hwui/Bitmap.h +++ b/libs/hwui/hwui/Bitmap.h @@ -54,28 +54,31 @@ typedef void (*FreeFunc)(void* addr, void* context); class ANDROID_API Bitmap : public SkPixelRef { public: + /* The allocate factories not only construct the Bitmap object but also allocate the + * backing store whose type is determined by the specific method that is called. + * + * The factories that accept SkBitmap* as a param will modify those params by + * installing the returned bitmap as their SkPixelRef. + * + * The factories that accept const SkBitmap& as a param will copy the contents of the + * provided bitmap into the newly allocated buffer. + */ + static sk_sp allocateAshmemBitmap(SkBitmap* bitmap); + static sk_sp allocateHardwareBitmap(const SkBitmap& bitmap); static sk_sp allocateHeapBitmap(SkBitmap* bitmap); static sk_sp allocateHeapBitmap(const SkImageInfo& info); - static sk_sp allocateHardwareBitmap(SkBitmap& bitmap); - - static sk_sp allocateAshmemBitmap(SkBitmap* bitmap); - static sk_sp allocateAshmemBitmap(size_t allocSize, const SkImageInfo& info, - size_t rowBytes); - - static sk_sp createFrom(sp graphicBuffer); + /* The createFrom factories construct a new Bitmap object by wrapping the already allocated + * memory that is provided as an input param. + */ static sk_sp createFrom(sp graphicBuffer, - sk_sp colorSpace); - + sk_sp colorSpace, + SkAlphaType alphaType = kPremul_SkAlphaType, + BitmapPalette palette = BitmapPalette::Unknown); + static sk_sp createFrom(const SkImageInfo& info, size_t rowBytes, int fd, void* addr, + size_t size, bool readOnly); static sk_sp createFrom(const SkImageInfo&, SkPixelRef&); - Bitmap(void* address, size_t allocSize, const SkImageInfo& info, size_t rowBytes); - Bitmap(void* address, void* context, FreeFunc freeFunc, const SkImageInfo& info, - size_t rowBytes); - Bitmap(void* address, int fd, size_t mappedSize, const SkImageInfo& info, size_t rowBytes); - Bitmap(GraphicBuffer* buffer, const SkImageInfo& info, - BitmapPalette palette = BitmapPalette::Unknown); - int rowBytesAsPixels() const { return rowBytes() >> mInfo.shiftPerPixel(); } void reconfigure(const SkImageInfo& info, size_t rowBytes); @@ -123,6 +126,15 @@ public: } private: + static sk_sp allocateAshmemBitmap(size_t size, const SkImageInfo& i, size_t rowBytes); + static sk_sp allocateHeapBitmap(size_t size, const SkImageInfo& i, size_t rowBytes); + + Bitmap(void* address, size_t allocSize, const SkImageInfo& info, size_t rowBytes); + Bitmap(void* address, void* context, FreeFunc freeFunc, const SkImageInfo& info, + size_t rowBytes); + Bitmap(void* address, int fd, size_t mappedSize, const SkImageInfo& info, size_t rowBytes); + Bitmap(GraphicBuffer* buffer, const SkImageInfo& info, BitmapPalette palette); + virtual ~Bitmap(); void* getStorage() const; diff --git a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp index c67134cddb8a1..1d3a244630576 100644 --- a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp +++ b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp @@ -158,7 +158,7 @@ sk_sp SkiaVulkanPipeline::allocateHardwareBitmap(renderthread::RenderThr ALOGW("SkiaVulkanPipeline::allocateHardwareBitmap() failed in GraphicBuffer.create()"); return nullptr; } - return sk_sp(new Bitmap(buffer.get(), skBitmap.info())); + return Bitmap::createFrom(buffer, skBitmap.refColorSpace()); } } /* namespace skiapipeline */ diff --git a/libs/hwui/tests/common/scenes/HwBitmapInCompositeShader.cpp b/libs/hwui/tests/common/scenes/HwBitmapInCompositeShader.cpp index 448408d19eb13..ec81f629ee457 100644 --- a/libs/hwui/tests/common/scenes/HwBitmapInCompositeShader.cpp +++ b/libs/hwui/tests/common/scenes/HwBitmapInCompositeShader.cpp @@ -50,7 +50,7 @@ public: pixels[4000 + 4 * i + 3] = 255; } buffer->unlock(); - sk_sp hardwareBitmap(Bitmap::createFrom(buffer)); + sk_sp hardwareBitmap(Bitmap::createFrom(buffer, SkColorSpace::MakeSRGB())); sk_sp hardwareShader(createBitmapShader(*hardwareBitmap)); SkPoint center; diff --git a/libs/hwui/utils/Color.h b/libs/hwui/utils/Color.h index 4daccda78e234..4473ce632b1bb 100644 --- a/libs/hwui/utils/Color.h +++ b/libs/hwui/utils/Color.h @@ -17,6 +17,7 @@ #define COLOR_H #include +#include #include #include @@ -117,7 +118,7 @@ bool transferFunctionCloseToSRGB(const SkColorSpace* colorSpace); android::PixelFormat ColorTypeToPixelFormat(SkColorType colorType); -sk_sp DataSpaceToColorSpace(android_dataspace dataspace); +ANDROID_API sk_sp DataSpaceToColorSpace(android_dataspace dataspace); struct Lab { float L;