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 web worker for Quantize and Score steps #129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FluorescentHallucinogen
Copy link

@FluorescentHallucinogen FluorescentHallucinogen commented Dec 2, 2023

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.

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

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? 🤔

@FluorescentHallucinogen FluorescentHallucinogen changed the title Use web worker Use web worker for Quantize and Score steps Dec 3, 2023
@FluorescentHallucinogen
Copy link
Author

See also #133 and #128 for further possible performance optimizations.

@FluorescentHallucinogen
Copy link
Author

@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'});

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? 🤔

@guidezpl
Copy link
Collaborator

guidezpl commented Dec 7, 2023

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);

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?

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

Successfully merging this pull request may close these issues.

None yet

2 participants