-
Notifications
You must be signed in to change notification settings - Fork 159
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 web worker for Quantize and Score steps #129
base: main
Are you sure you want to change the base?
Use web worker for Quantize and Score steps #129
Conversation
@@ -29,7 +26,8 @@ import {argbFromRgb} from './color_utils.js'; | |||
export async function sourceColorFromImage(image: HTMLImageElement) { | |||
// Convert Image data to Pixel Array | |||
const imageBytes = await new Promise<Uint8ClampedArray>((resolve, reject) => { | |||
const canvas = document.createElement('canvas'); | |||
const element = document.createElement('canvas'); | |||
const canvas = 'OffscreenCanvas' in window ? element.transferControlToOffscreen() : element; |
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.
Does it make any sense to use transferControlToOffscreen()
without using this canvas in the web worker? 🤔
If not, does it make any sense to move the callback
function code into the web worker? 🤔
@rodydavis @guidezpl Could you please take a look? 😉 |
let ranked: number[]; | ||
|
||
if (window.Worker) { | ||
const worker = new Worker(new URL('./image_utils_worker.js', import.meta.url), {type: 'module'}); |
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.
If I create a new web worker here, do I have to destroy it somehow afterwards? 🤔
Hi, I'm not working on this project now, please tag @pennzht @marshallworks instead :) |
if (window.Worker) { | ||
const worker = new Worker(new URL('./image_utils_worker.js', import.meta.url), {type: 'module'}); | ||
|
||
worker.postMessage(imageBytes); |
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.
The second argument of postMessage
is an optional array of transferable objects, which are objects that can be transferred from one context to another with zero-copy. The imageBytes
is a Uint8ClampedArray
that is not a transferable type. This means that we can't just write:
worker.postMessage(imageBytes, [imageBytes]);
The supported types of transferable objects are ArrayBuffer
, MessagePort
, and ImageBitmap
.
So to fix this, we need to pass the underlying ArrayBuffer
of the imageBytes
as the first argument, and [imageBytes.buffer]
as the second argument. This way, we are transferring the ownership of the ArrayBuffer
to the worker, and avoiding unnecessary copying:
worker.postMessage(imageBytes.buffer, [imageBytes.buffer]);
But in this case, we need in the web worker to use:
const imageBytes = new Uint8ClampedArray(event.data);
instead of:
const imageBytes = event.data;
Does it make any sense? Does new Uint8ClampedArray(event.data)
create a copy? 🤔
Can we write
worker.postMessage(imageBytes, [imageBytes.buffer]);
instead of
worker.postMessage(imageBytes.buffer, [imageBytes.buffer]);
Is that even correct? The browser doesn't give any errors. But does it work? Does such a code make any sense at all?
This pull request moves the extraction of the dominant colors of the image to web worker to avoid heavy work (especially for high resolution images) on the main thread, improving the UI responsiveness. It also use OffscreenCanvas to convert the image to pixel array.
It uses feature-detect for Web Workers and OffscreenCanvas support, and if available, use them.