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.

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

Additional per-module image pass-through tests #1

Closed
wants to merge 3 commits into from

Conversation

jywarren
Copy link
Member

@jywarren jywarren commented Mar 4, 2017

No description provided.

@jywarren
Copy link
Member Author

OK, I'm pretty close here. The issue is that in nodejs, there's no native JavaScript Image class, so we're working with, for example, the Canvas.Image built into node-canvas -- which isn't identical. For example, if I run this in a nodejs console:

$ var img = new Image()
$ img.onload = function() { console.log('LOADED!') }
$ img.src = "DATA URL HERE"
$ img.src
> ""

Basically it doesn't let me assign img.src the way you could with a native browser JS image.

This does work in a browser, and is essentially what we're trying to do here: https://github.com/publiclab/image-sequencer/blob/master/src/modules/ImageThreshold.js#L29-L33

I also tried doing:

img.src = new Buffer("DATA URL HERE", "base64");

Since that's how node-canvas operates, apparently -- but it didn't work.

@ccpandhare - any ideas about how to approach this? I'm looking through various other Image replacements offered in nodejs, like: https://www.npmjs.com/search?q=image&page=1&ranking=optimal

Also, more docs on node-canvas's Image implementation: Automattic/node-canvas#122 (in case we can get it working, just not natively, or perhaps we could shim their implementation to provide a closer match to browser behavior)

@ccpandhare
Copy link
Collaborator

@jywarren I'll have a look at this and let you know.

@ccpandhare
Copy link
Collaborator

@jywarren I had a look at the node-canvas documentation.
Sorry, I got a bit late on doing that.

I found out that node-canvas does support the src.
This is a piece of code from the documentation:

var Canvas = require('canvas')
  , Image = Canvas.Image
  , canvas = new Canvas(200, 200)
  , ctx = canvas.getContext('2d')
  , fs = require('fs');

fs.readFile(__dirname + '/images/squid.png', function(err, squid){
  if (err) throw err;
  img = new Image;
  img.src = squid;
  ctx.drawImage(img, 0, 0, img.width / 4, img.height / 4);
});

So I don't understand why img.src is giving an error. Am I missing out something here?

@jywarren
Copy link
Member Author

jywarren commented Mar 13, 2017 via email

@ccpandhare
Copy link
Collaborator

ccpandhare commented Mar 14, 2017

I was able to run this on my local machine successfully:

var Canvas = require('canvas');
var Image = Canvas.Image;
var img = new Image;
var data = new Buffer("data:image/gif;base64,somethingsomethingsomething","base64");
img.src = data;
console.log(img);

Earlier I was getting an unexpected token error, but that was because the string was in multiline.

Output

[Image]

@ccpandhare
Copy link
Collaborator

ccpandhare commented Mar 14, 2017

@jywarren I was playing around with this, and realized that the onload doesn't fire. This issue is faced by many node-canvas users, as you referenced. Does the onload work on your machine?

I don't think the issue is with img.src but instead is with the .onload .

@ccpandhare
Copy link
Collaborator

Also, I couldn't understand why the Travis CI Build is failing. Could you please explain that?

@jywarren
Copy link
Member Author

Hmm, but my question was about when you assign img.src, then you enter img.src in the console, it returns an empty string, even though we just assigned it a value. Do you think we need to trigger something besides just assigning the value?

@jywarren
Copy link
Member Author

The travisCI build is failing i believe because in includes tests for several modules to check that they accept an image and produce an image, but one of them is failing due to this non-browser Image implementation.

@ccpandhare
Copy link
Collaborator

ccpandhare commented Mar 14, 2017

Hmm, but my question was about when you assign img.src, then you enter img.src in the console, it returns an empty string, even though we just assigned it a value. Do you think we need to trigger something besides just assigning the value?

Yes, I saw that. That's really odd. We needn't trigger something for that to happen.
Just a thought though:

img = new Image()
ctx.drawImage(img,...)

This returns an error beacuse at the point of drawing img has 'not loaded'.

So, maybe, similarly:

img = new Image()
img.src = "something"

when the src attribute is added, img has not loaded?

Is that the case, @jywarren ?

@jywarren
Copy link
Member Author

jywarren commented Mar 14, 2017 via email

@ccpandhare
Copy link
Collaborator

You are right.

Also, I went through and tried out a few other npm modules for images.
I had problems installing them. So couldn't come up with the answer to whether we should use them or not.

@jywarren
Copy link
Member Author

OK, found some promising possible image lib replacements: sharp, jimp, node-images, lwip

Going to be trying them out while traveling.

@jywarren
Copy link
Member Author

@ccpandhare
Copy link
Collaborator

ccpandhare commented Mar 21, 2017 via email

@jywarren
Copy link
Member Author

Aha - got a pure data url based image replacement, here:

var Jimp = require('jimp');

var string = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgAgMAAAAOFJJnAAAABGdBTUEAALGPC/xhBQAAAAFzUkdCAK7OHOkAAAAgY0hSTQAAeiYAAICEAAD6AAAAgOgAAHUwAADqYAAAOpgAABdwnLpRPAAAAAxQTFRFAAAABQkEaqiYV8E6eKx7SQAAAAF0Uk5TAEDm2GYAAAABYktHRAH/Ai3eAAAACXBIWXMAAABIAAAASABGyWs+AAAAXUlEQVQY04XOoQ7AMAgEUEzNTH/tDAbTr8PU7Otmahhk2VY6sQviicsFIma6cqPQhgzUvkJEf9GkMxJsqK8mGCBYIB+YacbhN6HUgD83w1pCtPcxgz3SlV/4UhgPToo5Yg32KuZBAAAAJXRFWHRkYXRlOmNyZWF0ZQAyMDE3LTAxLTMwVDE3OjExOjM4LTA1OjAwCQ+zKAAAACV0RVh0ZGF0ZTptb2RpZnkAMjAxNy0wMS0zMFQxNzoxMToyNS0wNTowMNUvasoAAAAASUVORK5CYII=';

console.log(string);

var regex = /^data:.+\/(.+);base64,(.*)$/;

var matches = string.match(regex);
var ext = matches[1];
var data = matches[2];
var buffer = new Buffer(data, 'base64');

Jimp.read(buffer, function(err,img) {
  img.invert();
  img.getBase64('image/png', function(err,_img) {
    console.log(_img);
  });
});

Here we echo out the 'before' image, read it in, invert it, and output the 'after' image, both as data-urls.

Ok, so a few more issues; it's not super efficient to pass as data urls, but it is a guarantee of parse-ability by either commandline or browser. But I'm using Jimp, which has multiple means of outputting images.

I think this can be used to replace the default Image class, and think we should perhaps write a wrapper class called ISImage (or somthing) which will detect isBrowser and act accordingly.

This raises the question of whether we should require a native browser Image object as the API, since it's not standard on nodejs. For broad compatibility we could standardize on data urls, but that's not very efficient (although that's often done in using the Canvas API in browsers). We might also want to consider allowing image buffers, which could result in better performance on larger images, in some cases.

@ccpandhare -- thoughts on this as a way forward? We could start by implementing on data-urls, and get it running in node and browser, then move forward with more optimization once it's basically working?

@jywarren
Copy link
Member Author

Note that we could explore other options too, but JIMP seems pretty solid, described as An image processing library for Node written entirely in JavaScript, with zero external or native dependencies. -- it's more full-featured than sharp, no native compilation necessary as in lwip.

https://github.com/zhangyuanwei/node-images is the only one that seems comparable from my search.

@jywarren
Copy link
Member Author

Actually, it occurs to me that we may just be able to get everything to pass by either:

  1. simply using data urls instead of native browser image objects
  2. somehow trying to create a shim for Image by using the jimp module... but can we trigger something upon image.src being assigned? I'm not familiar with that kind of listener, but if so, we could properly mock up a fake Image class that acts just like the browser version.

Actually i wonder if (2) is just what node-canvas is doing, minus the triggering... like, if we could get it to trigger onload, maybe it'd just work. In any case, some more abstract Image class which can accept data-urls, buffers, or whatever might be a good idea, so that the next module can receive multiple types and simply adapt.

@ccpandhare
Copy link
Collaborator

ccpandhare commented Mar 22, 2017

@jywarren Yes, Jimp is standalone and pretty solid, and will have a better performance. I think we should consider using it after all!
_

I think this can be used to replace the default Image class, and think we should perhaps write a wrapper class called ISImage (or somthing) which will detect isBrowser and act accordingly.

Yes. I agree. we need a standard way of dealing with images. This would make the code much more clean and anyways, we are using 'isBrowser' at other places. Currently the functioning of image-sequencer is too browser-ish (for the lack of a better word). This would standardize things.

@ccpandhare
Copy link
Collaborator

I think that using Jimp would be a better option. Developing an image class on our own would shift the focus from Image Sequencer to that, because we'd have to test that as well, make it efficient and solve issues for it too. We can instead build-up on Jimp and make it work the way we want, just like the browser counterparts. As far as I have read, Jimp is void of external dependancies and is very fast as compared to ImageMagick, which is considered to be very good. What do you think?

@ccpandhare
Copy link
Collaborator

@warren I was playing with Jimp and found it great. Although it doesn't have an Image class as we were looking for, the code:

Jimp.read(path,function(err,img){
    img.doSomething();
});

would be equivalent to:

img = new Image();
img.onload = function(){
    img.src = path;
    img.doSomething();
}

Also, we wouldn't have to worry about the onload anymore. So this does look like a solid example.

@ccpandhare
Copy link
Collaborator

ccpandhare commented Mar 28, 2017

Looks likeJimp is giving some trouble with dataurls & buffers:

var Jimp = require('jimp');
var data = "..something..";
var buffer = new Buffer(data,"base64");
Jimp.read(buffer).then(
  function(img){
    console.log(img);
  }).catch(function(err){
    console.error(err);
  });

This gives An error. (Error: mime must be a string)
Entering a dataURL in Jimp.read(...) gives an ENOENT error, as expected.

whereas with Sharp, things work out very well.
The code:

sharp = require('sharp');
var data = "..something.."
buffer = new Buffer(data,"base64");
sharp(buffer).toFile('PATH');

works flawlessly.

Sharp has a greater user base and is regularly updated and also works flawlessly.
I think Sharp would be a better option.

Have a look at this: Jimp vs Sharp

@jywarren What do you think? Am I doing something wrong in the Jimp implementation?

@jywarren
Copy link
Member Author

I'm not sure if you're doing anything wrong, but if Sharp works for you, that's great. Do you want to try getting the basic tests in this PR running using Sharp, somehow? Not all the tests need to run, but just seeing that the system itself can be booted up in Nodejs would be great. I may have time to work on this in the coming week but I'm not sure.

I really like how you show equivalent code for JIMP and browser Image implementations. That's what I'm thinking we might want to wrap in an Image adapter of some kind -- so modules don't have to think about whether it's inBrowser or not, and can use the Image adapter. Ideally it'd work the same as Image, but even if it doesn't, we could keep it as simple as possible.

@jywarren
Copy link
Member Author

However, Sharp uses an external library -- any thoughts on the pros/cons of this? We could think about how to make it as easy as possible to use either JIMP or Sharp by having a very simple modular implementation. That might make the code more portable to different architectures.

@ccpandhare
Copy link
Collaborator

ccpandhare commented Mar 30, 2017

Do you want to try getting the basic tests in this PR running using Sharp, somehow?

I am working on this. But firstly I need to have a look at sharp more thoroughly. And then implement it. I'm working on it locally. Will keep you updated.

@ccpandhare
Copy link
Collaborator

Yes, Sharp has external dependancies.
Possible advantages:

  1. The dependancies have their own developers. These dependancies have their own testing. Hence, every small part is tested independently. This ensures stability.
  2. More updates

Possible disadvantages:

  1. Harder to maintain as the Number of dependancies for Image Sequencer increase.

@jywarren jywarren mentioned this pull request May 4, 2017
@ccpandhare
Copy link
Collaborator

Should I close this now?

@jywarren
Copy link
Member Author

OK, i'm closing it but opening a new issue requesting per-module image pass through tests: #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants