Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Implements ImageDecoders to decouple decoding from downloading. #1890

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions build.gradle
Expand Up @@ -10,7 +10,8 @@ buildscript {
]

ext.deps = [
androidPlugin: 'com.android.tools.build:gradle:3.2.1',
androidPlugin: 'com.android.tools.build:gradle:3.3.0',
androidSvg: 'com.caverock:androidsvg-aar:1.3',
okhttp: "com.squareup.okhttp3:okhttp:${versions.okhttp}",
okio: "com.squareup.okio:okio:${versions.okio}",
mockWebServer: "com.squareup.okhttp3:mockwebserver:${versions.okhttp}",
Expand Down Expand Up @@ -99,7 +100,7 @@ subprojects {
version = VERSION_NAME

afterEvaluate {
tasks.findByName('check').dependsOn('checkstyle')
tasks.getByName('check').dependsOn('checkstyle')
}

apply plugin: 'net.ltgt.errorprone'
Expand Down
15 changes: 15 additions & 0 deletions decoders/svg/README.md
@@ -0,0 +1,15 @@
Picasso SVG Image Decoder
====================================

An image decoder that allows Picasso to decode SVG images.

Usage
-----

Provide an instance of `SvgImageDecoder` when creating a `Picasso` instance.

```java
Picasso p = new Picasso.Builder(context)
.addImageDecoder(new SvgImageDecoder())
.build();
```
34 changes: 34 additions & 0 deletions decoders/svg/build.gradle
@@ -0,0 +1,34 @@
apply plugin: 'com.android.library'

android {
compileSdkVersion versions.compileSdk

defaultConfig {
minSdkVersion versions.minSdk
}

compileOptions {
sourceCompatibility versions.sourceCompatibility
targetCompatibility versions.targetCompatibility
}

lintOptions {
textOutput 'stdout'
textReport true
lintConfig file('lint.xml')
}
}

dependencies {
api project(':picasso')
implementation deps.androidSvg
compileOnly deps.androidxAnnotations
testImplementation deps.junit
testImplementation deps.robolectric
testImplementation deps.truth
testImplementation deps.mockito

annotationProcessor deps.nullaway
}

apply from: rootProject.file('gradle/gradle-mvn-push.gradle')
4 changes: 4 additions & 0 deletions decoders/svg/gradle.properties
@@ -0,0 +1,4 @@
POM_ARTIFACT_ID=picasso-decoder-svg
POM_NAME=Picasso SVG Descoder
POM_DESCRIPTION=An image decoder that supports SVG images.
POM_PACKAGING=aar
1 change: 1 addition & 0 deletions decoders/svg/src/main/AndroidManifest.xml
@@ -0,0 +1 @@
<manifest package="com.squareup.picasso3.decoder.svg" />
@@ -0,0 +1,52 @@
package com.squareup.picasso3.decoder.svg;

import android.graphics.Bitmap;
import android.graphics.Canvas;
import com.caverock.androidsvg.SVG;
import com.caverock.androidsvg.SVGParseException;
import com.squareup.picasso3.ImageDecoder;
import com.squareup.picasso3.Request;
import java.io.IOException;
import okio.BufferedSource;

class SvgImageDecoder implements ImageDecoder {

@Override public boolean canHandleSource(BufferedSource source) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like we should hand a peeked source here rather than let consumers worry about it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 89081a3

try {
SVG.getFromInputStream(source.inputStream());
return true;
} catch (SVGParseException e) {
return false;
}
}

@Override public Image decodeImage(BufferedSource source, Request request) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we'd be better off with a design where canHandleSource returned either:

  1. An opaque Object (or maybe T) which is forwarded to this method. In this case it would be the SVG so it doesn't need re-decoded. Return null if you can't handle.
  2. The actual Decoder and this would change to a Decoder.Factory. That way you could propagate as much information as you wanted. Return null if you can't handle. I guess technically you can already do this with case Start actual website content. #1 by just using some data class as your type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that simplify things?

In the SVG case it would remove a parse, since we can't currently detect an SVG without parsing it, but I think that's sort of a special case. Bitmap, where we just decode the bounds to identify if it's parseable, doesn't have that limitation, and also nothing useful to return. In those cases I think this dead simple API is nice, though I'll admit I'm not entirely sure what case #2 would look like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bounds are useful to propagate for Bitmap so we can avoid re-reading them to apply transformations during decode.

Case #2 would work like Retrofit Converter.Factory or CallAdapter.Factory or Moshi JsonAdapter.Factory. If you can handle, return a handler.

try {
SVG svg = SVG.getFromInputStream(source.inputStream());
if (request.hasSize()) {
if (request.targetWidth != 0) {
svg.setDocumentWidth(request.targetWidth);
}
if (request.targetHeight != 0) {
svg.setDocumentHeight(request.targetHeight);
}
}

int width = (int) svg.getDocumentWidth();
if (width == -1) {
width = (int) svg.getDocumentViewBox().width();
}
int height = (int) svg.getDocumentHeight();
if (height == -1) {
height = (int) svg.getDocumentViewBox().height();
}
Bitmap bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
Canvas canvas = new Canvas(bitmap);
svg.renderToCanvas(canvas);

return new Image(bitmap);
} catch (SVGParseException e) {
throw new IOException(e);
}
}
}
@@ -0,0 +1,67 @@
package com.squareup.picasso3.decoder.svg;

import android.net.Uri;
import com.squareup.picasso3.Request;
import java.io.IOException;
import java.io.InputStream;
import okio.BufferedSource;
import okio.Okio;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;

import static com.google.common.truth.Truth.assertThat;
import static com.squareup.picasso3.ImageDecoder.Image;
import static org.mockito.Mockito.mock;

@RunWith(RobolectricTestRunner.class)
public class SvgImageDecoderTest {

private SvgImageDecoder decoder;

@Before public void setup() {
decoder = new SvgImageDecoder();
}

@Test public void canHandleSource_forSvg_returnsTrue() {
BufferedSource svg = bufferResource("/android.svg");
assertThat(decoder.canHandleSource(svg)).isTrue();
}

@Test public void canHandleSource_forBitmap_returnsFalse() {
BufferedSource jpg = bufferResource("/image.jpg");
assertThat(decoder.canHandleSource(jpg)).isFalse();
}

@Test public void decodeImage_withoutTargetSize_returnsNativelySizedImage() throws IOException {
BufferedSource svg = bufferResource("/android.svg");
Request request = new Request.Builder(mock(Uri.class)).build();
Image image = decoder.decodeImage(svg, request);

assertThat(image.bitmap).isNotNull();
assertThat(image.bitmap.getWidth()).isEqualTo(96);
assertThat(image.bitmap.getHeight()).isEqualTo(105);
}

@Test public void decodeImage_withTargetSize_returnsResizedImage() throws IOException {
BufferedSource svg = bufferResource("/android.svg");
Request request = new Request.Builder(mock(Uri.class))
.resize(50, 50)
.build();
Image image = decoder.decodeImage(svg, request);

assertThat(image.bitmap).isNotNull();
assertThat(image.bitmap.getWidth()).isEqualTo(50);
assertThat(image.bitmap.getHeight()).isEqualTo(50);
}

private BufferedSource bufferResource(String name) {
InputStream in = SvgImageDecoderTest.class.getResourceAsStream(name);
if (in == null) {
throw new IllegalArgumentException("Unknown resource for name: " + name);
}
return Okio.buffer(Okio.source(in));
}

}
11 changes: 11 additions & 0 deletions decoders/svg/src/test/resources/android.svg
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added decoders/svg/src/test/resources/image.jpg
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions decoders/svg/src/test/resources/robolectric.properties
@@ -0,0 +1,3 @@
sdk: 18
constants: com.squareup.picasso3.BuildConfig
manifest: --default
2 changes: 1 addition & 1 deletion picasso-sample/src/main/java/com/example/picasso/Data.java
Expand Up @@ -15,7 +15,7 @@ final class Data {
BASE + "Q54zMKT" + EXT, BASE + "9t6hLbm" + EXT, BASE + "F8n3Ic6" + EXT,
BASE + "P5ZRSvT" + EXT, BASE + "jbemFzr" + EXT, BASE + "8B7haIK" + EXT,
BASE + "aSeTYQr" + EXT, BASE + "OKvWoTh" + EXT, BASE + "zD3gT4Z" + EXT,
BASE + "z77CaIt" + EXT,
BASE + "z77CaIt" + EXT, "https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/android.svg"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was looking for a Picasso related sample SVG to use, but couldn't find much. This should, perhaps, be replaced with something we can guarantee.

};

private Data() {
Expand Down
Expand Up @@ -17,15 +17,13 @@

import android.content.Context;
import android.content.res.AssetManager;
import android.graphics.Bitmap;
import android.net.Uri;
import androidx.annotation.NonNull;
import java.io.IOException;
import okio.BufferedSource;
import okio.Okio;
import okio.Source;

import static android.content.ContentResolver.SCHEME_FILE;
import static com.squareup.picasso3.BitmapUtils.decodeStream;
import static com.squareup.picasso3.Picasso.LoadedFrom.DISK;
import static com.squareup.picasso3.Utils.checkNotNull;

Expand Down Expand Up @@ -56,11 +54,18 @@ public void load(@NonNull Picasso picasso, @NonNull Request request, @NonNull Ca

boolean signaledCallback = false;
try {
Source source = Okio.source(assetManager.open(getFilePath(request)));
BufferedSource source = Okio.buffer(Okio.source(assetManager.open(getFilePath(request))));
try {
Bitmap bitmap = decodeStream(source, request);
ImageDecoder imageDecoder = request.decoderFactory.getImageDecoderForSource(source);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is repeated in each RequestHandler and should probably be extracted into the abstract base class.

if (imageDecoder == null) {
callback.onError(
new IllegalStateException("No image decoder for source: " + getFilePath(request))
);
return;
}
ImageDecoder.Image image = imageDecoder.decodeImage(source, request);
signaledCallback = true;
callback.onSuccess(new Result(bitmap, DISK));
callback.onSuccess(new Result(image.bitmap, image.drawable, DISK, image.exifOrientation));
} finally {
try {
source.close();
Expand Down
@@ -0,0 +1,33 @@
package com.squareup.picasso3;

import android.graphics.BitmapFactory;
import androidx.annotation.NonNull;
import java.io.IOException;
import okio.BufferedSource;

import static com.squareup.picasso3.BitmapUtils.decodeStream;

public final class BitmapImageDecoder implements ImageDecoder {

@Override public boolean canHandleSource(@NonNull BufferedSource source) {
try {
if (Utils.isWebPFile(source)) {
return true;
}

BitmapFactory.Options options = new BitmapFactory.Options();
options.inJustDecodeBounds = true;
BitmapFactory.decodeStream(source.inputStream(), null, options);
// we successfully decoded the bounds
return options.outWidth > 0 && options.outHeight > 0;
} catch (IOException e) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm, i think this whole method should just always true. an I/O problem and all the other things are different from not being able to handle the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IOException is just because that's required to be handled, the main idea here is that the stream might not be something Bitmap factory can handle (an SVG, an animated GIF, a fat jpeg, a live photo), so this needs to test that it can decode a bitmap from the stream.

The fastest way I could think of, without needing to load the entire bitmap into memory, was but simply decoding the bounds.

}
}

@NonNull @Override
public Image decodeImage(@NonNull BufferedSource source, @NonNull Request request)
throws IOException {
return new Image(decodeStream(source, request));
}
}
16 changes: 7 additions & 9 deletions picasso/src/main/java/com/squareup/picasso3/BitmapUtils.java
Expand Up @@ -15,7 +15,6 @@
*/
package com.squareup.picasso3;

import android.annotation.SuppressLint;
import android.content.Context;
import android.content.res.Resources;
import android.graphics.Bitmap;
Expand Down Expand Up @@ -106,22 +105,21 @@ static Bitmap decodeStream(Source source, Request request) throws IOException {
ExceptionCatchingSource exceptionCatchingSource = new ExceptionCatchingSource(source);
BufferedSource bufferedSource = Okio.buffer(exceptionCatchingSource);
Bitmap bitmap = SDK_INT >= 28
? decodeStreamP(request, bufferedSource)
: decodeStreamPreP(request, bufferedSource);
? decodeStreamP(bufferedSource, request)
: decodeStreamPreP(bufferedSource, request);
exceptionCatchingSource.throwIfCaught();
return bitmap;
}

@RequiresApi(28)
@SuppressLint("Override")
private static Bitmap decodeStreamP(Request request, BufferedSource bufferedSource)
throws IOException {
ImageDecoder.Source imageSource =
ImageDecoder.createSource(ByteBuffer.wrap(bufferedSource.readByteArray()));
private static Bitmap decodeStreamP(BufferedSource source, Request request) throws IOException {
android.graphics.ImageDecoder.Source imageSource =
android.graphics.ImageDecoder.createSource(ByteBuffer.wrap(source.readByteArray()));
return decodeImageSource(imageSource, request);
}

private static Bitmap decodeStreamPreP(Request request, BufferedSource bufferedSource)
@NonNull
private static Bitmap decodeStreamPreP(BufferedSource bufferedSource, Request request)
throws IOException {
boolean isWebPFile = Utils.isWebPFile(bufferedSource);
boolean isPurgeable = request.purgeable && SDK_INT < Build.VERSION_CODES.LOLLIPOP;
Expand Down
Expand Up @@ -18,19 +18,18 @@
import android.content.ContentResolver;
import android.content.Context;
import android.content.UriMatcher;
import android.graphics.Bitmap;
import android.net.Uri;
import android.provider.ContactsContract;
import androidx.annotation.NonNull;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import okio.BufferedSource;
import okio.Okio;
import okio.Source;

import static android.content.ContentResolver.SCHEME_CONTENT;
import static android.provider.ContactsContract.Contacts.openContactPhotoInputStream;
import static com.squareup.picasso3.BitmapUtils.decodeStream;
import static com.squareup.picasso3.Picasso.LoadedFrom.DISK;
import static com.squareup.picasso3.Utils.checkNotNull;

Expand Down Expand Up @@ -78,9 +77,16 @@ public void load(@NonNull Picasso picasso, @NonNull Request request, @NonNull Ca
try {
Uri requestUri = checkNotNull(request.uri, "request.uri == null");
Source source = getSource(requestUri);
Bitmap bitmap = decodeStream(source, request);

BufferedSource bufferedSource = Okio.buffer(source);
ImageDecoder imageDecoder = request.decoderFactory.getImageDecoderForSource(bufferedSource);
if (imageDecoder == null) {
callback.onError(new IllegalStateException("No image decoder for source: " + request));
return;
}
ImageDecoder.Image image = imageDecoder.decodeImage(bufferedSource, request);
signaledCallback = true;
callback.onSuccess(new Result(bitmap, DISK));
callback.onSuccess(new Result(image.bitmap, image.drawable, DISK, image.exifOrientation));
} catch (Exception e) {
if (!signaledCallback) {
callback.onError(e);
Expand Down