preco21 / merge-img

Merge multiple images into a single image
MIT License
94 stars 29 forks source link

img.getMIME is not a function #5

Open turakvlad opened 6 years ago

turakvlad commented 6 years ago

I have tried to use your sample but it does not work for me with Node 8.9.4 (of course, I have provided the paths to the real images on my local machine). I am getting img.getMIME is not a function.

Thanks for the help.

turakvlad commented 6 years ago

One more thing. As I can see you're using: img.getBuffer(img.getMIME(), (buf) => console.log(buf));

I think it's not right because jimp uses error-first callback here and the first argument will contain an error and the second will contain the result (buffer in our case). Please, take a look at this - https://github.com/oliver-moran/jimp#writing-to-buffers. In comments there are

Node-style callback will be fired with result

It should be img.getBuffer(img.getMIME(), (error, buffer) => console.log(buffer));

I am still investigating the issue with getMIME(). It works fine with this code:

jimp.read("sample.jpg").then(image => {
    console.log(image.getMIME());
}).catch(console.log);

But this function does not work with the result of your mergeImg function. I mean that this object (https://github.com/preco21/merge-img/blob/master/src/index.js#L84) does not have getMIME() method on itself.

preco21 commented 6 years ago

Thanks for reporting the issue!

One more thing. As I can see you're using: img.getBuffer(img.getMIME(), (buf) => console.log(buf));

Yes, indeed this is wrong usage. I made this for a long time ago and I could not find until you report it 😂. I'm going to fix the document now. Again, thanks for reporting this.

Let's get back to the original issue here, I assume this is probably upstream issue: img.getMIME is not a function.

I could find such method on the upstream source code: https://github.com/oliver-moran/jimp/blob/f7b5e5b543b012069c513ae8a2368c388e54e6ad/index.js#L834-L837

However, I'm not quite sure when this method is applied in which version. But I could find that the getMIME() method is added over about a year ago: https://github.com/oliver-moran/jimp/commit/3e707347fb1ff6e1f1bc1d110de62b3c8aeb7f63#diff-168726dbe96b3ce427e7fedce31bb0bcR697

@turakvlad Could you provide which version of jimp you used for your experiments?

turakvlad commented 6 years ago

@preco21, no problem. I am happy to help.

I am using 0.2.28 for my experiments. As I can see, you're using 0.2.27. I've just played with these versions a little bit and ... the problem is actually with 0.2.27. I have updated jimp from 0.2.27 to 0.2.28 in merge-img package in my local node_modules folder and now it works like a charm.

Please, try it on your side. If everything is okay, it looks like we resolve this issue :smiley:

turakvlad commented 6 years ago

You were right. getMIME() was added just 2 commits after releasing 0.2.27. The 0.2.27 was released on August, 12 and the support for getMIME() was added on August, 13.

Releasing 0.2.27 version - https://github.com/oliver-moran/jimp/commit/2dbb58f974dbd4f6639ca05edfd5a1f4eb1efd64. Adding support for getMIME() - https://github.com/oliver-moran/jimp/commit/3e707347fb1ff6e1f1bc1d110de62b3c8aeb7f63 selection_018

preco21 commented 6 years ago

@turakvlad Hmm, I think this is going to be much more tricky: https://github.com/preco21/merge-img/issues/3

I have downgraded it because of the issue causing some sort of errors on browser. Actually this should not be an issue since this module is focused on node environment. But, as you know, better than having nothing.

jimp has infrequent releasing cycle. They didn't update the module after releasing v0.2.28 and there are a number of commits made to their module ever since that release.

The thing is that they have different build to support browser:

I believe this issue was on the use of browser build of the module in the node-like environment like Electron via webpack.

Since webpack automatically manages the environment as like node, so the developers don't have a reason to use browser build and webpack will do rest of jobs.

So hacky and simple the workaround is to change mainFields option in webpack from user side.

According to jimp collaborator, he mentioned that the module currently published to npm is not identical to one in the repo: https://github.com/oliver-moran/jimp/issues/280#issuecomment-306420101

So I installed the module via npm directly and I found this:

carbon

I can confirm that is not the same one. Since this is an upstream issue, I don't think we could solve it from our side.

Thoughts?

turakvlad commented 6 years ago

Hi, @preco21!

I am sorry for my delayed response.

I have downgraded my jimp from 0.2.28 to 0.2.27 because of this issue - https://github.com/oliver-moran/jimp/issues/262. This problem was crucial for me (and I think not for me only).

To my mind, 0.2.28 is not stable and "buggy" a little bit.

Maybe, it would be better not to upgrade jimp to 0.2.28 and leave it as now - 0.2.27. I just recommend you to update this sample (remove img.getMIME() function) in your README.md.

preco21 commented 6 years ago

@turakvlad Alright, thanks for pointing out that. I’ll check it.

elipeters commented 6 years ago

Probably need to update the readme on NPM also..

elipeters commented 6 years ago

I'm also noticing a third reduction in file size between img.getMIME() over implicitly setting the mime type to Jimp.MIME_JPEG

All source images I used are jpg and the result is 367.8 KB to 212.2 KB.

preco21 commented 6 years ago

@elipeters If I understand what you meant correctly: Using img.getMIME() instead of Jimp.MIME_JPEG produces different file sizes even if you match every input image's mime type to jpeg?

If so, I guess this is yet another upstream issue. As for now, merge-img does not take handle image processing itself. Instead, it's essentially relying on another library. As you know it's jimp.

I have plan to make it more like a general purpose in v3, still working on it.

Dogatorix commented 6 years ago

So now that getMIME is removed from merge-img, now what?

preco21 commented 6 years ago

@PokeTehDoge Now things left is to make some stuff better.

But I have been so busy for a couple of months so I couldn’t get to make some stuff for this module.

I’ll start work on it as soon as I finish my job.

But, always PRs are welcome.