Replace underlying rwcarlsen/goexif with dsoprea/go-exif

[danedavid]

Fixes #8586

• Replace underlying EXIF library (rwcarlsen/goexif) with an actively developed one (dsoprea/go-exif)
• Add tests for disable options (date & location) while decoding EXIF.

[danedavid]

Couple of questions:

1. In the body of Decode method, there are many calls to goexif code that return an error object as a second value. Being new to Go, I was wondering if we need to add the following after each assignment to err? Is there a better, idiomatic way?

if err != nil {
  return
}

2. As the values of the Tags map are of type interface{}, there is no guarantee that a particular tag value has the same type as when we were using the previous library, except the tags (DateTime, LatLong) that are handled specifically. Do we need to add any type specific tests there?

3. The API of exif module is unchanged, but would you recommend adding more tests? If so, what parts need to be tested? Have added one new test to check for disable LatLong & Date options at the module level. I believe it's already being tested in the image module.

4. Should the old library be removed from dependency list in README?

5. The unexported exifWalker struct & its Walk method may not be needed as such because the new library doesn't require a walker function. However, have maintained the same signatures for brevity. Let me know if those need to be changed.

[bep]

1. No, but I prefer

if err != nil {
  return err
}

A common pattern to make the above a little more compact is to do:

if val, err := foo(); err != nil {
  return err
}

As in, I almost never used named return values (or whatever they're called).

2. That is a good question. The one I would be curious about, which I don't see a particulary good test for, is that the old library returned Rat values for some floats (e.g. f-stop)
3. The test coverage looks fine to me.
4. I'll update the dependency list in README.
5. You're welcome to simplify/remove the Walker struct if its not needed.

[danedavid]

@bep Made a few changes. Fixed the build issue when file has no Exif data. Also, added type assertions & type-based test cases for tag values.

[danedavid]

@bep This logic existed in the previous code, making the value of rational exif tags either float64 or big.Rat type depending on the numerator. Have copied the logic as such and added corresponding tests; however, let me know if returning just one of the types (float64 or big.Rat) all the time makes more sense.

[bep]

Hey,

It still fails for me in the case of an image with no Exif.

What I suggest is that you make:

Decode(r io.Reader) (ex *Exif, err error)

Into

Decode(r io.Reader) (*Exif, error)

And remove the panic recovery at the top.

And then you need to explicitly handle all errors, including NewIfdMappingWithStandard. I'm not sure where to set the line between "no exif data" and a real error that the user should/could do something about.

The above also suggests that we miss out on a test case for a JPG without Exif metadata ...

[danedavid]

Hi @bep
I'd run hugo on the project you'd mentioned and verified, but apparently I seem to have made a mistake while building. Apologies for that.

I've added a new test case by stripping sunset.jpg of its EXIF data (using https://www.verexif.com/en/). I had to add a PNG check & extra dependancy to satisfy the new test case AND this one:

hugo/resources/images/exif/exif_test.go

Line 65 in fcd63de

func TestExifPNG(t *testing.T) {

Hope it doesn't look clumsy.

[danedavid]

Hi @bep, were you able to look into this?

[bep]

@danedavid looking at it now ...

[bep]

This works great now; I'm going to rebase and merge this in #8761 -- thanks!

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.