Canon cr3 previews

[dhoulder]

Extract previews from Canon CR3 files.
Fixes #1893

[lgtm-com]

This pull request introduces 1 alert when merging 3717a90 into 4901c5d - view on LGTM.com

new alerts:

• 1 for Wrong type of arguments to formatting function

[clanmills]

This looks good, @dhoulder. It builds and passes the test suite.

Several comments:

1. I'd like somebody else to review and approve this as it will have to be approved on 0.27-maintenance after v0.27.5 ships next week. I want out of the loop.

2. Is using import hashlib part of the "standard" python3 installation?

3. I added something to the test suite for dealing with temporary files. See test_issue_1484.py You might find that useful (or your might not). I can't find the carcass of your temporary extracted files. The test suite on 'main' has a different user experience from 0.27-maintenance, so I'm not sure what it's doing. I ran you test manually as follows:

$ 677 rmills@rmillsm1:~/gnu/github/exiv2/main/tests $ python3 runner.py --verbose bugfixes/github/test_issue_1893.py 
test_run (test_issue_1893.issue_1893_cr3_preview) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.033s

OK
678 rmills@rmillsm1:~/gnu/github/exiv2/main/tests $

4. I'm unclear if you've addressed the issue we discussed about needlessly allocating and reading the mdat box.

[dhoulder]

Thanks @clanmills

Is using import hashlib part of the "standard" python3 installation?

Yes. Even exists in python 2. https://docs.python.org/3/library/hashlib.html says "Constructors for hash algorithms that are always present in this module are sha1(), sha224(), sha256(), sha384(), sha512(), blake2b(), and blake2s()."

I'm unclear if you've addressed the issue we discussed about needlessly allocating and reading the mdat box.

No not yet. I wanted to fix and test one thing at a time.

I can't find the carcass of your temporary extracted files.

Feature, not a bug 😄 . I didn't want to litter test/data with output files. They're generated in a temporary directory that python guarantees to remove when self.preview_image_tmp_dir goes out of scope.

I've discovered that there's a wrong format descriptor in Internal::stringFormat("width,height,size = %u,%u,%ld", ...). Should be "width,height,size = %u,%u,%u". I'm not sure if I can "fix" a pull request or if I have to submit another one - will investigate.

[clanmills]

Well done, @dhoulder. You've discovered that you can update the PR. You have to set the tracked branch. I use SmartGit to do that because I don't understand git.

[codecov]

Codecov Report

Merging #1958 (0714194) into main (2b040e9) will increase coverage by 0.15%.
The diff coverage is 80.76%.

@@            Coverage Diff             @@
##             main    #1958      +/-   ##
==========================================
+ Coverage   61.12%   61.28%   +0.15%     
==========================================
  Files          96       96              
  Lines       19068    19079      +11     
  Branches     9745     9748       +3     
==========================================
+ Hits        11656    11692      +36     
+ Misses       5093     5069      -24     
+ Partials     2319     2318       -1     

Impacted Files	Coverage Δ
include/exiv2/bmffimage.hpp	100.00% <ø> (ø)
src/bmffimage.cpp	77.77% <80.76%> (+6.34%)	⬆️
src/types.cpp	87.98% <0.00%> (-0.46%)	⬇️
src/makernote_int.cpp	67.62% <0.00%> (ø)
include/exiv2/image.hpp	100.00% <0.00%> (ø)
src/crwimage_int.cpp	74.07% <0.00%> (+0.03%)	⬆️
src/crwimage_int.hpp	84.61% <0.00%> (+0.24%)	⬆️
src/tags_int.cpp	77.05% <0.00%> (+0.24%)	⬆️
src/pngimage.cpp	60.10% <0.00%> (+0.26%)	⬆️
src/nikonmn_int.cpp	46.31% <0.00%> (+0.35%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b040e9...0714194. Read the comment docs.

[kmilos]

Minor cosmetic changes while we're at it.

Looks good otherwise!

[kevinbackhouse]

@clanmills: It seems like the macOS -fstack-clash-protection issue is back. Do you think I should revert #1857?

[dhoulder]

@clanmills wrote:

I added something to the test suite for dealing with temporary files. See test_issue_1484.py You might find that useful (or your might not)

Ah, yes that's probably a bit more succinct than my approach. Next time…

[clanmills]

@dhoulder Do nothing. What you have works.

@kevinbackhouse Groan. I'm surprised. My MBP has the M1 chip and build David's code without issue.

There was another PR which used check_linker_flag(-fcf-protection HAS_FCF_PROTECTION_LINK). We should test that both check_compiler_flag() AND check_linker_flag() are successful.

When I attempted to use that for v0.27.5 RC3, it didn't deliver happiness although I didn't know why not. What a miserable thing. I'm on vacation at the moment. I expect we'll revisit this next week before v0.27.5 gets safely out the door.

[kevinbackhouse]

Ignore the Mac build failures. I have created #1962 to try to fix that.

[dhoulder]

I see #1962 is merged. Do I need to do something to rerun the checks?

[clanmills]

@dhoulder. You're probably good. When I want to force a rebuild, I submit a cosmetic change (add a space to a comment in the code), update the PR and label the change as "build trigger". There are probably smarter ways that I don't know.

If it fails on macOS, you can apply the code change from #1962 to this PR and see what happens.

I'll build your code on my machines, although I don't expect any issues. My MacBookPro has the M1 processor and the latest Xcode (13.0). The MacMini is Intel, however the latest Xcode can't be installed (can't install on your 7 year old machine, buy a new MacMini, sucker), so it's stuck at 12.1.

I don't know what the CI uses to build, however I expect it to be apparent in the build transcript.

[kevinbackhouse]

Is this ready to merge?

[clanmills]

@dhoulder The builds on the MacBookPro/M1/Xcode13 and MacMini/Intelx64/Xcode12.1 are both good.

@kevinbackhouse I think David has to do a rebase merge or something to update the code base of this PR. When he submits that change, the CI will run and pass. Then we can approve and merge this branch. How does David update the code base for this PR?

When I build v0.27.5 (probably Thursday 2021-10-21), I'll update cmake/compilerFlags.cmake from:

            # macOS M1 will set ARCHITECTURE == arm64
            EXECUTE_PROCESS( COMMAND uname -m COMMAND tr -d '\n' OUTPUT_VARIABLE ARCHITECTURE )
            if ( NOT ${ARCHITECTURE} STREQUAL arm64 )
                check_cxx_compiler_flag(-fstack-clash-protection HAS_FSTACK_CLASH_PROTECTION)
            endif()

to:

            if ( NOT APPLE )
                check_cxx_compiler_flag(-fstack-clash-protection HAS_FSTACK_CLASH_PROTECTION)
            endif()

[kevinbackhouse]

I can just do a "squash and merge" if you would like this to be squashed into a single commit.

[kevinbackhouse]

I can use mergify to backport #1962 to 0.27-maintenance. Should this PR be backported too?

[clanmills]

Thank, @kevinbackhouse I'm happy to merge cmake/ changes into 0.27-maintenance for v0.27.5 GM. I don't want C++ code changes in 0.27-maintenance until after we have shipped.

[kevinbackhouse]

@clanmills: ok, thanks. I created #1966 to backport the cmake changes.

@dhoulder: Do you think this should be back-ported to the 0.27-maintenance branch? If so, please could you add a comment like this one to this PR? It won't get merged until after v0.27.5 has shipped though.

[dhoulder]

@Mergifyio backport 0.27-maintenance

[mergify]

backport 0.27-maintenance

🟠 Waiting for conditions

• merged [:pushpin: backport requirement]

[dhoulder]

Hi @kevinbackhouse , @clanmills : Thanks for your help.

@dhoulder: Do you think this should be back-ported to the 0.27-maintenance branch? If so, please could you add a comment like this one to this PR?

Done. I had "backport to 0.27" on my to-do list, but I'll scrub that now.

I can just do a "squash and merge" if you would like this to be squashed into a single commit.

Yep, fine by me. Thanks.

For the record I manged to trigger the CI rerun with git commit -m "retrigger checks" --allow-empty; git push.

[mergify]

backport 0.27-maintenance

✅ Backports have been created

• #1968 Canon cr3 previews (backport #1958) has been created for branch 0.27-maintenance

[clanmills]

@dhoulder Thank You very much for working on this David. Very valuable contribution. I've updated #1893 to ask @paolobenve to build and test with his CR3 files.

@kevinbackhouse Thank You very much for taking care of the integration, Kev.