GPU Acceleration achieved! #1038
Conversation
harshkhandeparkar
requested review from
jywarren and
tech4GT
and removed request for
jywarren
|
Ok the package was successfully installed. Now it is showing a different error. I cannot track it. It says that it cannot recognise |
|
I didn't even add something like that. |
|
@jywarren why do I have merge rights? |
|
@harshkhandeparkar I will also be reviewing this in an hour. Actually my semester lab exams have started so a bit late. Will do this in an hour. |
|
@Divy123 thanks |
|
Yes this way it will be more clear.
…On Wed 1 May, 2019, 1:16 AM Harsh Khandeparkar, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/modules/Blur/Module.js
<#1038 (comment)>
:
> @@ -15,7 +15,7 @@ module.exports = function Blur(options, UI) {
var step = this;
function extraManipulation(pixels) {
- pixels = require('./Blur')(pixels, options.blur);
+ pixels = require('./Blur')(pixels, parseFloat(options.blur));
Do you mean like options.blur = parseFloat(options.blur)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1038 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHOHJL5PTVA3B4QIAW5ARDTPTCOYBANCNFSM4HHSPRVQ>
.
|
There was a problem hiding this comment.
I have given some reviews. Looking into detail further.
Also awesome testing @harshkhandeparkar !!
|
In that case I think we should maintain a separate file because that would
make this one too heavy with some redundant data.
Maintaining two files can be better.
I mean we can keep the older version can be kept in one file and the newer
in another.
This will be easier to maintain.
Please tell me if I understood incorrectly.
…On Wed 1 May, 2019, 1:21 AM Harsh Khandeparkar, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/modules/Convolution/Convolution.js
<#1038 (comment)>
:
> @@ -1,71 +1,98 @@
var _ = require('lodash');
-module.exports = exports = function(pixels, constantFactor, kernelValues) {
- let kernel = kernelGenerator(constantFactor, kernelValues), oldpix = _.cloneDeep(pixels);
- kernel = flipKernel(kernel);
+module.exports = exports = function(pixels, constantFactor, kernelValues, texMode) {
+ let kernel = kernelGenerator(constantFactor, kernelValues);
+
+ // const oldpix = _.cloneDeep(pixels);
if we need to fall back to the old version at any time in the future, we
can use this. They are comments so they do not affect performance. I can
still remove them though.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1038 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHOHJLYXBO6BPZ3OBT7DV43PTCPLLANCNFSM4HHSPRVQ>
.
|
|
Or I can just delete the comments if not needed. Saving in a different file doesn't really do much. Should I delete them? |
|
You can do so if they are not really required.
Or you can collect them at the bottom by providing appropriate references.
…On Wed 1 May, 2019, 1:41 AM Harsh Khandeparkar, ***@***.***> wrote:
Or I can just delete the comments if not needed. Saving in a different
file doesn't really do much. Should I delete them?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1038 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHOHJL7GBQRLI6RFUDQAY2LPTCRVPANCNFSM4HHSPRVQ>
.
|
|
@Divy123 committed the changes you had requested. |
harshkhandeparkar
requested review from
jywarren,
Divy123 and
a team
and removed request for
jywarren
|
Hi @harshkhandeparkar - would you be able to add a simple test for Convolution and Edge Detect, to keep our coverage from falling? Neither of those two modules have tests, and CodeCov is pointing it out: What do you think? With that, i'd be very happy to merge this! |
|
Also, @harshkhandeparkar this is so cool. 🎉 Do you think it would be worth refactoring Webgl-distort to use this? would it be faster in a server-node environment than starting up a puppeteer instance? https://github.com/tech4GT/image-sequencer/blob/a3f1b016dacfee38b55966cab63c917094dd3d1e/src/modules/WebglDistort/Module.js#L154 |
I want to add tests for modules, all the remaining ones in a followup as soon as this is merged. Is this ok? |
Maybe, but we will have to write the distort code in JS rather than GLSL and also the compilation takes a while, a long while. The bigger the kernel, longer the time. Also, if the output image can be bigger than the input image, handling that will also require more computation. The decision is up to the community. I have just added a General Purpose GPU computation utility method called |
|
Hi, @harshkhandeparkar I would really like to see the tests associated within a single PR, and our coverage monitoring would appreciate it too! We can wait on this one for the time being if you are busy, but I think we should start accepting tests + new code together as a matter of policy, in order to really ratchet up our coverage and stability. I hope you understand! ❤️ Thank you for your fantastic work! |
|
This PR concerns GPU acceleration and that is tested. The modules are still
the same so I want to do that in a separate PR as they are not related to
this one. They just use a different tool and the tool is tested.
…On Thu 2 May, 2019, 9:26 PM Jeffrey Warren, ***@***.***> wrote:
Hi, @harshkhandeparkar <https://github.com/HarshKhandeparkar> I would
really like to see the tests associated within a single PR, and our
coverage monitoring would appreciate it too! We can wait on this one for
the time being if you are busy, but I think we should start accepting tests
+ new code together as a matter of policy, in order to really ratchet up
our coverage and stability. I hope you understand! ❤️ Thank you for your
fantastic work!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1038 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIJI5H6XINFQPEPSIWNIELTPTMFKJANCNFSM4HHSPRVQ>
.
|
|
I hear you, @harshkhandeparkar, and I really empathize with your desire to close this very large PR up. I do want to note that you've changed the internals of those modules, and I worry a little bit that without tests, we can't see that they are working in Travis. Although, without tests before, we couldn't see that they were working before either! So, because this is a gradual process, I'm going to merge this, and I think we'll all feel great about it 🙌 However, I think we will be getting more and more committed to always asking for tests with any module changes at all as time goes on. I'd LOVE it if you would consider filing a follow-up quickly to help improve our coverage over these modules specifically! Thank you so much! ❤️ 👍 🎉 |
|
@jywarren thanks a lot. I will open a follow-up PR as soon as I can. If it is possible today then today or tomorrow. |
Fixes #849
Fixes #998
Fixes #260
23 April 2019 1:48AM (IST), ImageSequencer received an Alcubierre Warp Drive Ship
EDIT: It took me 20min to write the PR body so it is technically 2:08 AM
Important: Please view any screenshots/GIFs in fullscreen mode(new tab)
Changes
Added
src/modules/_nomodule/gpuUtils.js. It exportsconvolvefor convolution andcomputefor general computing on a GPU using JS!ndarray-gaussian-blurpackage. Edge Detect was bottlenecked because ofndarray-gaussian-blur.Better
p.s: I had stopped the previews
Edge Detect is a bit more detail preserving(with the default blur value) and will need some future tweaks.
Here is a GIF of the old EdgeDetect
Blur no longer produces a black border! New blur module takes care of the black border by adding some padding to the pixels.
Convolution module runs at 60fps
This is the old one for comparison
Things not done
Best Part: It all works in NodeJS
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test@publiclab/is-reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto 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!