rwcarlsen / goexif

Decode embedded EXIF meta data from image files.
BSD 2-Clause "Simplified" License
627 stars 134 forks source link

Add support for reading maker notes #11

Closed gsadams closed 11 years ago

gsadams commented 11 years ago

I've started with Nikon and Canon. I'll add more (probably as I need them), or even better, other folks can contribute, too.

rwcarlsen commented 11 years ago

It looks pretty good on first glance. I'll give it a closer look within the next couple days and probably merge then. Good work!

gsadams commented 11 years ago

Great, thanks!

rwcarlsen commented 11 years ago

This looks generally good. I want to put the makernote stuff into its own package. I threw in a couple of commits on top of your maker-notes branch that do this. The canon offset stuff was handled incorrectly in your version - the offsets were not absolute, but they were w.r.t. the beginning of the entire tiff directory structure. I added a field to the tiff.Tag that records this for later reference to support the canon makernote (and possibly other) use cases. I submitted a PR to myself (https://github.com/rwcarlsen/goexif/pull/12) and would be happy if you could look it over and tell me what you think.

rwcarlsen commented 11 years ago

I've pushed up a slightly modified version of your registration (#12). I implemented the basic exif field parsing as Parsers also. I changed the mknote package to have two separate Parsers for canon and nikon. I like the idea of explicit registration. This allows the parsers to be used without them being compulsorily (is that a word?) registered with the exif package. Use like this:

import (
    ".../mknote"
    ".../exif"
)

func main() {
    exif.RegisterParsers(mknote.All...)
    ...
    ...
    x, err := exif.Decode(r)
}
gsadams commented 11 years ago

Yeah, that looks good. I was considering making the main Exif-parsing code a similar module, but wasn't sure if I should go that far. But this is cleaner. I like it.

I left one comment in-line, about a no-longer-needed comment.

I've tested it, and it works for me. I say merge it! :)

rwcarlsen commented 11 years ago

included in #12.

rwcarlsen commented 11 years ago

Thanks for the help :+1: