return specific error types for sub-IFDs

[mpl]

Context: https://code.google.com/p/camlistore/issues/detail?id=493

We have some Camlistore users reporting that they have pictures on which we choke. It looks like goexif is rightly having a problem processing a badly formed tiff tag type in at least the first case, and that's fine.

However, I think they do have a point when they compare the result with some other tools that do report the error ("Warning: Bad InteropIFD directory" from exiftool), but still give whatever useful info was extracted from the (broken) file.

So I'd like to propose a mechanism to return some specific error types, so that we know when it's still possible to use whatever was decoded by goexif in some cases. Something like:

ex, err := exif.Decode(f)
if err != nil {
    if !exif.IsGPSInfoError(err) && !exif.IsInteroperabilityError(err) {
        log.Fatalf("exif.Decode: %v", err)
    }       
}
// else try and use ex anyway to get e.g. DateTime info.

I've just toyed with this idea and this commit is maybe a bit gross. If you're ok with the concept then I'll try and think of a better implementation.

[rwcarlsen]

This is actually something that has been on my (far too long) todo list for a while - a best effort decode when data is corrupt. I'd be happy for such changes to be worked on by someone other than me ;-)

[mpl]

here's another approach, what do you think?

(I'll squash all the commits together eventually).

[rwcarlsen]

It looks fine. The tiff package probably could use some modifications to do a best-effort decode. Right now, as soon as it runs into any error, it returns nil up the stack regardless of what had been (and could have been) decoded correctly. If an IFD offset is bad, then the decoding has to abort, but could still return prev IFD's. Also, because tags within an IFD are fixed size, if a single tag is corrupt, decoding could continue for the rest of the IFD.

[mpl]

Ok, I've squashed the commits and added some doc.

Just to be sure though: you're saying that if an IFD offset is bad the decoding should abort. So does that mean that I should analyse an error returned by e.g. loadSubDir(x, ExifIFDPointer, exifFields) to make sure we don't have an offset error before letting loadSubDir(x, GPSInfoIFDPointer, gpsFields) run?

I understand your best-effort suggestions but I propose to try and address them in subsequent commits (as they're lower priority for me). Can't make any promise but I think I'll be able to find some time and give it a go if you haven't got around to it. I'll also open an issue about it the Camlistore issue tracker, as I think it could motivate some others to have a go at it. Sounds good?

[mpl]

Just to be sure, is there anything you need from me about that change?

[rwcarlsen]

I'm busy with research now. I added you as a committer - be good.

On Tue, Oct 7, 2014 at 10:27 AM, mpl notifications@github.com wrote:

Just to be sure, is there anything you need from me about that change?

— Reply to this email directly or view it on GitHub https://github.com/rwcarlsen/goexif/pull/28#issuecomment-58203007.

[mpl]

ack, thanks. I'll ask the other camli contributors to have a look at that one before committing anyway.

On 7 October 2014 17:42, Robert Carlsen notifications@github.com wrote:

I'm busy with research now. I added you as a committer - be good.

On Tue, Oct 7, 2014 at 10:27 AM, mpl notifications@github.com wrote:

Just to be sure, is there anything you need from me about that change?

— Reply to this email directly or view it on GitHub https://github.com/rwcarlsen/goexif/pull/28#issuecomment-58203007.

— Reply to this email directly or view it on GitHub https://github.com/rwcarlsen/goexif/pull/28#issuecomment-58205435.

[fawick]

LGTM. Maybe the name of the erroneous field should be featured more prominently in the error string?

Done in loadSubDir

[saljam]

But that's just nitpicking. LGTM.

[mpl]

I'll squash and merge that PR tomorrow unless somebody objects.