gomfunkel / node-exif

A node.js library to extract Exif metadata from images.
MIT License
581 stars 104 forks source link

Better output data structure #11

Closed tremby closed 11 years ago

tremby commented 11 years ago

Is there any particular reason why the output data structure is so obtuse? Maybe I'm missing something obvious, but for every bit of data I want to extract I have to loop through all the array members until I find one with the right tagName property. Seems very inefficient.

gomfunkel commented 11 years ago

You're right, there is no easy way to access the information by tag name right now. That's something pretty much on top of my todo list, though. Any wishes on how this should look like?

One idea is to provide a method like this:

image.getTagValue(String tagName, Enum exifSection);
image.getTagValue('ExposureTime', makernote);

Or like this:

image.makernote.getTagValue(String tagName);
image.makernote.getTagValue('ExposureTime');

Another idea is to provide the tag name as properties of the exif objects like this:

image.exif['ExposureTime'];

Maybe both? Something different? Any thoughts?

tremby commented 11 years ago

I was thinking the last version, but any of those would be better than what's there now! :)

gomfunkel commented 11 years ago

The version you wished for is now available as of version 0.3.0, along with many other changes.

Documentation will be updated later on. Thanks for opening issue!

tremby commented 11 years ago

It's an improvement but it still isn't the most usable. For instance, some of the output looks like this:

> data
{ image: 
   { ImageWidth: 
      { tag: <Buffer 00 01>,
        tagName: 'ImageWidth',
        format: 4,
        components: 1,
        valueOffset: 30,
        value: 2560 },
...

If I want that image width, I have to test if data.image exists, then if data.image.ImageWidth exists, then I can use data.image.ImageWidth.value, or I risk throwing a TypeError when we try to read properties of undefined.

Are the properties other than value actually useful to anyone? Perhaps it's useful for the module itself to know (for instance it might want to write the values back to the image file and so may still need offsets etc) but exposing them to the user is surely way out of scope. It'd be quite an improvement in usability, I think, if those values were all skipped so you have in this case something more like

> data
{ image:
  { ImageWidth: 2560,
...

That way we still have to check if data.image exists, but then we can dive in to reading properties. We might get undefined back if we don't check data.image.ImageWidth exists of course, but that's no big deal and can be checked first if required.

gomfunkel commented 11 years ago

You're full of good ideas! ;)

I changed the format according to your suggestions as it makes total sense to me. Could you have another try and provide feedback in this issue? I haven't pushed the new version to npm yet so you have to pull it directly from Github.

gomfunkel commented 11 years ago

By the way: data.image, data.makernote and so on are always available as they are initialized empty no matter what. You don't have to check for their availability.

tremby commented 11 years ago

Cool, thanks, I'll try it out.

data.gps too?

gomfunkel commented 11 years ago

Yes, all output data containers (image, thumbnail, exif, gps, interoperability and makernote) are initialized empty and should now work as you suggested.

I should really update the documentation...

tremby commented 11 years ago

I think there may be something wrong. Running it over a photo I have which has lots of exif data and GPS too, it outputs

{ image: 
   { ImageWidth: 2560,
     ImageHeight: 1920,
     Make: 'google',
     Model: 'Nexus S',
     Orientation: 1,
     Software: 'IMM76D',
     ModifyDate: '2013:04:18 13:06:37',
     YCbCrPositioning: 1,
     ExifOffset: 176,
     GPSInfo: 559 },
  thumbnail: 
   { ImageWidth: 320,
     ImageHeight: 240,
     Compression: 6,
     Orientation: 1,
     XResolution: 72,
     YResolution: 72,
     ResolutionUnit: 2,
     ThumbnailOffset: 917,
     ThumbnailLength: 23382 },
  exif: {},
  gps: {},
  interoperability: {},
  makernote: {} }
gomfunkel commented 11 years ago

Yeah, forgot to change some lines of code... I hope I did now, could you please try again? Thanks!

tremby commented 11 years ago

Much better -- I now have

{ image: 
   { ImageWidth: 2560,
     ImageHeight: 1920,
     Make: 'google',
     Model: 'Nexus S',
     Orientation: 1,
     Software: 'IMM76D',
     ModifyDate: '2013:04:18 13:06:37',
     YCbCrPositioning: 1,
     ExifOffset: 176,
     GPSInfo: 559 },
  thumbnail: 
   { ImageWidth: 320,
     ImageHeight: 240,
     Compression: 6,
     Orientation: 1,
     XResolution: 72,
     YResolution: 72,
     ResolutionUnit: 2,
     ThumbnailOffset: 917,
     ThumbnailLength: 23382 },
  exif: 
   { ExposureTime: 0.0006802721088435374,
     FNumber: 2.6,
     ExposureProgram: 3,
     ISO: 50,
     ExifVersion: <Buffer 30 32 32 30>,
     DateTimeOriginal: '2013:04:18 13:06:37',
     CreateDate: '2013:04:18 13:06:37',
     ShutterSpeedValue: 11,
     ApertureValue: 3,
     BrightnessValue: 10,
     ExposureCompensation: NaN,
     MaxApertureValue: 3,
     MeteringMode: 2,
     Flash: 0,
     FocalLength: 3.43,
     UserComment: <Buffer 00 00 00 49 49 43 53 41 00>,
     ColorSpace: 1,
     ExifImageWidth: 2560,
     ExifImageHeight: 1920,
     ExposureMode: 0,
     WhiteBalance: 0,
     SceneCaptureType: 0 },
  gps: 
   { GPSVersionID: [ 2, 2, 0, 0 ],
     GPSLatitudeRef: 'N',
     GPSLatitude: [ 11.202651, 0, 0 ],
     GPSLongitudeRef: 'E',
     GPSLongitude: [ 119.2693223, 0, 0 ],
     GPSAltitudeRef: 0,
     GPSAltitude: 21,
     GPSTimeStamp: [ 5, 6, 38 ],
     GPSProcessingMethod: <Buffer 41 53 43 49 49 00 00 00 47 50 53>,
     GPSDateStamp: '2013:04:18' },
  interoperability: {},
  makernote: {} }

I see that a few things are buffers. Is that expected?

gomfunkel commented 11 years ago

Yes, that is expected (at the moment). The format of the data is defined by each Exif entry itself. The buffer entries are of type "undefined" so it's not possible to automatically determine their type (string, long, etc.). As far as I know there is information available on how to "decode" these buffers properly but checking that is something still on my todo list.

Are you otherwise happy with the output? It looks good to me now and I hope that everything is covered.

tremby commented 11 years ago

I guess there are lots of things which could be done like automatically turning the dates into Date objects and normalizing latitude/longitude degrees/minutes/seconds and their reference directions.

But automatically turning dates into Date objects is probably a bad idea because of the terrible state of uncertainty which exif dates have (in particular that there's no official way to put the timezone in there).

GPS normalization might be good. If it were mine, I'd normalize to -90 to 90 degrees latitude north and -180 to 180 longitude east (as is pretty standard), and altitude above sea level (negative for below). At the same time merge the minutes and seconds into it if they're given. I'm doing that in my code with these values anyway and it's not difficult, but it's something that needs to be done every time unless your application for some reason wants to print them out literally as the data is stored.

But if you don't want to add that in, that's okay with me and it's probably good to go. Thanks.

gomfunkel commented 11 years ago

Good ideas and I'd love to add them over time!

Would you be so kind and open separate issues for your feature requests? This way it's more easy to tackle them one after another in a structured way. Additionally it's more easy for others to submit pull requests with these features, if someone wants to contribute. That would be totally awesome!

I'm closing this issue now as it seems to be cleared.