feat(jpeg): add thumbnail functionality to jpeg reader#5333
feat(jpeg): add thumbnail functionality to jpeg reader#5333antond-weta wants to merge 2 commits into
Conversation
|
|
Co-authored-by: Ken McGaugh <ken@mcgaugh.co.uk> Signed-off-by: Anton Dukhovnikov <antond@wetafx.co.nz>
337ff62 to
b3a4c2b
Compare
| void | ||
| JpgInput::scan_for_thumbnail(cspan<uint8_t> exif) | ||
| { | ||
| try { |
There was a problem hiding this comment.
This code performs a basic scan for the subimage tag in the exif, it doesn't check if the found bit pattern is not a part of any other exif data. We may want to move this functionality to exif.cpp, but that may affect any other readers relying on exif.
| return false; | ||
| } | ||
|
|
||
| image_input->close(); |
There was a problem hiding this comment.
Without this I was getting address sanitiser errors in the image_input's destructor. I couldn't understand the error, and don't really think that it is related to the change.
| } | ||
|
|
||
| bool | ||
| JpgInput::get_thumbnail(ImageBuf& thumb, int subimage) |
There was a problem hiding this comment.
The get_thumbnail() methods in the TGA input and ImageBuf contain lock guards. Does this method need to be thread safe?
lgritz
changed the title
| if (!result) { | ||
| errorfmt( | ||
| "failed to initialise an ImageInput object with the thumbnail data"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think that if get_thumbnail, it's sufficient to just return false. But if you really want to return an error message, what you should be returning is image_input->geterror().
| if (!result) { | ||
| errorfmt( | ||
| "failed to initialise an ImageInput object of type \"jpeg\" with the thumbnail data"); | ||
| image_input->close(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Here, too. Either just return false with no error, or do
errorfmt("{}", image_input->geterror());
| } catch (const std::exception& e) { | ||
| errorfmt("ERROR scanning for thumbnail: {}", e.what()); | ||
| } |
There was a problem hiding this comment.
Why the try/catch here? Is there anything called inside that will actually throw an exception?
| JpgInput::get_thumbnail(ImageBuf& thumb, int subimage) | ||
| { | ||
| if (!m_decomp_create) { | ||
| errorfmt("ImageInput hasn't been initialised properly"); |
There was a problem hiding this comment.
I truly hate to pick on this, but in this code base we try to uniformly use the American spelling conventions: "initialized"
Co-authored-by: Larry Gritz <lg@larrygritz.com> Signed-off-by: Anton Dukhovnikov <antond@wetafx.co.nz>
2 participants
Description
This adds the thumbnail functionality to the jpeg reader by implementing get_thumbnail().
Tests
I could not find any unit tests related to get_thumbnail(), so I implemented a test in
python-thumbnail-jpeg. The test is done in python because we don't expose the thumbnails in oiiotool. The test is pretty basic, it just checks the thumbnail dimensions for now. Happy to add more unit tests if needed.I am also going to submit another PR soon fixing the thumbnails in raw reader, that will add
python-thumbnail-raw, hence the test name.Checklist:
and if I used AI coding assistants, I have an
Assisted-by: TOOL / MODELline in the pull request description above.
behavior.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.