-
Notifications
You must be signed in to change notification settings - Fork 210
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
Using wasm to accelerate PixelManipulation.js #1093
Conversation
The C code is:
|
@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!! |
Oh, i'm not familiar with 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 |
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 |
@tech4GT , can you elaborate something about the use of pace and what is pace.op() ? |
Thanks @jywarren . Looking into the issues now. |
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. |
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>
.
|
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? |
The issue I am facing is fs module not working. @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. |
@Divy123 do you have a code snippet? |
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); |
What is the test command? which file tests this? all(most) of them? |
Basically run() is not working as it involves execution of this piece of code in some modules of ours. |
So the test file is run.js? |
Test file is image-sequencer.js in test folder. |
Ok. Looking into it |
Thanks a lot !! |
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? |
That I am unaware of now. 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. |
It is browserified first. Maybe while it is browserified, fs module isn't appended. Try running the file directly in CLI. |
Doing it now!! |
No benefits!! |
Ah but:
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! |
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.
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?
try { | ||
var pace = require('pace')(pixels.shape[0] * pixels.shape[1]); | ||
} catch (e) { | ||
options.inBrowser = true; |
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.
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?
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.
Disbling ui is not actually working here.
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.
@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!!
The documentation is a part of contributing.md file mainly. |
Ya sure @tech4GT, looking into it. Thanks a lot!! |
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.
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! 🎉
@tech4GT tests have passed!! |
@tech4GT are you talking of the tests? |
No, just cd into image sequencer and run this node ./index.js -s "brightness" |
Sure!! |
I just want to make sure that the spinner is working fine now! |
I am getting Can't read file. |
Hmm, just a sec. |
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" |
|
Also the output is present in ./output |
This looks great!! That is exactly what we expected! |
Perfect. Thanks both very much for your thoroughness, patience, care, and excellence! I'm so happy to merge this! 🎉 |
Awesome! 🎉 |
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>
.
|
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! |
Fixes #400
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test
@publiclab/is-reviewers
for help, in a comment belowIf 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!