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

isPrintableText() seems to return false if string contains \n or \r #55

Closed
lastzero opened this issue Feb 12, 2021 · 15 comments
Closed

Comments

@lastzero
Copy link

This effectively converts a proper description like "Hello World\n\nWhere are you?" to "string with binary data (%d bytes)" which then shows up as image description throughout the application:

if isPrintableText(t) == false {
	phrase = fmt.Sprintf("string with binary data (%d bytes)", len(t))
	return phrase, nil
}

It seems better to allow \n, \r, and \t or return an empty string so that the value returned by Exiftool can be used as fallback. Same for Emojis if they don't qualify as printable text.

lastzero added a commit to photoprism/photoprism that referenced this issue Feb 13, 2021
lastzero added a commit to photoprism/photoprism that referenced this issue Feb 15, 2021
@dsoprea
Copy link
Owner

dsoprea commented Apr 28, 2021

That's disturbing. Fixed and added tests.

Emojis are inherently printable since they are always comprised of printable characters.

@dsoprea
Copy link
Owner

dsoprea commented Apr 28, 2021

Bumped PNG, JPEG, and TIFF integrations.

@lastzero
Copy link
Author

Amazing! Thank you, I'll test it :)

lastzero added a commit to photoprism/photoprism that referenced this issue Apr 30, 2021
@lastzero
Copy link
Author

@dsoprea
Copy link
Owner

dsoprea commented Apr 30, 2021 via email

@dsoprea dsoprea reopened this May 3, 2021
@dsoprea
Copy link
Owner

dsoprea commented May 8, 2021

I don't think that's related to this change. go-exif drops it regardless because it's defined as a NUL-terminated string and no NUL is present.

$ go run command/exif-read-tool/main.go -f /tmp/gopro_hd2.jpg -v
...
2021/05/08 02:39:50 exifcommon.parser: [WARNING]  ascii not terminated with nul as expected: [HD2p�3��A�]
2021/05/08 02:39:50 exif.utility: [WARNING]  Could not parse value for tag [IFD] (0110) [Model].
...

@lastzero
Copy link
Author

lastzero commented May 8, 2021

Would it be possible to make the parsing (a bit) less strict or does this result in other issues?

An earlier version of go-exif returned it and exiftool does as well. So not a huge issue as most of our users have exiftool enabled as a fallback.

@dsoprea
Copy link
Owner

dsoprea commented May 8, 2021 via email

@dsoprea
Copy link
Owner

dsoprea commented May 8, 2021 via email

@lastzero
Copy link
Author

lastzero commented May 8, 2021

Note that this is somewhat irrelevant to the current matter, though. None of this has changed, so why is your test suddenly breaking? Has it just been running through exiftool every time?

I've been upgrading from an earlier version of your library as the last version had the issue with isPrintableText(), in addition to the changed camera model parsing behaviour.

If you say this should not be touched, that's good enough for me... might have been something you want to look into.

Thank you very much!

@dsoprea
Copy link
Owner

dsoprea commented May 12, 2021

We were already ignoring the lack of a NUL byte. We were specifically looking for nonprintable binary. So, instead of failing, we now just stop-short and return the bytes before that point.

@dsoprea
Copy link
Owner

dsoprea commented May 12, 2021

Bumped all image-formats and go-exif-knife to use latest go-exif.

@lastzero
Copy link
Author

Can't upgrade somehow - do you know what's going on here? Wrong Go version?

$ go get -u github.com/dsoprea/go-exif/v3
# github.com/dsoprea/go-logging
/go/pkg/mod/github.com/dsoprea/go-logging@v0.0.0-20200710184922-b02d349568dd/log.go:355:14: cannot use "github.com/go-errors/errors".Wrap(err, 2) (type error) as type *"github.com/go-errors/errors".Error in assignment: need type assertion
/go/pkg/mod/github.com/dsoprea/go-logging@v0.0.0-20200710184922-b02d349568dd/log.go:445:21: cannot use "github.com/go-errors/errors".Wrap(err, 1) (type error) as type *"github.com/go-errors/errors".Error in return argument: need type assertion
/go/pkg/mod/github.com/dsoprea/go-logging@v0.0.0-20200710184922-b02d349568dd/log.go:451:20: cannot use "github.com/go-errors/errors".Wrap(err, 1) (type error) as type *"github.com/go-errors/errors".Error in return argument: need type assertion

@dsoprea
Copy link
Owner

dsoprea commented May 14, 2021

Because you're upgrading every component instead of just go-exif, and go-logging is currently broken because the stacktrace-management package that it (and the dozens of projects I've produced over the years) uses just introduced a breaking PR a couple of days ago (go-errors/errors#34).

Just do go get github.com/dsoprea/go-exif/v3@8213cfabc61b27b42b71133f18372eed1122a237.

lastzero added a commit to photoprism/photoprism that referenced this issue May 14, 2021
@lastzero
Copy link
Author

Excellent, thank you! Seems to work 🥳

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

No branches or pull requests

2 participants