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

Use Future or Isolate? #1

Open
vaetas opened this issue Apr 11, 2023 · 3 comments
Open

Use Future or Isolate? #1

vaetas opened this issue Apr 11, 2023 · 3 comments

Comments

@vaetas
Copy link

vaetas commented Apr 11, 2023

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
Copy link
Owner

PiN73 commented Apr 11, 2023

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
Copy link
Owner

PiN73 commented Apr 11, 2023

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
Copy link
Author

vaetas commented Apr 13, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants