briangonzalez / rgbaster.js

🎨 A simple library for extracting dominant colors from images.
1.74k stars 212 forks source link

Blocking UI thread when use analyze function #47

Open yxunknown opened 5 years ago

yxunknown commented 5 years ago

I am trying to use analyze function at version 2 to get dominant color of an image, but the call blocks my UI and make a long-time no response in browser. My code is below and runtime is Angular 7.

import analyze from 'rgbaster';

// lifecycle callback
async ngAfterViewInit() {
    const result = await analyze('../../assets/images/1542072554094.jpg');
    console.log(result);
 }

I am also trying to install rgbaster.js and import analyze from 'rgbaster.js', but it don't works. I learned that the version 2 of rgbaster was written in typescript, so i think there is no any problem to using this library in Angular framework. But I hava no idea about that the analyze function doesn't work and block ui Thread and raise a crash to browser.

Thanks for your any replies!

briangonzalez commented 5 years ago

Looks like we need to include the build files here: https://github.com/briangonzalez/rgbaster.js/blob/master/package.json#L27

Also, would you mind including the image you're processing?

Cheers.

AlfredJKwack commented 5 years ago

Hi @briangonzalez,

Not sure I follow:

Looks like we need to include the build files here:

Whatever we add to files in the package.json will be in the tarball that is created when the downstream project runs npm publish. Running npm pack on RGBaster to get the tarball for inspection I get the following:

> npm pack
npm notice 
npm notice 📦  rgbaster@2.0.0
npm notice === Tarball Contents === 
npm notice 1.1kB package.json   
npm notice 2.2kB README.md      
npm notice 2.8kB rgbaster.min.js
npm notice === Tarball Details === 
npm notice name:          rgbaster                                
npm notice version:       2.0.0                                   
npm notice filename:      rgbaster-2.0.0.tgz                      
npm notice package size:  3.0 kB                                  
npm notice unpacked size: 6.1 kB                                  
npm notice shasum:        7c21609febb4180ff52b40d9208269a90dc5425d
npm notice integrity:     sha512-rgQaLpeKUTfOQ[...]Mb9pXzy2ywakA==
npm notice total files:   3                                       
npm notice 
rgbaster-2.0.0.tgz

This looks to me like what I'd expect. I haven't set up an Angular project for further testing to see what would happen on a downstream project. Do you expect a different behaviour (ie. something other than inclusion of rgbaster.min.js) ?

Maybe it's a good time to start thinking of ugrading briangonzalez/jquery.adaptive-backgrounds.js to the next version.

yxunknown commented 5 years ago

Looks like we need to include the build files here: https://github.com/briangonzalez/rgbaster.js/blob/master/package.json#L27

Also, would you mind including the image you're processing?

Cheers.

1542072554094 i will try to add files section in my package.json configuration. thanks for your help.

And the image i try to process is not so big.

Happy new year to you!

briangonzalez commented 5 years ago

@AlfredJKwack On second glance, you're right. rgbaster.min.js is the only build file.

a-hariti commented 5 years ago

I think this issue is the most pressing one before the next release. This library does indeed block the UI while doing its thing. I suggest we look at 2 options:

  1. time-slicing with async recursion or otherwise
  2. web workers

or both of them. @briangonzalez ?

briangonzalez commented 5 years ago

I like #2 as it seems like the right tool for the job.

rgbaster is already async because we use onload, so it won't even be a breaking change.

a-hariti commented 5 years ago

I mean by (1) doing the the work in getCounts in chunks of the data at a time. I have fiddled with (1) and it looks promising, I'm going to explore (2) and discuss further. here's a proof of concept:

async function getCounts(
  data: Uint8ClampedArray,
  ignore: string[],
  pixelsPerChunk = 1000 // what's an ideal default ??
): Promise<[]> {
  function getCountMap(index: number, countMap = {}, resolve: Function) {
    let upperBoundary = Math.min(index + 4 * pixelsPerChunk, data.length)

    for (
      let i = index;
      i < upperBoundary;
      i += 4 /* 4 gives us r, g, b, and a*/
    ) {
      let alpha: number = data[i + 3]
      // skip FULLY transparent pixels
      if (alpha === 0) continue

      let rgbComponents: number[] = Array.from(data.subarray(i, i + 3))

      // skip undefined data
      if (rgbComponents.indexOf(undefined) !== -1) continue

      let color: string =
        alpha && alpha !== 255
          ? `rgba(${[...rgbComponents, alpha].join(',')})`
          : `rgb(${rgbComponents.join(',')})`

      // skip colors in the ignore list
      if (ignore.indexOf(color) !== -1) continue

      if (countMap[color]) countMap[color].count++
      else countMap[color] = { color, count: 1 }
    }
    if (upperBoundary == data.length) resolve(countMap)
    else {
      // queue the proecessing of the next chunk as a macrotsk
      // for the next tick of the event loop
      setTimeout( // what about requestAnimationFrame instead ??
        () => getCountMap(index + 4 * pixelsPerChunk, countMap, resolve),
        0
      )
    }
  }
  const countMap = await new Promise(resolve => getCountMap(0, {}, resolve))
  const counts = Object.values(countMap) as []
  return counts.sort((a: any, b: any) => b.count - a.count)
}
AlfredJKwack commented 5 years ago

Hi,

On the topic of chunking out the work.

Rather than guessing how many to chunk at once, it's also possible to let elapsed time be the guide for each chunk and to let it process as many as it can in a given time interval. This somewhat automatically guarantees browser responsiveness regardless of how CPU intensive the iteration is. So, rather than passing in a chunk size, you can pass in a millisecond value. This of course does open the question of how many milliseconds to pick.

On a typical laptop with a refresh rate of 60 Hz, each frame is on the screen for about 16-17 ms. Lets assume we take a timer of 10 ms. This delay means a roughly 66% chance of blocking the ui for a single screen refresh. Such a delay would not be noticeable under most conditions. If the page has lots of animation, a 10ms delay might be noticeable. A gamble but not a huge one IMHO.

Here's an example:

// last two args are optional
function processLargeArrayAsync(array, fn, maxTimePerChunk, context) {
    context = context || window;
    maxTimePerChunk = maxTimePerChunk || 10;
    var index = 0;

    function now() {
        return new Date().getTime();
    }

    function doChunk() {
        var startTime = now();
        while (index < array.length && (now() - startTime) <= maxTimePerChunk) {
            // callback called with args (value, index, array)
            fn.call(context, array[index], index, array);
            ++index;
        }
        if (index < array.length) {
            // set Timeout for async iteration
            setTimeout(doChunk, 0);
        }
    }    
    doChunk();    
}

processLargeArrayAsync(veryLargeArray, myCallback);

I think we need to profile this code a little more to start with. I'm thinking that a 4k image would be the worst thing someone might reasonably throw at this. That's about 35mio items in the array to process.

a-hariti commented 5 years ago

the image maipulation is a CPU intensive task, and it already takes a lot of time, even when choking the main thread, I think, as @briangonzalez mentioned, the web workers option would be better, giving us the freedom to process every thing in one loop without worries. there are two options for this:

  1. export the getImageData and getCounts functions to the user, and document how they would string them together with a new Worker('./someScript.js') that would contain some logic to execute getCounts
  2. workerize the getCounts function at build time using something similar to greenlet, I tried to include it but microbundle doesn't seem bundle it correctly, also the tests fail with an error telling me that the modules default export is not a function ! ps: it seems that pacel does have the issue in (2)

Any thoughts ?