From 7d940ba43d15836ccf32f373c778eebffacf1f5a Mon Sep 17 00:00:00 2001 From: Leon Scroggins III Date: Wed, 4 Apr 2018 16:19:33 -0400 Subject: [PATCH] Never scale nine-patches in ImageDecoder Bug: 76448902 Bug: 70889348 Test: Manual + CtsThemeHostTestCases (Ica5e7e81848c3880accee922ee6f1cc9e26262ca) Scaling a nine-patch requires scaling its divs. When the scale factor is not an integer, we have to round. This gets out of sync with the way the decoder scaled the image, resulting in stretching or keeping fixed the wrong portions of the image. Making this worse, when we scale down, we end up with divs colliding with each other, and we have to arbitrarily adjust them further so they do not collide. NinePatchDrawable and the drawing code already know how to handle drawing from the originally-sized image and do a better job stretching appropriately, so allow them to do their job. We already do something similar for Bitmaps created by ImageDecoder on apps targeting P and above - instead of scaling them up, we allow the BitmapDrawable's scaling code to handle density differences. We preserved the old behavior (scale up) on apps targeting pre-P because those apps may rely on the size of the Bitmap contained in a BitmapDrawable without accounting for its density (see Bug: 74061412). But that is not an issue for NinePatchDrawables, which do not allow peeking at their internal Bitmaps. Rewrite ImageDecoder.computeDensity. There is no need for it to be static, since it takes an ImageDecoder as a parameter and reads its fields, including the new field mIsNinePatch. Set mIsNinePatch in the constructor to avoid another down call into native. Split up the conditions that result in returning srcDensity without calling setTargetSize for clarity. Remove ImageDecoder constructor from the graylist. It was accidentally added due to the fact that it is called transitively from public APIs. Change-Id: I3c5ddd67f3352c991515f30ce1c477c9a608833f --- config/hiddenapi-light-greylist.txt | 1 - core/jni/android/graphics/ImageDecoder.cpp | 22 ++++------ .../java/android/graphics/ImageDecoder.java | 42 +++++++++++++------ 3 files changed, 39 insertions(+), 26 deletions(-) diff --git a/config/hiddenapi-light-greylist.txt b/config/hiddenapi-light-greylist.txt index 015c4b664bc32..19ea8d62af4b0 100644 --- a/config/hiddenapi-light-greylist.txt +++ b/config/hiddenapi-light-greylist.txt @@ -760,7 +760,6 @@ Landroid/graphics/GraphicBuffer;->createFromExisting(IIIIJ)Landroid/graphics/Gra Landroid/graphics/GraphicBuffer;->CREATOR:Landroid/os/Parcelable$Creator; Landroid/graphics/GraphicBuffer;->(IIIIJ)V Landroid/graphics/GraphicBuffer;->mNativeObject:J -Landroid/graphics/ImageDecoder;->(JIIZ)V Landroid/graphics/ImageDecoder;->postProcessAndRelease(Landroid/graphics/Canvas;)I Landroid/graphics/LinearGradient;->mColors:[I Landroid/graphics/Matrix;->native_instance:J diff --git a/core/jni/android/graphics/ImageDecoder.cpp b/core/jni/android/graphics/ImageDecoder.cpp index 825b7a0a98845..3ea6049855002 100644 --- a/core/jni/android/graphics/ImageDecoder.cpp +++ b/core/jni/android/graphics/ImageDecoder.cpp @@ -116,9 +116,10 @@ static jobject native_create(JNIEnv* env, std::unique_ptr stream, jobj const auto& info = decoder->mCodec->getInfo(); const int width = info.width(); const int height = info.height(); + const bool isNinePatch = decoder->mPeeker->mPatch != nullptr; return env->NewObject(gImageDecoder_class, gImageDecoder_constructorMethodID, reinterpret_cast(decoder.release()), width, height, - animated); + animated, isNinePatch); } static jobject ImageDecoder_nCreateFd(JNIEnv* env, jobject /*clazz*/, @@ -332,13 +333,6 @@ static jobject ImageDecoder_nDecodeBitmap(JNIEnv* env, jobject /*clazz*/, jlong } } - float scaleX = 1.0f; - float scaleY = 1.0f; - if (scale) { - scaleX = (float) desiredWidth / decodeInfo.width(); - scaleY = (float) desiredHeight / decodeInfo.height(); - } - jbyteArray ninePatchChunk = nullptr; jobject ninePatchInsets = nullptr; @@ -346,9 +340,6 @@ static jobject ImageDecoder_nDecodeBitmap(JNIEnv* env, jobject /*clazz*/, jlong if (!jpostProcess) { // FIXME: Share more code with BitmapFactory.cpp. if (decoder->mPeeker->mPatch != nullptr) { - if (scale) { - decoder->mPeeker->scale(scaleX, scaleY, desiredWidth, desiredHeight); - } size_t ninePatchArraySize = decoder->mPeeker->mPatch->serializedSize(); ninePatchChunk = env->NewByteArray(ninePatchArraySize); if (ninePatchChunk == nullptr) { @@ -408,7 +399,12 @@ static jobject ImageDecoder_nDecodeBitmap(JNIEnv* env, jobject /*clazz*/, jlong SkCanvas canvas(scaledBm, SkCanvas::ColorBehavior::kLegacy); canvas.translate(translateX, translateY); - canvas.scale(scaleX, scaleY); + if (scale) { + float scaleX = (float) desiredWidth / decodeInfo.width(); + float scaleY = (float) desiredHeight / decodeInfo.height(); + canvas.scale(scaleX, scaleY); + } + canvas.drawBitmap(bm, 0.0f, 0.0f, &paint); bm.swap(scaledBm); @@ -532,7 +528,7 @@ static const JNINativeMethod gImageDecoderMethods[] = { int register_android_graphics_ImageDecoder(JNIEnv* env) { gImageDecoder_class = MakeGlobalRefOrDie(env, FindClassOrDie(env, "android/graphics/ImageDecoder")); - gImageDecoder_constructorMethodID = GetMethodIDOrDie(env, gImageDecoder_class, "", "(JIIZ)V"); + gImageDecoder_constructorMethodID = GetMethodIDOrDie(env, gImageDecoder_class, "", "(JIIZZ)V"); gImageDecoder_postProcessMethodID = GetMethodIDOrDie(env, gImageDecoder_class, "postProcessAndRelease", "(Landroid/graphics/Canvas;)I"); gSize_class = MakeGlobalRefOrDie(env, FindClassOrDie(env, "android/util/Size")); diff --git a/graphics/java/android/graphics/ImageDecoder.java b/graphics/java/android/graphics/ImageDecoder.java index ecd443c25ae3d..098f10003b8a5 100644 --- a/graphics/java/android/graphics/ImageDecoder.java +++ b/graphics/java/android/graphics/ImageDecoder.java @@ -748,6 +748,7 @@ public final class ImageDecoder implements AutoCloseable { private final int mWidth; private final int mHeight; private final boolean mAnimated; + private final boolean mIsNinePatch; private int mDesiredWidth; private int mDesiredHeight; @@ -778,13 +779,14 @@ public final class ImageDecoder implements AutoCloseable { */ @SuppressWarnings("unused") private ImageDecoder(long nativePtr, int width, int height, - boolean animated) { + boolean animated, boolean isNinePatch) { mNativePtr = nativePtr; mWidth = width; mHeight = height; mDesiredWidth = width; mDesiredHeight = height; mAnimated = animated; + mIsNinePatch = isNinePatch; mCloseGuard.open("close"); } @@ -1665,7 +1667,7 @@ public final class ImageDecoder implements AutoCloseable { // this call potentially manipulates the decoder so it must be performed prior to // decoding the bitmap and after decode set the density on the resulting bitmap - final int srcDensity = computeDensity(src, decoder); + final int srcDensity = decoder.computeDensity(src); if (decoder.mAnimated) { // AnimatedImageDrawable calls postProcessAndRelease only if // mPostProcessor exists. @@ -1755,7 +1757,7 @@ public final class ImageDecoder implements AutoCloseable { // this call potentially manipulates the decoder so it must be performed prior to // decoding the bitmap - final int srcDensity = computeDensity(src, decoder); + final int srcDensity = decoder.computeDensity(src); Bitmap bm = decoder.decodeBitmapInternal(); bm.setDensity(srcDensity); @@ -1772,12 +1774,26 @@ public final class ImageDecoder implements AutoCloseable { } // This method may modify the decoder so it must be called prior to performing the decode - private static int computeDensity(@NonNull Source src, @NonNull ImageDecoder decoder) { + private int computeDensity(@NonNull Source src) { // if the caller changed the size then we treat the density as unknown - if (decoder.requestedResize()) { + if (this.requestedResize()) { return Bitmap.DENSITY_NONE; } + final int srcDensity = src.getDensity(); + if (srcDensity == Bitmap.DENSITY_NONE) { + return srcDensity; + } + + // Scaling up nine-patch divs is imprecise and is better handled + // at draw time. An app won't be relying on the internal Bitmap's + // size, so it is safe to let NinePatchDrawable handle scaling. + // mPostProcessor disables nine-patching, so behave normally if + // it is present. + if (mIsNinePatch && mPostProcessor == null) { + return srcDensity; + } + // Special stuff for compatibility mode: if the target density is not // the same as the display density, but the resource -is- the same as // the display density, then don't scale it down to the target density. @@ -1786,23 +1802,25 @@ public final class ImageDecoder implements AutoCloseable { // to the compatibility density only to have them scaled back up when // drawn to the screen. Resources res = src.getResources(); - final int srcDensity = src.getDensity(); if (res != null && res.getDisplayMetrics().noncompatDensityDpi == srcDensity) { return srcDensity; } + final int dstDensity = src.computeDstDensity(); + if (srcDensity == dstDensity) { + return srcDensity; + } + // For P and above, only resize if it would be a downscale. Scale up prior // to P in case the app relies on the Bitmap's size without considering density. - final int dstDensity = src.computeDstDensity(); - if (srcDensity == Bitmap.DENSITY_NONE || srcDensity == dstDensity - || (srcDensity < dstDensity && sApiLevel >= Build.VERSION_CODES.P)) { + if (srcDensity < dstDensity && sApiLevel >= Build.VERSION_CODES.P) { return srcDensity; } float scale = (float) dstDensity / srcDensity; - int scaledWidth = (int) (decoder.mWidth * scale + 0.5f); - int scaledHeight = (int) (decoder.mHeight * scale + 0.5f); - decoder.setTargetSize(scaledWidth, scaledHeight); + int scaledWidth = (int) (mWidth * scale + 0.5f); + int scaledHeight = (int) (mHeight * scale + 0.5f); + this.setTargetSize(scaledWidth, scaledHeight); return dstDensity; }