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

Changed exec for spawn, added better params handling & parsing multiple files #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jmunox
Copy link

@jmunox jmunox commented Oct 31, 2016

Hi Yves,

I made some changes on the code.

  1. Instead of calling 'exiftool' through 'child_process.exec', it calls: child_process.spawn which avoids buffer limitations.

  2. It also allows to send specific arguments to the 'exiftool' shell command

  3. It can read EXIF info from multiple files at once by calling the path to a folder instead of a file

You can check on https://github.com/jmunox/node-exif

Cheers

Jesus

…ild_process.exec', it calls, 'child_process.spawn' which avoids buffer limitations. It also allows to send specific arguments to the 'exiftool' shell command and to read EXIF info from multiple files at once.
Copy link

@Offirmo Offirmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello ! Nice PR

@@ -15,10 +15,20 @@
"main": "index",
"repository": {
"type": "git",
"url": "https://github.com/Yvem/node-exif.git"
"url": "https://github.com/jmunox/node-exif.git"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed it too early !

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change that back and remove from npm. I just wanted to do a full test, including testing it as a module. What should i do?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's completely fine publishing you own version on npm (open source rulz !) However, you should not include the name/repo change in the PR to the initial repo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it back to the original repo.

@@ -4,7 +4,8 @@ Image meta-information (EXIF, IPTC, XMP...) extraction using [exiftool](http://w

__NOTE__: This fork from https://github.com/Yvem/node-exif has a DIFFERENT
(improved !) API.
Changed the implementation.

Mayor Changes:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major

@Offirmo
Copy link

Offirmo commented Nov 2, 2016

However, I can't be merged because:

  1. you changed it to a fork too early, changing the module npm name and repo
  2. using spawn to circumvent the memory limitation is a "false good idea": what if the processed image is a crafted image to cause a memory explosion ? Being able to limit the memory used for processing is a feature.

@Offirmo
Copy link

Offirmo commented Nov 2, 2016

The other features are nice.

@jmunox
Copy link
Author

jmunox commented Nov 2, 2016

hi,
I understand what you say with point 2, but I think that if an image is crafted in such a way, the memory explosion might happen first on the exiftool.

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