PiN73 / thumbhash

Dart/Flutter implementation of ThumbHash algorithm ā€” a very compact representation of an image placeholder
5 stars 1 forks source link

Use Future or Isolate? #1

Open vaetas opened 1 year ago

vaetas commented 1 year ago

Hey, I've been checking this project as I might potentially use it in the future to replace Blurhash. Thank you for your work.

One thing that caught my eye is that this implementation does not use any async features. For example, flutter_blurhash uses Future for its decoding function. All this processing is going to be done on the UI thread and from my understanding this could cause render janks. Is there any specific reason why you chose to do it the sync way? (I saw that the official implementations for other languages also omit async, though I don't know how concurrency works for them.)

I do not have knowledge of the algorithm for decoding thumbhash so if it's significantly faster than the blurhash, concurrency might not be necessary. I would love to hear your insight.

Otherwise I think it would be cool for performance reasons to use something like Isolate.run() to wrap the methods and force them into different "thread". I have not done any performance benchmarks so that should be done first though. (Though spawning isolates used to take additional time, I'm not sure if it's fixed yet.) Blurhash implementation for Flutter allows specifying background Container with a color before the placeholder gets decoded and renered.

Please let me know what you think. Potentially I could help with that when I have time šŸ™‚

PiN73 commented 1 year ago

Thank you, it's interesting to think about.

Just wrapping functions with Future won't help because actual decoding will be still synchronous.

However, it's worth benchmarking decoding algorithm. Maybe it takes short time because thumbhash image is decoded maximum to 32x32 pixels. If it turns out that it takes long time, then I will consider decoding in Isolate for flutter_thumbhash.

PiN73 commented 1 year ago

Anyway we should add caching. With current straightforward implementation,

Image(
  image: ThumbHash.fromBase64('3OcRJYB4d3h/iIeHeEh3eIhw+j3A').toImage(),
)

seems to make decoding (plus encoding to bmp) on each build.

Instead of this, we should make API like

Image(
  image: ThumbHashImage.fromBase64('3OcRJYB4d3h/iIeHeEh3eIhw+j3A'),
)

where ThumbHashImage is ImageProvider which will decode to bmp (in isolate or not) once and cache the result.

vaetas commented 1 year ago

Yes, using ImageProvider would be way better. Doing it on every build a big performance issue.

As far as the Isolate goes we can leave this open and do the benchmarks when there is time.