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

rharter · 2018-04-19T14:24:31Z

This is a POC to demonstrate what I was thinking to support more image types than just the built in bitmap types. Currently, the tasks of loading (from network, disk, etc.) and decoding images are conflated, which means that the type of image you're loading (e.g. jpeg, png, svg) is tightly coupled with where it comes from (e.g. network, filesystem, content resolver).

To solve this, I've extracted the image decoding bits from RequestHandler into an ImageDecoder type, which could use a different name. Each Picasso instance can be configured with a set of ImageDecoders that it will use to attempt to decode images.

Furthermore, the user can specify an ImageDecoderFactory per request so that, if they know what type of image is going to be loaded, they can explicitly state which ImageDecoder to use, reducing the amount of searching that's required.

Still to do:

Decide if the SVG image decoder should be built in (it adds a dependency on a specific library) or an optional add on dependency.
Add tests for default image decoders.

rharter · 2018-04-19T14:25:22Z

picasso-sample/src/main/java/com/example/picasso/Data.java

@@ -15,7 +15,7 @@
      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"


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.

picasso/build.gradle

rharter · 2018-04-19T14:26:26Z

picasso/src/main/java/com/squareup/picasso3/AssetRequestHandler.java

      try {
-        Bitmap bitmap = decodeStream(source, request);
+        ImageDecoder imageDecoder = request.decoderFactory.getImageDecoderForSource(source);


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

rharter · 2018-04-19T14:27:11Z

picasso/src/main/java/com/squareup/picasso3/ImageDecoder.java

+import java.io.IOException;
+import okio.BufferedSource;
+
+public interface ImageDecoder {


This should be renamed so we don't conflict with android.graphics.ImageDecoder.

ImageFormatDecoder?

rharter · 2018-04-19T14:27:25Z

picasso/src/main/java/com/squareup/picasso3/ImageDecoder.java

+
+public interface ImageDecoder {
+
+  public class Image {


rharter · 2018-04-19T14:28:54Z

picasso/src/main/java/com/squareup/picasso3/ImageDecoderFactory.java

+   * @return The first ImageDecoder that can decode the source, or null.
+   */
+  @Nullable ImageDecoder getImageDecoderForSource(BufferedSource source) {
+    for (ImageDecoder decoder : decoders) {


I wasn't sure if we should shortcut and just return if there is only one decoder, as in a user added .asBitmap() to a request. That would remove the need to check if the decoder can handle the source, but might also make the error message more confusing since it will move the error from here into the decode method.

it should always call canHandleSource, at least to avoid surprise.

rharter · 2018-04-19T14:30:07Z

picasso/src/main/java/com/squareup/picasso3/Picasso.java

@@ -745,6 +748,17 @@ public Builder listener(@NonNull Listener listener) {
      return this;
    }

+    /** Add an decoder that can decode custom image formats. */
+    @NonNull
+    public Builder addImageDecoder(@NonNull ImageDecoder imageDecoder) {


Allows system wide addition of new ImageDecoders, so you could add your own MP4->Drawable decoder, for example. By default the included image decoders will be added, so upgrades don't require any code changes.

rharter · 2018-04-19T14:32:14Z

picasso/src/main/java/com/squareup/picasso3/Request.java

@@ -573,6 +579,24 @@ public Builder priority(@NonNull Priority priority) {
      return this;
    }

+    @NonNull
+    public Builder asBitmap() {


This is to make it easier to specify the exact image decoder, avoiding checking all of the default ones. This is completely optional, so no code changes are required.

Usage:

Picasso.get() .load(myImageUrl) .asBitmap() .into(myImageView)l

umm, that's the RequestCreator one :)

This is fine for now, but I want to nuke this whole API from orbit. I'd like requests to be separate from how they're fulfilled.

picasso/src/main/java/com/squareup/picasso3/SourceBufferingInputStream.java

picasso/src/test/java/com/squareup/picasso3/SourceBufferingInputStreamTest.java

picasso/src/main/java/com/squareup/picasso3/SourceBufferingInputStream.java

rharter · 2018-04-24T14:18:09Z

Woah, I had no idea but just found issue #1049 that I filed back in 2015 asking for exactly this.

jrodbx · 2018-08-30T12:44:27Z

@rharter Sorry for the delay in getting to this. Looks like this PR has a few conflicts, plus I've pulled out the BitmapUtils stuff in #1985. Mind rebasing?

I've gone ahead and pushed a branch with what that might look like: 05c9983

And also added some cleanup here: c53f667

to make rebasing less painful.

jrodbx · 2018-08-30T12:46:19Z

Re: the SVG dependency, maybe we pull the SVG decoder out into a separate artifact? That can be a followup PR.

…es imports.

rharter · 2019-01-22T21:10:02Z

Who's ready for an update!?!

Sorry for the delay, but I've rebased this using your commits, then rebased on master, then extracted the SVG bits into an external artifact, like we discussed.

I think the structure of how decoders (I'm thinking we'll have multiples, like another for Animated GIF) are included is up for debate, but got this in to start.

I'm having errorprone problems locally, so let me know if something is off.

NightlyNexus · 2019-01-22T22:24:39Z

decoders/svg/src/main/java/com/squareup/picasso3/decoder/svg/SvgImageDecoder.java

+
+  @Override public boolean canHandleSource(BufferedSource source) {
+    try {
+      SVG.getFromInputStream(source.peek().inputStream());


is it sufficient to check for xml header bytes?

The library internally handles gzipped content here, so we'd have to move that logic out into this, and we'd also have to validate the SVG. It could be done but I think that should live in the SVG library.

OkHttp should be transparently un-gzipping. Or are you worried about reading local files?

That's a fair point. My point was simply that determining if the stream is an SVG is a little more complex that check that it (1) is an XML file and (2) has a DOCTYPE or something. Taking out the GZipInputStream case (which I guess is for http streams), the existing SVG library will validate the SVG as well as load it, which is something that makes sense in an SVG parser, as opposed to reimplementing here.

One example of the added complexity, from what I can see, the existing library checks the DOCTYPE, which might contain <!ENTITY in which case it falls back to a different parser. I think understanding the complexities around XML, and SVG in particular, make sense to be handled in the parser.

Ideally the library had another method that would simply check if the stream is an SVG without parsing the whole thing, but this one doesn't.

decoders/svg/src/main/java/com/squareup/picasso3/decoder/svg/SvgImageDecoder.java

NightlyNexus · 2019-01-22T22:32:54Z

picasso/src/main/java/com/squareup/picasso3/ImageDecoderFactory.java

+   * @return The first ImageDecoder that can decode the source, or null.
+   */
+  @Nullable ImageDecoder getImageDecoderForSource(BufferedSource source) {
+    for (ImageDecoder decoder : decoders) {


it should always call canHandleSource, at least to avoid surprise.

NightlyNexus · 2019-01-22T22:33:25Z

picasso/src/main/java/com/squareup/picasso3/ImageDecoderFactory.java

+        return decoder;
+      }
+    }
+    return null;


throw the ISE here?

In the current implementation the ISE is thrown one level up, where the RequestHandler has the information to create a useful error message, which includes the URI or file path of the request.

We could throw one here, then catch it one level up and wrap it with a useful message, or just let the caller handle it.

picasso/src/main/java/com/squareup/picasso3/ImageDecoderFactory.java

picasso/src/main/java/com/squareup/picasso3/Picasso.java

NightlyNexus · 2019-01-22T22:37:33Z

picasso/src/main/java/com/squareup/picasso3/Picasso.java

  final Map<Object, Action> targetToAction;
  final Map<ImageView, DeferredRequestCreator> targetToDeferredRequestCreator;
+  final List<RequestHandler> extraRequestHandlers;


why was this added?

Fixed in 9aa2877

NightlyNexus · 2019-01-22T22:39:59Z

picasso/src/main/java/com/squareup/picasso3/Picasso.java

@@ -156,7 +159,9 @@ void onImageLoadFailed(@NonNull Picasso picasso, @NonNull Uri uri,
    this.closeableCache = closeableCache;
    this.cache = cache;
    this.listener = listener;
+    this.imageDecoderFactory = imageDecoderFactory;


when you call newBuilder().addImageDecoder(...).build(), are the old image decoders retained and appended to? (add a test?)

Fixed in 7efb773

NightlyNexus · 2019-01-22T22:47:18Z

picasso/src/main/java/com/squareup/picasso3/BitmapImageDecoder.java

+      // we successfully decoded the bounds
+      return options.outWidth > 0 && options.outHeight > 0;
+    } catch (IOException e) {
+      return false;


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.

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.

NightlyNexus · 2019-01-22T22:50:26Z

picasso/src/main/java/com/squareup/picasso3/Picasso.java

@@ -136,8 +136,10 @@ void onImageLoadFailed(@NonNull Picasso picasso, @NonNull Uri uri,
  private final @Nullable okhttp3.Cache closeableCache;
  final PlatformLruCache cache;
  final Stats stats;
+  final ImageDecoderFactory imageDecoderFactory;


where is this used? it should get passed to the RequestCreate/Builder thing.

This is used by the RequestCreator and Request.Builder. The convention seems to be passing the whole Picasso instance into the RequestCreator and having it access the properties on it, like defaultBitmapConfig, dispatcher, cache, etc.

build.gradle

JakeWharton · 2019-02-06T04:41:18Z

decoders/svg/src/main/java/com/squareup/picasso3/decoder/svg/SvgImageDecoder.java

+
+class SvgImageDecoder implements ImageDecoder {
+
+  @Override public boolean canHandleSource(BufferedSource source) {


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

Fixed in 89081a3

JakeWharton · 2019-02-06T04:42:05Z

decoders/svg/src/main/java/com/squareup/picasso3/decoder/svg/SvgImageDecoder.java

+
+  @Override public boolean canHandleSource(BufferedSource source) {
+    try {
+      SVG.getFromInputStream(source.peek().inputStream());


OkHttp should be transparently un-gzipping. Or are you worried about reading local files?

JakeWharton · 2019-02-06T04:45:03Z

decoders/svg/src/main/java/com/squareup/picasso3/decoder/svg/SvgImageDecoder.java

+    }
+  }
+
+  @Override public Image decodeImage(BufferedSource source, Request request) throws IOException {


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

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.

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.

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.

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.

JakeWharton · 2019-02-06T05:07:22Z

picasso/src/main/java/com/squareup/picasso3/Request.java

@@ -573,6 +579,24 @@ public Builder priority(@NonNull Priority priority) {
      return this;
    }

+    @NonNull
+    public Builder asBitmap() {


This is fine for now, but I want to nuke this whole API from orbit. I'd like requests to be separate from how they're fulfilled.

… worry about it.