Using wasm to accelerate PixelManipulation.js #1093

Divy123 · 2019-05-29T20:18:21Z

Fixes #400

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
code is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
ask @publiclab/is-reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

Divy123 · 2019-05-29T20:18:47Z

The C code is:

int getPixels(int , int , int );

int setPixels(int, int, int, int);

int * changePixel(int, int, int, int, int, int);

void paceOp();

void consoleLog(int);

void manipulatePixel(int width, int height, int inBrowser, int test){
  consoleLog(100);
  for (int x = 0; x < width; x++) {
        for (int y = 0; y <height; y++) {
          int* pixel =changePixel(
            getPixels(x, y, 0),
            getPixels(x, y, 1),
             getPixels(x, y, 2),
             getPixels(x, y, 3),
            x,
            y
          );
          setPixels(x, y, 0, pixel[0]);
          setPixels(x, y, 1, pixel[1]);
          setPixels(x, y, 2, pixel[2]);
          setPixels(x, y, 3, pixel[3]);

          if (!inBrowser && !test) paceOp();
        }
      }
}

Divy123 · 2019-05-31T06:00:36Z

@jywarren I have sucessfully made the first transition from js to wasm on the commented out code involving loops, as you can see.

Please provide your feedback on the same. Also, can you please provide some information on pace.op() as I have to use that in wasm but am not familiar with that.

Thanks!!

jywarren · 2019-05-31T15:55:59Z

Oh, i'm not familiar with pace.op() either - can you use the blame feature to see who added it?

Did you then figure out the issue you'd had trouble with earlier? If so, congrats!!!

Did you look at why tests are failing?

One thing you might consider is looking at the benchmarks:

https://travis-ci.org/publiclab/image-sequencer/jobs/539546409#L1318

And, what about setting up the tests to run both with WebAssembly and without (using an options flat like withWebAssembly = true, so that we get 2x benchmarks and can compare across? Ensuring that the code runs with and without the webassembly changes would be great to have as an option and also great to codify in the tests. What do you think?

jywarren · 2019-05-31T15:56:57Z

Huh - this error is because different offsets are supposed to change the output, but it doesn't seem to:

https://travis-ci.org/publiclab/image-sequencer/jobs/539546409#L1173

Divy123 · 2019-06-01T16:13:26Z

@tech4GT , can you elaborate something about the use of pace and what is pace.op() ?

Divy123 · 2019-06-01T16:16:57Z

Thanks @jywarren . Looking into the issues now.

tech4GT · 2019-06-01T16:40:32Z

Hi, pace is actually used for drawing the progress bars, I originally planned to fork pace and make some changes specific to Image sequencer and enhance the whole feature but never got around to it.
pace.op() actually represents one tick of the progress bar on the console.

Divy123 · 2019-06-02T12:32:57Z

Thanks @tech4GT .
@jywarren what's happening in this is I was using WebAssembly.instantiateStreaming method which works only for browsers which I came to know today only 😅 .

I will be using another function and updating the PR as soon as possible.

jywarren · 2019-06-02T12:42:06Z

Oh, great breakthrough! Awesome!

…

On Sun, Jun 2, 2019, 8:33 AM Slytherin ***@***.***> wrote: Thanks @tech4GT <https://github.com/tech4GT> . @jywarren <https://github.com/jywarren> what's happen in this is I was using WebAssembly.instantiateStreaming method which works only for browsers which I came to know today only 😅 . I will be using another function and updating the PR as soon as possible. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1093?email_source=notifications&email_token=AAAF6J6MOUD7PJMQ52SO5TLPYO4XTA5CNFSM4HQUJWO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWXUUMQ#issuecomment-498027058>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J65BGH2ARRELHJE6PTPYO4XTANCNFSM4HQUJWOQ> .

Divy123 · 2019-06-02T15:07:39Z

Just one thing here, I am able to see is options.inBrowser is true while running tests. Is it correct and if yes, why are we doing this?

Divy123 · 2019-06-02T15:52:48Z

The issue I am facing is fs module not working.
When I console.log fs, I see {} and fs.readFileSync() is undefined inside pixelManipulation.js file while running tests.

@jywarren , @tech4GT can you please help me with this?

I don't know but I think this is because of tests are running in browser mode and fs is not recognised there.

harshkhandeparkar · 2019-06-02T16:04:20Z

@Divy123 do you have a code snippet?

Divy123 · 2019-06-02T16:05:23Z

Yes sharing now

if (options.changePixel) {
      isDone = true;
      /* Allows for Flexibility
       if per pixel manipulation is not required */
      const fs = require('fs');
      console.log(fs.readFileSync)
      var source = fs.readFileSync('./p1.wasm');
      var typedArray = new Uint8Array(source);

harshkhandeparkar · 2019-06-02T16:06:22Z

What is the test command? which file tests this? all(most) of them?

Divy123 · 2019-06-02T16:07:03Z

Basically run() is not working as it involves execution of this piece of code in some modules of ours.

harshkhandeparkar · 2019-06-02T16:07:42Z

So the test file is run.js?

Divy123 · 2019-06-02T16:08:09Z

Test file is image-sequencer.js in test folder.

harshkhandeparkar · 2019-06-02T16:08:46Z

Ok. Looking into it

Divy123 · 2019-06-02T16:09:20Z

Thanks a lot !!

harshkhandeparkar · 2019-06-02T16:11:27Z

In this command

TEST=true istanbul cover tape test/core/*.js test/core/ui/user-interface.js test/core/modules/*.js | tap-spec; browserify test/core/sequencer/image-sequencer.js test/core/sequencer/chain.js test/core/sequencer/meta-modules.js test/core/sequencer/replace.js test/core/sequencer/import-export.js test/core/sequencer/run.js test/core/sequencer/dynamic-imports.js test/core/util/*.js test/core/sequencer/benchmark.js | tape-run --render=\"tap-spec\"

the file is browserified, does it get browserified properly without errors?

Divy123 · 2019-06-02T16:13:10Z

That I am unaware of now.
Posting error stack here:

 run() runs the sequencer and returns output to callback

    {}
    TypeError: fs.readFileSync is not a function
        at http://localhost:41821/bundle.js:91964:23
        at Image.img.onload (http://localhost:41821/bundle.js:25664:5)
npm ERR! Test failed.  See above for more details.

harshkhandeparkar · 2019-06-02T16:14:12Z

It is browserified first. Maybe while it is browserified, fs module isn't appended. Try running the file directly in CLI.

Divy123 · 2019-06-02T16:15:00Z

Doing it now!!

Divy123 · 2019-06-02T16:17:12Z

No benefits!!

jywarren · 2019-06-20T22:43:30Z

Ah but:

I kind of had that feeling about pace! It can be disabled with ui false now!

That wasn't responding to our question about removing the browser-environment meta-module test. It might be a suggestion about how to work around the error and re-instate the browser meta-module test. Maybe this is what you did just now? Let me look! Thanks!

jywarren

OK, made a few suggestions and we should be OK. I do think it's worthwhile trying to identify the environment where this should run ahead of time, instead of using catch to just default to the fallback for /any/ error that may occur -- but at least logging out the error will help us better understand what actually happened instead of assuming it's just not the environment we want. How does this sound?

jywarren · 2019-06-20T22:44:46Z

src/modules/_nomodule/PixelManipulation.js

+      try {
+        var pace = require('pace')(pixels.shape[0] * pixels.shape[1]);
+      } catch (e) {
+        options.inBrowser = true;


Is this a good assumption? If our error is TypeError: fs.readFileSync is not a function shouldn't we try to work around it by disabling ui as @tech4GT suggested?

Disbling ui is not actually working here.

src/modules/_nomodule/PixelManipulation.js

tech4GT

@Divy123 @jywarren I think pace is not very well maintained, and looking through their github it's a good idea to move away from that. Let's completely deprecate pace based progress reporting and replace it with a prompt for now.
@Divy123 I am available throughout the weekend if you need any help with that, if you need me to do it, that is OK too! Anyway, we also need to update the associated documentation.
I hope this also solves the issues we were having over on the is-app.
You have done amazing work here, we'll soon merge this!!

tech4GT · 2019-06-21T06:02:54Z

The documentation is a part of contributing.md file mainly.
Also the progressObj is deeply integrated with the system, and that API is fine.
I'll try to explain:
What happens is that we have a progressObj which is a normal spinner. Pixel manipulation overrides that with it's own pace object and uses that to report progress. The idea was to make this customizable on a module per module basis. So let's first try removing 'pace', the way to do that will be to remove all code associated with it inside pixelManipulation. Also, inside every module which uses it, we would have to remove the code which stops the spinner.
Then we can disable the spinner too, maybe based on ui value.
@Divy123 Does that make sense?

Divy123 · 2019-06-21T07:41:05Z

Ya sure @tech4GT, looking into it.

Thanks a lot!!

tech4GT

Great work here! Let's just make sure our tests pass and the travis builds. Once that is done, we should be ready to merge! 🎉

Divy123 · 2019-06-21T12:08:40Z

@tech4GT tests have passed!!

tech4GT · 2019-06-21T12:11:03Z

Great! @jywarren I think this is ready to merge then! I had a call with Divy, and he is going to refactor the progress reporting API shortly, but for now this should be enough!! Awesome!
Also, @Divy123 Can you post a screenshot of your terminal after you run it through the CLI

Divy123 · 2019-06-21T12:13:51Z

@tech4GT are you talking of the tests?

tech4GT · 2019-06-21T12:14:47Z

No, just cd into image sequencer and run this

node ./index.js -s "brightness"

Divy123 · 2019-06-21T12:15:32Z

Sure!!

tech4GT · 2019-06-21T12:15:35Z

I just want to make sure that the spinner is working fine now!

Divy123 · 2019-06-21T12:16:31Z

I am getting

Can't read file.

tech4GT · 2019-06-21T12:16:50Z

Hmm, just a sec.

tech4GT · 2019-06-21T12:19:16Z

Oh, you have to give an image as well! It does not use the sample image automatically!

node ./index.js -i "<path-to-image>" -s "brightness"

Divy123 · 2019-06-21T13:12:12Z

@tech4GT here we are!!

Divy123 · 2019-06-21T13:20:23Z

Also the output is present in ./output

tech4GT · 2019-06-21T14:18:01Z

This looks great!! That is exactly what we expected!
cc @jywarren

jywarren · 2019-06-21T14:54:46Z

Perfect. Thanks both very much for your thoroughness, patience, care, and excellence! I'm so happy to merge this! 🎉

tech4GT · 2019-06-21T14:55:41Z

Awesome! 🎉
@jywarren I'll pull these changes into is-app. Maybe this will solve our problems there too!!

jywarren · 2019-06-21T14:59:08Z

Hooray!!!!

…

On Fri, Jun 21, 2019, 10:56 AM Varun Gupta ***@***.***> wrote: Awesome! 🎉 @jywarren <https://github.com/jywarren> I'll pull these changes into is-app. Maybe this will solve our problems there too!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1093?email_source=notifications&email_token=AAAF6J5F6XDVX4V7QIL3K6TP3TTW5A5CNFSM4HQUJWO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYIWHMY#issuecomment-504456115>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JZOPALLKS6NENSEFW3P3TTW5ANCNFSM4HQUJWOQ> .

skilfullycurled · 2019-06-21T22:40:00Z

This was a really great thread to watch unfold. Congratulations on figuring everything out. It was really cool to to see the real time collaboration!

Divy123 added 2 commits May 30, 2019 01:44

Add wasm code

2019622

Merge remote-tracking branch 'upstream/main' into wasm

1ea2d20

Divy123 mentioned this pull request May 29, 2019

explore acceleration w WebAssembly #400

Closed

Divy123 requested review from jywarren and tech4GT May 29, 2019 20:19

First working model

f7f8f3a

jywarren requested changes Jun 20, 2019

View reviewed changes

tech4GT requested changes Jun 21, 2019

View reviewed changes

tech4GT mentioned this pull request Jun 21, 2019

Load testing! publiclab/image-sequencer-app#19

Open

5 tasks

Correct doc and removed pace fuctionality

6f4fc3b

tech4GT approved these changes Jun 21, 2019

View reviewed changes

jywarren approved these changes Jun 21, 2019

View reviewed changes

jywarren merged commit 30659d4 into publiclab:main Jun 21, 2019

This was referenced Jun 21, 2019

Crashing with big images in demo #1122

Closed

Add WASM acceleration to PixelManipulation code #1068

Closed

jywarren mentioned this pull request Jul 12, 2020

Bump to v3.6.0 for new npm release #1695

Merged

harshkhandeparkar added this to Done in v3.6.0 Oct 17, 2020

Using wasm to accelerate PixelManipulation.js #1093

Using wasm to accelerate PixelManipulation.js #1093

Conversation

Divy123 commented May 29, 2019 • edited

Divy123 commented May 29, 2019

Divy123 commented May 31, 2019

jywarren commented May 31, 2019

jywarren commented May 31, 2019

Divy123 commented Jun 1, 2019

Divy123 commented Jun 1, 2019

tech4GT commented Jun 1, 2019

Divy123 commented Jun 2, 2019 • edited

jywarren commented Jun 2, 2019 via email

Divy123 commented Jun 2, 2019

Divy123 commented Jun 2, 2019 • edited

harshkhandeparkar commented Jun 2, 2019

Divy123 commented Jun 2, 2019

harshkhandeparkar commented Jun 2, 2019

Divy123 commented Jun 2, 2019 • edited

harshkhandeparkar commented Jun 2, 2019

Divy123 commented Jun 2, 2019

harshkhandeparkar commented Jun 2, 2019

Divy123 commented Jun 2, 2019

harshkhandeparkar commented Jun 2, 2019

Divy123 commented Jun 2, 2019

harshkhandeparkar commented Jun 2, 2019

Divy123 commented Jun 2, 2019

Divy123 commented Jun 2, 2019

jywarren commented Jun 20, 2019

jywarren left a comment

Choose a reason for hiding this comment

jywarren Jun 20, 2019

Choose a reason for hiding this comment

Divy123 Jun 21, 2019

Choose a reason for hiding this comment

tech4GT left a comment

Choose a reason for hiding this comment

tech4GT commented Jun 21, 2019 • edited

Divy123 commented Jun 21, 2019

tech4GT left a comment

Choose a reason for hiding this comment

Divy123 commented Jun 21, 2019

tech4GT commented Jun 21, 2019

Divy123 commented Jun 21, 2019

tech4GT commented Jun 21, 2019 • edited

Divy123 commented Jun 21, 2019

tech4GT commented Jun 21, 2019

Divy123 commented Jun 21, 2019

tech4GT commented Jun 21, 2019

tech4GT commented Jun 21, 2019 • edited

Divy123 commented Jun 21, 2019

Divy123 commented Jun 21, 2019

tech4GT commented Jun 21, 2019

jywarren commented Jun 21, 2019

tech4GT commented Jun 21, 2019

jywarren commented Jun 21, 2019 via email

skilfullycurled commented Jun 21, 2019

Divy123 commented May 29, 2019 •

edited

Divy123 commented Jun 2, 2019 •

edited

Divy123 commented Jun 2, 2019 •

edited

Divy123 commented Jun 2, 2019 •

edited

tech4GT commented Jun 21, 2019 •

edited

tech4GT commented Jun 21, 2019 •

edited

tech4GT commented Jun 21, 2019 •

edited