-
Notifications
You must be signed in to change notification settings - Fork 4k
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
base: master
Are you sure you want to change the base?
Changes from all commits
53948f0
4d686a2
a448949
bac7e19
fb2508f
b14c340
90e9044
77b57c3
a2f09cf
df5b56f
21aa3c8
c2fa402
9aa2877
7efb773
ac7cfe9
89081a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(); | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
<manifest package="com.squareup.picasso3.decoder.svg" /> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) { | ||
try { | ||
SVG.getFromInputStream(source.inputStream()); | ||
return true; | ||
} catch (SVGParseException e) { | ||
return false; | ||
} | ||
} | ||
|
||
@Override public Image decodeImage(BufferedSource source, Request request) throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we'd be better off with a design where
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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)); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
sdk: 18 | ||
constants: com.squareup.picasso3.BuildConfig | ||
manifest: --default |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 89081a3