Fix generateOutput called multiple times #1564

rishabhshuklax · 2020-01-22T22:48:32Z

Concerns this
Fixes #1575 Fixes #1515 Fixes #1457 Fixes #1532 (this)

Note: This PR is a work-in-progress

This PR fixes generateOutput called multiple times when extramanipulation is used.

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 run test-all
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
Insert-step functionality is working correct as expected.

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
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!

rishabhshuklax · 2020-01-22T22:56:06Z

This PR is work in progress, At the moment Resize, Crop & Constrained Crop module doesn't work correctly unless used with a setTimeout to generateOutput, This PR solves problem of QR not generating after merging #1532 and allows text-overlay module to work correctly in Node #1518 (finally!!!!)

I appreciate all the help I can get because I am stuck now :(

@jywarren @harshkhandeparkar @publiclab/is-reviewers

rishabhshuklax · 2020-01-23T12:28:02Z

I removed all the instances of setTimeout and added changePixel method to the modules, this makes them work without a problem, but I don't know why this is happening, for some reason code is async and when changePixel is added it runs after extramanipulation(which it shouldn't) and generates output, so if generate output is even removed from extramanipulation it would work fine, This type of behaviour shouldn't be in our code, this should work fine even when changePixel is not added!! And why is extraManipulation even called before changePixel?

image-sequencer/src/modules/_nomodule/PixelManipulation.js

Lines 235 to 245 in f6d5316

    
           fetch('../../../dist/manipulation.wasm').then(response => 
        
             response.arrayBuffer() 
        
           ).then(bytes => 
        
             WebAssembly.instantiate(bytes, imports) 
        
           ).then(results => { 
        
             results.instance.exports.manipulatePixel(framePix.shape[0], framePix.shape[1], inBrowser, test); 
        
             wasmSuccess = true; 
        
             resolvedFrames++; 
        
             generateOutput(); 
        
           }).catch(err => {

This piece of code seems to be the most likely culprit, so i set useWasm option on histogram to false and this was the output

😕
The reason to this seems to be this:

image-sequencer/src/modules/_nomodule/PixelManipulation.js

Lines 279 to 283 in f6d5316

    
           perPixelManipulation(); 
        
           wasmSuccess = false; 
        
           resolvedFrames++; 
        
           generateOutput();

I think we should add a more if block here like this:

if(!options.extraManipulation)
            generateOutput();

This works fine!!

@harshkhandeparkar you were working on #1515? have you found the solution to this??
We need to take care of that async code to fix this problem!

rishabhshuklax · 2020-01-23T14:41:18Z

Ok, so i changed pixelManipulation such that generateOutput will be called only once and extraManipulation will be called only after changePixel.
But, as of now resize and crop modules are not working expectedly, I'm trying to figure out why.

rishabhshuklax · 2020-01-23T15:09:48Z

At the moment these modules are not working

~~canvas-resize (this is also not working on beta site)~~
~~colorbar(will be fixed when crop module is fixed)~~
constrained-crop(will be fixed when crop module is fixed)
~~crop~~
resize
rotate

rishabhshuklax · 2020-01-23T21:38:40Z

Canvas Resize is now fixed 🎉 and I've also added an option to change the background color of the new canvas 😄

rishabhshuklax · 2020-01-24T01:06:03Z

Crop Module is also fixed 🎉

So, since crop is fixed hence colorbar is also fixed there is minor change to be made in constrained crop and it will be fixed too 😄

rishabhshuklax · 2020-01-26T01:49:42Z

AddQr test was failing because it's benchmark was wrong, probably changed in #1550, I've updated it, It should pass now!

rishabhshuklax · 2020-01-26T01:50:11Z

All modules are working fine now 🎉

codecov · 2020-01-26T02:00:12Z

Codecov Report

Merging #1564 (10322c3) into main (853e719) will decrease coverage by 0.21%.
The diff coverage is 62.84%.

@@            Coverage Diff             @@
##             main    #1564      +/-   ##
==========================================
- Coverage   66.67%   66.46%   -0.22%     
==========================================
  Files         130      132       +2     
  Lines        2686     2821     +135     
  Branches      433      440       +7     
==========================================
+ Hits         1791     1875      +84     
- Misses        895      946      +51

Impacted Files	Coverage Δ
src/Modules.js	`100.00% <ø> (ø)`
src/modules/Crop/Crop.js	`100.00% <ø> (ø)`
src/modules/WebglDistort/Module.js	`2.29% <0.00%> (ø)`
src/modules/ColorHalftone/Module.js	`3.63% <3.63%> (ø)`
src/modules/BlobAnalysis/Module.js	`25.00% <25.00%> (-5.77%)`	⬇️
src/modules/_nomodule/PixelManipulation.js	`42.74% <53.84%> (+2.06%)`	⬆️
src/modules/Average/Module.js	`100.00% <100.00%> (ø)`
src/modules/Blur/Module.js	`100.00% <100.00%> (ø)`
src/modules/CanvasResize/Module.js	`100.00% <100.00%> (+12.50%)`	⬆️
src/modules/ColorHalftone/index.js	`100.00% <100.00%> (ø)`
... and 25 more

rishabhshuklax · 2020-01-26T02:14:27Z

@publiclab/is-reviewers @jywarren @harshkhandeparkar please review this!!

rishabhshuklax · 2020-01-28T16:40:51Z

@jywarren could you please take a look at this!

jywarren · 2020-11-03T15:29:35Z

Gosh @blurry-x-face a ton of your PRs are finally getting merged! Your work is dominating the upcoming release: https://github.com/publiclab/image-sequencer/projects/4

harshkhandeparkar

A few questions :)

harshkhandeparkar · 2020-11-03T15:43:18Z

src/modules/CanvasResize/Module.js

@@ -1,6 +1,8 @@
+const ndarray = require('ndarray');


ndarray is imported here

Are you saying it shouldn't be necessary? Just checking, let's make a code suggestion perhaps?

ah i see it's required again below. I'll remove that one.

harshkhandeparkar · 2020-11-03T15:43:34Z

src/modules/CanvasResize/Module.js

-    function extraManipulation(pixels) {
-
+    function extraManipulation(pixels, setRenderState, generateOutput, frames, f) {
+      setRenderState(false);
      let newPixels = require('ndarray')(new Uint8Array(4 * options.width * options.height).fill(0), [options.width, options.height, 4]);


Imported twice

Suggested change

let newPixels = require('ndarray')(new Uint8Array(4 * options.width * options.height).fill(0), [options.width, options.height, 4]);

let newPixels = ndarray(new Uint8Array(4 * options.width * options.height).fill(0), [options.width, options.height, 4]);

harshkhandeparkar · 2020-11-03T15:44:09Z

src/modules/CanvasResize/Module.js

 /*
 * Changes the Canvas Size
 */
+const getPixels = require('get-pixels');


This new import is added but doesnt look like it is used anywhere.

Suggested change

const getPixels = require('get-pixels');

harshkhandeparkar · 2020-11-03T15:45:30Z

src/modules/ConstrainedCrop/Module.js

@@ -15,7 +15,12 @@ module.exports = function ConstrainedCrop(options, UI) {
      widthRatio = Number(aspectRatio[0]),
      heightRatio = Number(aspectRatio[1]);

-    function extraManipulation(pixels) {
+    function changePixel(r, g, b, a) {


Is this necessary? Wouldn't it add a redundant step?

harshkhandeparkar · 2020-11-03T15:47:14Z

src/modules/_nomodule/PixelManipulation.js

      if (!(renderableFrames < numFrames) && !(resolvedFrames < numFrames)) {
-
+        // if(rend){


What is this comment for?

Suggested change

// if(rend){

jywarren

Just the one clarification question for @blurry-x-face from @harshkhandeparkar -- and the other suggestions we should be able to accept and merge!

jywarren · 2020-11-10T15:50:57Z

src/modules/CanvasResize/Module.js

@@ -1,6 +1,8 @@
+const ndarray = require('ndarray');


Are you saying it shouldn't be necessary? Just checking, let's make a code suggestion perhaps?

jywarren · 2020-11-10T15:51:11Z

src/modules/CanvasResize/Module.js

 /*
 * Changes the Canvas Size
 */
+const getPixels = require('get-pixels');


Suggested change

const getPixels = require('get-pixels');

jywarren · 2020-11-10T15:52:11Z

src/modules/CanvasResize/Module.js

@@ -1,6 +1,8 @@
+const ndarray = require('ndarray');


ah i see it's required again below. I'll remove that one.

jywarren · 2020-11-10T15:52:26Z

src/modules/CanvasResize/Module.js

-    function extraManipulation(pixels) {
-
+    function extraManipulation(pixels, setRenderState, generateOutput, frames, f) {
+      setRenderState(false);
      let newPixels = require('ndarray')(new Uint8Array(4 * options.width * options.height).fill(0), [options.width, options.height, 4]);


Suggested change

let newPixels = require('ndarray')(new Uint8Array(4 * options.width * options.height).fill(0), [options.width, options.height, 4]);

let newPixels = ndarray(new Uint8Array(4 * options.width * options.height).fill(0), [options.width, options.height, 4]);

jywarren · 2020-11-10T15:52:56Z

src/modules/_nomodule/PixelManipulation.js

      if (!(renderableFrames < numFrames) && !(resolvedFrames < numFrames)) {
-
+        // if(rend){


Suggested change

// if(rend){

jywarren · 2020-11-17T22:04:37Z

Hi @harshkhandeparkar hope you're well, did you have a chance to look at my suggestions? If you agree we could go ahead and accept them...

harshkhandeparkar · 2020-11-18T05:06:00Z

I am fine, ty! How are you?

I did go through the suggestions but I was waiting for Rishabh. Do you think we should just commit those and merge or wait for him?

rishabhshuklax · 2020-11-18T05:18:02Z

Hi @jywarren @harshkhandeparkar I was a bit busy with university, I'll fix the PR ASAP, ty :)

rishabhshuklax · 2020-11-18T06:08:26Z

@harshkhandeparkar tests for edge detect are failing for some reason do you have any idea?

harshkhandeparkar · 2020-11-18T06:12:59Z

Can you upload the expected and generated outputs?

rishabhshuklax · 2020-11-18T07:29:25Z

@harshkhandeparkar

What I am getting:

What is expected:

Should I just update the benchmark?

harshkhandeparkar · 2020-11-18T07:36:30Z

New output has slightly thicker edges at some places. Maybe some defaults changed or some options are wrong?

rishabhshuklax · 2020-11-18T07:41:29Z

Blur option was wrong, updated output, still this is different from expected output

harshkhandeparkar · 2020-11-18T07:43:03Z

It is still not the same as before. But it doesn't really matter. Should we change the benchmark image?

rishabhshuklax · 2020-11-18T07:43:59Z

Yea I think we should, except edge detect all other benchmarks seems to paas, what do you think @jywarren ?

jywarren · 2020-11-18T13:23:20Z

Sure that sounds good! Yeah I think the important takeaway here is that the test is working to catch variations like this!

…

On Wed, Nov 18, 2020, 2:44 AM Rishabh Shukla ***@***.***> wrote: Yea I think we should, except edge detect all other benchmarks seems to paas, what do you think @jywarren <https://github.com/jywarren> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1564 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6JZMU7IZZSBK6QKAE73SQN3M3ANCNFSM4KKNYWHA> .

jywarren · 2022-01-25T22:52:21Z

If we update the edge detection expected output, we should be good here!

initial commit, change pixelManipulation

57f072e

rishabhshuklax added the work-in-progress label Jan 22, 2020

h added 3 commits January 23, 2020 16:39

add async/await

658ed6d

reverse last commit

480479a

remove setTimeout

71d55ef

fix changepixel getting called before extramanipulation

63d9210

h added 2 commits January 24, 2020 03:12

fix canvas resize

421fda1

fix crop module

3e6dbd4

rishabhshuklax mentioned this pull request Jan 24, 2020

[Fixed] Add Qr Module not working #1572

Closed

5 tasks

rishabhshuklax requested review from jywarren and harshkhandeparkar January 25, 2020 18:49

h added 4 commits January 26, 2020 05:39

Fix PixelManipulation

c6439e2

fix constrained crop

40827e4

fix add-qr test benchamrk

830a8ad

fix edge detect

188bb55

code cleanup

ebb444e

rishabhshuklax removed the work-in-progress label Jan 26, 2020

h added 2 commits January 28, 2020 22:14

resolve merge conflicts

13a3b8a

resolve merge conflicts

3a79526

jywarren added the high-priority label Nov 3, 2020

harshkhandeparkar reviewed Nov 3, 2020

View reviewed changes

jywarren requested changes Nov 10, 2020

View reviewed changes

rishabhshuklax added 2 commits November 18, 2020 10:56

Merge remote-tracking branch 'upstream/main' into extraManipulation-fix

5635742

fix merge conflicts

f1a8859

rishabhshuklax requested review from a team as code owners November 18, 2020 05:36

fix tests

e602cbc

harshkhandeparkar marked this pull request as draft July 27, 2021 21:34

jywarren added the almost-complete PRs that are almost done but little more work. label Jan 25, 2022

jywarren mentioned this pull request Jan 25, 2022

Checklist and coordination for v3.7.2 patch release and v3.7.3 follow-up patch release #2037

Open

18 tasks

jywarren mentioned this pull request Mar 22, 2022

Checklist and coordination for v3.8.0 minor release #2089

Open

18 tasks

	let newPixels = require('ndarray')(new Uint8Array(4 * options.width * options.height).fill(0), [options.width, options.height, 4]);
	let newPixels = ndarray(new Uint8Array(4 * options.width * options.height).fill(0), [options.width, options.height, 4]);

		if (!(renderableFrames < numFrames) && !(resolvedFrames < numFrames)) {

		// if(rend){

Fix generateOutput called multiple times #1564

Are you sure you want to change the base?

Fix generateOutput called multiple times #1564

Conversation

rishabhshuklax commented Jan 22, 2020 • edited Loading

rishabhshuklax commented Jan 22, 2020

rishabhshuklax commented Jan 23, 2020

rishabhshuklax commented Jan 23, 2020 • edited Loading

rishabhshuklax commented Jan 23, 2020 • edited Loading

rishabhshuklax commented Jan 23, 2020

rishabhshuklax commented Jan 24, 2020

rishabhshuklax commented Jan 26, 2020

rishabhshuklax commented Jan 26, 2020

codecov bot commented Jan 26, 2020 • edited Loading

Codecov Report

rishabhshuklax commented Jan 26, 2020

rishabhshuklax commented Jan 28, 2020

jywarren commented Nov 3, 2020

harshkhandeparkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Nov 17, 2020

harshkhandeparkar commented Nov 18, 2020

rishabhshuklax commented Nov 18, 2020

rishabhshuklax commented Nov 18, 2020

harshkhandeparkar commented Nov 18, 2020

rishabhshuklax commented Nov 18, 2020

harshkhandeparkar commented Nov 18, 2020

rishabhshuklax commented Nov 18, 2020 • edited Loading

harshkhandeparkar commented Nov 18, 2020

rishabhshuklax commented Nov 18, 2020

jywarren commented Nov 18, 2020 via email

jywarren commented Jan 25, 2022

rishabhshuklax commented Jan 22, 2020 •

edited

Loading

rishabhshuklax commented Jan 23, 2020 •

edited

Loading

rishabhshuklax commented Jan 23, 2020 •

edited

Loading

codecov bot commented Jan 26, 2020 •

edited

Loading

rishabhshuklax commented Nov 18, 2020 •

edited

Loading