Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Issues With ImageManipulation.js #25

Closed
ccpandhare opened this issue Jun 4, 2017 · 31 comments
Closed

Issues With ImageManipulation.js #25

ccpandhare opened this issue Jun 4, 2017 · 31 comments
Assignees
@ccpandhare
Labels
bug help wanted

Comments

@ccpandhare
Copy link
Collaborator

There is some problem with the savePixels part of the ImageManipulation.js with all images.
Some pixels don't get saved and also the saturation of the image, somehow, decreases.

Before:
Timetable Before

After:
Timetable After

In fact, the "After" image is corrupted, those at the bottom are empty pixels and not white pixels.
The module used with ImageManipulation.js was DoNothingPix.js. This module just includes ImageManipulation.js without changing any pixels. This module was made specifically for this purpose.

I believe the number of empty pixels isn't proportional to image size. This is because larger images have smaller area of "empty" pixels. For example:
Before:
Sundar Before

After:
Sundar After

One might be tempted to think that the "After" image of Sundar Pichai is perfectly fine, but upon close inspection, it comes out that there is a thin strip of empty pixels at the bottom, and also, the image is slightly desaturated.

Maybe this is a flaw with save-pixels module (the desaturation part).
We are surely missing something due to which we are gettng the blank strip of pixels.
@ccpandhare
Copy link
Collaborator Author

Update: Been working on it for the past two days. The same code used to work flawlessly on Browsers. Also, the DataURL is corrupted it seems.

The latest commit is on PR #24

@jywarren
Copy link
Member

jywarren commented Jun 7, 2017

First, let me say it's really cool to be seeing image processing before/after images, even if there is a problem. We should put such examples into the README as it really brings the project to life!

To be clear, are you saying that it does work in browsers, but not in node, or that it used to work in browsers, and now works in neither?

Second, here's the link to the save-pixels package: https://www.npmjs.com/package/save-pixels

Here's the source: https://github.com/scijs/save-pixels/blob/master/save-pixels.js, and it has a test suite here: https://github.com/scijs/save-pixels/blob/master/test/test.js

Is the saturation issue reproducible using just the save-pixels module? If so, we could submit a test to demonstrate it.

My instincts are:

  1. the saturation could relate to a mistaken dynamic range - are values stored as 0-255, but in the original file, they have far more depth?
  2. A debugging method: you could try to sample single pixels and see if there is a consistent change, like is it 33% lower or higher value?
  3. If you rerun the module on the desaturated output image, does it desaturate further, or is it just the same? If it's the same, that supports the (1) theory above, right?
  4. It's possible that defaults for precision vary between browser and nodejs... i don't know. The library seems designed for nodejs use, so knowing if this occurs only on the browser would be helpful. Maybe the maintainer of save-pixels would be willing to release a new version to address this?

@jywarren
Copy link
Member

jywarren commented Jun 7, 2017

One odd thing is that the larger images don't seem as affected by the cutoff issue. I'd have guessed that this was due to running out of memory or something, but i'm interested in whether the cutoff with transparent pixels happens the same way each time, or if it's gets a different amount through the image each time. This would tell us if it's a consistent baked-in limitation or if it's some other issue.

@jywarren
Copy link
Member

jywarren commented Jun 7, 2017

Also - where's the code for DoNothing? can you link that too?

@ccpandhare
Copy link
Collaborator Author

Hey @jywarren Thanks for the debugging ideas. Will look into them later today.

Clarification: The save-pixels thing did work on browsers earlier. Earlier I did get the same once issue but i really don't remember how I fixed that. As of now, due to lack of a proper UI, testing this on a browser is really tedious. That is due to the fact that we can not access user files due to browser limitations, unlike node.js. So the user must enter a file via the HTML File Input. Since I was focusing on the functionality, I left out the the UI part as of now. So all I know is that we don't get any problems on the browser one (the one in ./index.html) No disappearing pixels, no saturation issues. This is an older browserified version, though.

The Code for the DoNothing module is in PR #24

@ccpandhare
Copy link
Collaborator Author

ccpandhare commented Jun 10, 2017 •
edited

Update: The issue of incomplete pixels occurs only with PNG images and not with JPEG images.
But the desaturation problem is still there.

@jywarren
Copy link
Member

jywarren commented Jun 10, 2017 via email

@jywarren
Copy link
Member

jywarren commented Jun 10, 2017 via email

@ccpandhare
Copy link
Collaborator Author

ccpandhare commented Jun 10, 2017 •
edited

Hey Jeff, Thanks a lot for your inputs! I was pondering over some things. Pretty mind-boggling results.

Some Tests

Test 1 : Creating a PNG DataURL From PNG Image (The DataURL is corrupted, image is desaturated)

      var buffer = require('fs').createWriteStream('output.txt');
      var enc = require('base64-stream').encode();
      savePixels(pixels, 'png').on('end', function() {
        //Writes a DataURL to  output.txt
        buffer.write("data:image/png;base64,"+enc.read().toString());
      }).pipe(enc);

Test 2 : Creating a binary PNG File from PNG Image (The Image is desaturated but COMPLETE)

      var buffer = require('fs').createWriteStream('output.png');
      savePixels(pixels, 'png').on('end', function() {
        //Writes an Image to  output.png
        console.log("Image Written.");
      }).pipe(buffer);

Test 3 : Creating a JPEG DataURL from PNG Image (The DataURL is not corrupted, Image desaturated)

      var buffer = require('fs').createWriteStream('output.txt');
      var enc = require('base64-stream').encode();
      savePixels(pixels, 'jpeg').on('end', function() {
        buffer.write("data:image/jpeg;base64,"+enc.read().toString());
      }).pipe(enc);

Known Test 4 : Works flawlessly on browsers. No desaturation or loss of image data

Test 5 : Comparison of JPEG DataURLs generated by The 'urify' npm module I am using to make initial DataURLs from an input image and the ones generated by get-pixels & save-pixels

There is a module, DoNothingPix which takes the output of previous step, runs get-pixels on it and then save-pixels on it.
There is another module DoNothing which takes the output DataURL of previous image and copies it directly to the output of current step without any get-pixels or save-pixels.

This is my module stack [do-nothing,do-nothing-pix,do-nothing-pix]
So there are four DataURLs, The initial one and the output of each of these three modules.

  1. Initial DataURL = A
  2. After Module 1 = B
  3. After Module 2 = C
  4. After Module 3 = D

A=B and C=D but B is not equal to C
Maybe the desaturation plays a role. The DataURL changes after do-nothing-pix but then remains constant after further do-nothing-pix modules. Maybe the encoding technique is different?

Conclusions

  • The problem is not with the save-pixels module as far as the incomplete pixels thing is concerned, the issue is with storing the output as a PNG DataURL.
  • The problem is not with PNG Images, but with storing these PNG Images as a PNG DataURL after modifying using get-pixels, save-pixels. Saving a PNG Image as a JPEG DataURL or Binary PNG works fine (except for the desaturation thing.)

Further Steps

I believe that what we are doing as

      var buffer = require('fs').createWriteStream('output.txt');
      var enc = require('base64-stream').encode();
      savePixels(pixels, 'png').on('end', function() {
        //Writes a DataURL to  output.txt
        buffer.write("data:image/png;base64,"+enc.read().toString());
      }).pipe(enc);

In the enc.read().toString() part is lacking something. As I am not so good with WriteStreams, could you help me out with this, @jywarren ? Is there a better way to convert a stream object into a dataURL, maybe using a library? I had a look at this option, but failed.

Could you please pull the development branch from ccpandhare/image-sequencer (because the changes from #24 haven't been merged in yet.) and have a look?

Some Doubts (Concerns/Follow-ups)

If you rerun the module on the desaturated output image, does it desaturate further, or is it just the same? If it's the same, that supports the (1) theory above, right?

Yes, you are right. It doesn't desaturate further.

the saturation could relate to a mistaken dynamic range - are values stored as 0-255, but in the original file, they have far more depth?

How can I verify this?

A debugging method: you could try to sample single pixels and see if there is a consistent change, like is it 33% lower or higher value?

I actually tested an image with a 100x100 red square in JPEG format. Couldn't see any noticeable change unlike the red in the before-after example shown above. (I would like to admit here that I did not check the RGB Values, Sorry for that.)

@jywarren
Copy link
Member

Hi, just so I can go through this methodically:

Could you please pull the development branch from ccpandhare/image-sequencer (because the changes from #24 haven't been merged in yet.) and have a look?

Can you tell me exactly what to try out?

In the enc.read().toString() part is lacking something. As I am not so good with WriteStreams, could you help me out with this, @jywarren ? Is there a better way to convert a stream object into a dataURL, maybe using a library? I had a look at this option, but failed.

I think that's possible -- but we need to more narrowly test the issue. I'd like to suggest you debug this by console.logging the string result and/or doing a comparison somehow if you think that's the problem. Do you suspect this because of something you read? Or (as I do) do you think it's possible that since it's a stream, it expects to run continuously, and by using toString() we may be introducing a non-asynchronous step which overloads memory or otherwise messes up?

Maybe the best way to approach this is to encapsulate the narrow portion we suspect in a separate method, and to unit test that method with actual jpg and png data to try to (with automated tests) confirm what's going wrong.

In general, I'd love to try to

And above:

How can I verify this?

My suggestion for looking for a 3x difference or something like that in the RGB values was an attempt at this. Can we narrow the error any more? Does it affect red only, or other colors? Can you run a test image with pure RGB through it? Something like these: https://duckduckgo.com/?q=calibration+images&atb=v62-4&ia=images

And then you could upload the before/after images here -- http://www.imagecolorpicker.com/ and see if the values are different by a round factor. But this is just a guess.

Finally, you could also try to post a narrow description of this error (linking back to here for full context) to StackOverflow -- a pretty good idea, i think -- and also ask your fellow students and mentors to help you brainstorm a bit...

@publiclab/reviewers -- any ideas here?

@jywarren
Copy link
Member

Did this make sense? Keep us updated -- don't want you to get stuck and frustrated! Were you able to post to StackOverflow?

@ccpandhare
Copy link
Collaborator Author

Yes, @jywarren thanks for your help!
The issue is certainly surprising. I am having a look.
Was engaged with the Phase-1 completion of the Image Sequencer, as you might have seen.
As soon as I am done with some small bugfixes and minor jobs with that, I will focus on this.
Was too saturated, so shifted my focus for a while! Hope you don't mind.

I will surely take action upon all that you have suggested and revert to you.

@jywarren
Copy link
Member

jywarren commented Jun 14, 2017 via email

@ccpandhare
Copy link
Collaborator Author

ccpandhare commented Jun 18, 2017 •
edited

I opened up an issue on the save-pixels GitHub repository. Hope that would help a bit and we'll also get some insight on these issuees:

  • Is toString() the right way to produce the DatURI
  • Why is there a desaturation happening?

@jywarren
Copy link
Member

jywarren commented Jun 18, 2017 via email

@jywarren
Copy link
Member

Hi, can you open up a StackOverflow issue too and link it from here? Thanks!

@ccpandhare
Copy link
Collaborator Author

I made the StackOverflow Issue. Here's the link.

@jywarren
Copy link
Member

OK -- let's move forward with other tasks while we wait for a response. I can do some additional testing and outreach to find a solution if we don't get anything in a few days.

@jywarren
Copy link
Member

Perhaps you should change the title on SO to include "red channel desaturated and/or cut off" and "using save-pixels" -- there's an art to a well-written issue title, and I think it'll get better input. :-)

@ccpandhare
Copy link
Collaborator Author

Done that! Thanks :)

@ccpandhare
Copy link
Collaborator Author

Guess what? I've found the problem with this!
It should be

savePixels(...).pipe(buffer).on('finish',function(){});

and not

savePixels(...).on('finish',function(){}).pipe(buffer);

@ccpandhare
Copy link
Collaborator Author

Now I feel like a fool.

@ryzokuken
Copy link
Member

@ccpandhare it happens to the best of us. I am glad you found the solution to this :)

@jywarren
Copy link
Member

jywarren commented Jun 28, 2017 via email

@jywarren
Copy link
Member

Let's get some of these issues closed up now! Things are getting exciting :-)

Be sure to update your StackOverflow and other issues with the solution!

@ccpandhare
Copy link
Collaborator Author

Thanks a lot for the motivation @jywarren and @ryzokuken! I really appreciate that!

Yes, I'll write an answer to the SO question and GitHub issue at the earliest. Thanks.

@ccpandhare
Copy link
Collaborator Author

@jywarren you could remove the bounty for this issue now :-)

@jywarren
Copy link
Member

jywarren commented Jun 28, 2017 via email

@ccpandhare
Copy link
Collaborator Author

So should I do that? Is it okay?
Not sure about that...

@jywarren
Copy link
Member

jywarren commented Jun 28, 2017 via email

@ccpandhare
Copy link
Collaborator Author

Woah, Okay! Thanks!

@ccpandhare ccpandhare self-assigned this Aug 22, 2017
@jywarren jywarren mentioned this issue Apr 29, 2018
Open
24 tasks
@jywarren jywarren mentioned this issue May 23, 2018
Closed
2 tasks
@gitmate gitmate bot mentioned this issue Nov 18, 2018
Closed
@plotsbot plotsbot mentioned this issue Jan 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ccpandhare ccpandhare

Labels
bug help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jywarren @ryzokuken @ccpandhare