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

Improve Nikon maker note parsing. #18

Open
wants to merge 1 commit into
base: go1
Choose a base branch
from

Conversation

gsadams
Copy link
Contributor

@gsadams gsadams commented Dec 24, 2013

Implement parsing Nikon type 1 maker notes, and don't fail on unfamiliar Nikon maker note formats.

The rename of ImageAdjustment to Nikon_ImageAdjustment is a breaking change for anyone who may have been using that field. But the field value really has to be interpreted in a Nikon-specific way, so it seems to make sense.

(There still seems to be a problem with the Canon maker note parser failing on some images in my library, by which I mean it causes the entire exif parser to return failure, even when it's just failing to recognize the particular format inside the maker note. I'll track that down and send a separate pull request.)

Implement parsing Nikon type 1 maker notes, and don't fail on unfamiliar
Nikon maker note formats.
@rwcarlsen
Copy link
Owner

Thanks! It would probably be a good idea to write a couple of tests for the
makernote parsing that check for the problem fixed by this PR and the other
problem you mention. I can write tests if you don't want to. Regardless,
I will be enjoying my Christmas vacation for the next several days and so
won't do anything till I'm back (including this merge).

On Tue, Dec 24, 2013 at 1:20 AM, Geoff Adams [email protected]:

Implement parsing Nikon type 1 maker notes, and don't fail on unfamiliar
Nikon maker note formats.

The rename of ImageAdjustment to Nikon_ImageAdjustment is a breaking
change for anyone who may have been using that field. But the field value
really has to be interpreted in a Nikon-specific way, so it seems to make
sense.

(There still seems to be a problem with the Canon maker note parser
failing on some images in my library, by which I mean it causes the entire
exif parser to return failure, even when it's just failing to recognize the
particular format inside the maker note. I'll track that down and send a

separate pull request.)

You can merge this Pull Request by running

git pull https://github.com/gsadams/goexif improve-mknote

Or view, comment on, or merge it at:

#18
Commit Summary

  • Improve Nikon maker note parsing.

File Changes

Patch Links:

@gsadams
Copy link
Contributor Author

gsadams commented Dec 27, 2013

I agree about the tests, and it bothered me to send this pull request without them. But I'm not sure the best way to write such tests.

Would you recommend constructing trivial test images that trigger the problems, or doing some programmatic construction of just enough data structures to test, or doing a more function-level unit test? Or some other method?

Enjoy your vacation!

@@ -37,7 +37,6 @@ const (
ImageAuthentication = "ImageAuthentication"
ActiveDLighting = "ActiveDLighting"
VignetteControl = "VignetteControl"
ImageAdjustment = "ImageAdjustment"
Copy link
Owner

Choose a reason for hiding this comment

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

Now that you've brought attention to this, it looks like the concept of "common" makernote fields don't really exist - only maker-specific fields. So perhaps we should scrap them all in favor of maker-prefixed constants. e.g. Nikon_VignetteControl.

@rwcarlsen
Copy link
Owner

Test images are what I've used for testing so far. What I have done is truncate the images after the app1 section leaving approximately tens of bytes. And then my tests use the truncated file. My truncated test files are in the exif/samples directory. You are certainly welcome to manually construct images for testing if you really want to, but I wouldn't if it were me. Let me know if you are up for making a few tests.

Also, I have been thinking of changing the tiff and exif packages' error-handling paradigm to do a best-effort - basically to have the decoding return as much decoded data as possible in addition to any errors rather than just nil and the error.

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.

None yet

2 participants