Subsampling support improvements

[bvitaliyg]

First of all – thank you for the library! I am very happy to use it, but I recently encountered a few limitations for subsampling. Here they are:

1. By default, it doesn't work with anything else but default models for specific models in official loaders support modules – like here it supports only File and Uri models for Coil3.
   There are 2 ways to support custom models: using SubSamplingImage (and losing most of the benefits ZoomableImage UI tweaks and caching of image loader) or forking ZoomableImageSource implementation (Coil3ImageSource in this case). Surely there could be an option with simpler API – like having a specific interface that loads subsampled images as one of the arguments of ZoomableImage and handles nothing else (unlike, for instance, Coil3ImageSource, which handles interaction with image loader to get a preview).

2. There's no efficient way to provide a custom Bitmap SubSamplingImageSource. You can use a file, an asset, a resource, Uri, or a Source (aka InputStream 2.0). But there's no way to use some sort of custom BitmapFactory. This makes using something like PdfRenderer very difficult – the only way to use it would be to take the entire decoded bitmap into memory, compress it into one of the file formats, and provide a Source from the bytes of this format. This takes a lot of memory and CPU time.

3. The reason why the SubSamplingImageSource is limited is because it's tightly coupled with the usage of BitmapRegionDecoder and all ways of customising the SubSamplingImageSource are restricted by it. I think having the option to use a custom Factory would be a much more flexible choice. Also, using BitmapRegionDecoder blocks subsampling to be migrated to KMP.

4. Another method that makes working creating custom SubSamplingImageSource difficult is SubSamplingImageSource.peek(). It ties the implementation to using one or another form of bytes array to decode the image from. That wouldn't make sense for PdfRenderer, for example. Internally, this method is used for:

• getting the image orientation from its exif data
• getting the image's original dimensions
• checking whether the image could be subsampled
• checking whether the source exists (aka file could be read)

I think a better option would be to have these parameters provided explicitly by SubSamplingImageSource. The subclasses could still read EXIF and have the same checks done if they rely on compressed image formats (peek() could still be there, but declared as private).

So, to summarise: an additional parameter for subsampling images could be added to ZoomableImage(). It could exist as a loader-agnostic interface, something like subsampler: suspend (Model) -> SubSamplingImageSource. The could be some default implementations for standard models.

SubSamplingImageSource itself could have peek() replaced with getImageOrientation(), getImageDimensions(), canBeSubsampled(), canBeRead(), and decoder() returning an abstract decoding interface (with something like suspend fun decodeRegion(rect: IntRect, dstSize: IntSize, options: Options): Image?) – or this method could replace decoder() itself.

This is just one of the potential API surfaces, any other implementation would also be great as long as it solves the problems described above.

What do you think? I could try to help with the PR if you're okay with the changes – or if you clarify what you think could be changed.

[saket]

I love this detailed feedback!

1: I'm sorry I don't fully understand this. Are you using Coil for loading your images? If so, could you clarify why does Coil3ImageSource not work for your custom models?

2, 3, 4: I agree that SubSamplingImage() needs to be decoupled from Android. I have a plan to address this by opening up SubSamplingImageSource so developers can load any kind of image bitmaps. I'll work on this soon.

[bvitaliyg]

Thank you! I think I got a bit confused about the first item. In my app, I use Telephoto for 2 use cases – to display the actual photos and to render the PDF as I described above. The photos were not subsampled as well – and I assumed it was related to using a custom model that doesn't fit into one of the categories.

I've double-checked the Coil3ImageSource source code, and it turns out it actually supports subsampling from anything that gets cached on a disk OR sourced from the supported models. After investigation, I found out that my custom Fetcher doesn't use the disk cache correctly – and once it was fixed, the subsampling worked. What you've done there with disk cache support there is quite remarkable TBF :)

The issue with PDF rendering still remains though. In theory, you could write a fetcher that renders the page in its full scale to disk cache – but considering that the PDF renderers will output raw bitmap only (not a stream or something), it has to be stored in-memory during the rendering which kinda makes the subsampling useless (OOM can happen).

[saket]

I found out that my custom Fetcher doesn't use the disk cache correctly – and once it was fixed, the subsampling worked

I'm glad you got it working, but I feel bad that you had to dive into the source code to figure this out. Is there a better way I could surface this information for developers?

The issue with PDF rendering still remains though. In theory, you could write a fetcher that renders the page in its full scale to disk cache – but considering that the PDF renderers will output raw bitmap only (not a stream or something), it has to be stored in-memory during the rendering which kinda makes the subsampling useless (OOM can happen).

Yea that's fair. I've just pushed a commit that makes SubSamplingImageSource non-sealed so that consumers can use custom image decoders with SubSamplingImage(). Next, I'm going to find a way to kill the peek() function so you can start using it for rendering your PDFs.

[bvitaliyg]

Thank you! I think the documentation is great – I noticed that the image should be sub-sampled from very beginning, it's just it wasn't working for my case, so I assumed after brief reading of the code that it's related to the model – not to the disk cache as well.

[saket]

@bvitaliyg It is done. could you try out 0.15.0-SNAPSHOT and share your feedback?

[bvitaliyg]

Thank you! I cloned the repo and published the snapshot to Mavel local repository.
The API felt like it could be simplified initially (e.g. to have no ImageRegionDecoder.Factory or even have fun decode(region: IntRect, sampleSize: Int, imageOptions: FactoryParams): ImageBitmap in SubSamplingImageSource directly, but your approach made a lot of sense when I saw PooledAndroidImageRegionDecoder.

I've also encountered an FPS drop when the PDF was zoomed. I checked out the threads and I found out that decodeRegion() method was called in main thread. When I switched to another dispatcher, everything was smooth again. The library I use for PDF rendering requires some sync for Bitmap rendering so it was causing this all together.

Never the less, custom image sources might take some time to render (including BitmapFactory/BitmapRegionDecoder from Android Framework), so it would be nice to have built-it dispatching for image decoding (or mentioning of coroutine being executed on the main thread in the docs).

Everything else worked like charm! Looking forward to the next release

[saket]

Glad to hear!

I've also encountered an FPS drop when the PDF was zoomed. I checked out the threads and I found out that decodeRegion() method was called in main thread. When I switched to another dispatcher, everything was smooth again.

While I'd like to advocate that functions should never make any assumptions about the thread they're called from, you're right that I should probably mention that decode gets called on the main thread. I'll add something.

Are you building an OSS for rendering PDFs? Let me know if you need any other changes.

[bvitaliyg]

Agreed, we shouldn't assume on which thread the suspend functions are called. Just thought if you decode the subsampled bitmaps off the main thread it can result in smoother user experience overall if you currently invoke BitmapRegionDecoder on the main thread.

I'm building the PDF renderer, currently closed-source, but I'm considering making it OSS when I it's done. Thank you again for the library, it already has a lot of features I need (such as having coordinates of tap/long tap), and it's super-stable. Will let you know if I need any other changes :)