Support brotli compressed boxes in JPEG XL #2381

kmilos · 2022-10-18T14:15:01Z

This introduces a dependency on the brotli library.

For some reason the Conan builds cannot find it, but the lib is found otherwise on macOS, MinGW, and Cygwin builds.

codecov · 2022-10-18T14:20:22Z

Codecov Report

Merging #2381 (6eb0956) into main (9767e37) will decrease coverage by 0.32%.
The diff coverage is 5.76%.

❗ Current head 6eb0956 differs from pull request most recent head a58e52e. Consider uploading reports for the commit a58e52e to get more accurate results

@@            Coverage Diff             @@
##             main    #2381      +/-   ##
==========================================
- Coverage   64.37%   64.05%   -0.33%     
==========================================
  Files         119      119              
  Lines       21090    21133      +43     
  Branches    10399    10426      +27     
==========================================
- Hits        13577    13537      -40     
- Misses       5363     5441      +78     
- Partials     2150     2155       +5

Impacted Files	Coverage Δ
include/exiv2/bmffimage.hpp	`100.00% <ø> (ø)`
src/bmffimage.cpp	`67.98% <0.00%> (-7.77%)`	⬇️
src/version.cpp	`88.16% <100.00%> (+0.21%)`	⬆️
src/minoltamn_int.cpp	`60.98% <0.00%> (-8.08%)`	⬇️
src/makernote_int.cpp	`65.39% <0.00%> (-0.96%)`	⬇️
src/olympusmn_int.cpp	`40.79% <0.00%> (-0.73%)`	⬇️
src/nikonmn_int.cpp	`61.46% <0.00%> (-0.44%)`	⬇️
src/easyaccess.cpp	`93.10% <0.00%> (-0.40%)`	⬇️
src/tiffvisitor_int.cpp	`79.38% <0.00%> (-0.36%)`	⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

1div0

OK

postscript-dev · 2022-10-18T16:34:36Z

@kmilos
Is it worth updating README.md to reflect the new build option?

kmilos · 2022-10-19T09:44:48Z

@postscript-dev Good catch - will do at some point, and then you can comment on the changes.

The bigger stopper here is the Conan build problem, no ideas there...

Pull request has been modified.

1div0

Gr8.

src/bmffimage.cpp

Pull request has been modified.

kmilos · 2022-10-20T08:29:23Z

@piponazo Do you have an idea about Conan problems? Is it related to case matching of files and variables? I saw Conan generates FindBrotli.cmake (no idea about its contents), while the working, hand-crafted one is FindBROTLI.cmake...

piponazo

I took a quick look to the changes from my tablet (I'm on vacations at the moment) and they look good.

cmake/FindBrotli.cmake

cmake/findDependencies.cmake

piponazo · 2022-10-21T13:26:22Z

@piponazo Do you have an idea about Conan problems? Is it related to case matching of files and variables? I saw Conan generates FindBrotli.cmake (no idea about its contents), while the working, hand-crafted one is FindBROTLI.cmake...

When I look at the checks from my tablet, I do not see any issues. Could you provide me a link to look at these issues?

What you describe is the normal behavior of Conan. The new Conan recipe that you are consuming will generate a FindBrotli.CMake in the build folder. In our CMake code we modify the CMAKE_PREFIX_PATH to include the CMake folder and the build in order to find such FindXXX.cmake files.

kmilos · 2022-10-21T13:33:48Z

Thanks @piponazo that was exactly it - I had to change the CMAKE_MODULE_PATH list order so the Conan modules are searched for first (if they exist), and make sure Conan's and the handcrafted module use the same letter case for variables.

All seems good now.

Pull request has been modified.

kmilos · 2022-10-21T14:50:52Z

Ok, now CIFuzz and ~~CodeQL~~ are failing because brotli is on by default and now required: can someone update the docker images to include "libbrotli-dev" as well please, I have no idea how...? @piponazo @kevinbackhouse

Edit: Ok, CodeQL was easy enough to handle...

.github/workflows/codeql-analysis.yml

kevinbackhouse · 2022-10-21T15:48:04Z

@kmilos: For CIFuzz, I think need to update this file:

https://github.com/google/oss-fuzz/blob/master/projects/exiv2/Dockerfile

Maybe we can change that to use ci/install_dependencies.sh so that it's less fragile in future.

kmilos · 2022-10-21T15:54:06Z

@kevinbackhouse Please go ahead, I'm running out of time/steam on this one...

src/bmffimage.cpp

kevinbackhouse · 2022-10-21T20:45:20Z

@kmilos: google/oss-fuzz#8835

src/bmffimage.cpp

Call exiv2's `install_dependencies.sh` script rather than using a hard-coded list of packages. This should make the build less fragile when we add new dependencies to exiv2. (Example: Exiv2/exiv2#2381.)

Co-authored-by: Kevin Backhouse <[email protected]> Use safe add to prevent overflow Co-authored-by: Kevin Backhouse <[email protected]>

kmilos · 2022-10-24T11:06:15Z

@kevinbackhouse I guess we have a chicken vs egg situation: the CIFuzz image still uses the install script from main, and not from this branch...

kevinbackhouse · 2022-10-24T11:09:43Z

@kmilos: No problem. We can merge without CIFuzz passing. Let me just check the latest changes before I approve.

kmilos · 2022-10-24T11:13:45Z

Thanks, merging then.

Call exiv2's `install_dependencies.sh` script rather than using a hard-coded list of packages. This should make the build less fragile when we add new dependencies to exiv2. (Example: Exiv2/exiv2#2381.)

kmilos mentioned this pull request Oct 18, 2022

Make cjxl output Exif/xml/jumb boxes libjxl/libjxl#1834

Merged

kmilos requested review from 1div0, kevinbackhouse and postscript-dev October 18, 2022 14:21

kmilos force-pushed the jxl_brotli branch from 7766807 to 3989e95 Compare October 18, 2022 14:37

kmilos added enhancement feature / functionality enhancements imageHandler Anything related to specific ImageHandlers labels Oct 18, 2022

1div0 previously approved these changes Oct 18, 2022

View reviewed changes

kmilos mentioned this pull request Oct 19, 2022

Exif start can be at any byte in payload, not word aligned #2382

Merged

1div0 previously approved these changes Oct 19, 2022

View reviewed changes

kevinbackhouse reviewed Oct 19, 2022

View reviewed changes

src/bmffimage.cpp Outdated Show resolved Hide resolved

kmilos force-pushed the jxl_brotli branch from e8e02d8 to efae07c Compare October 20, 2022 07:36

kmilos requested a review from piponazo October 20, 2022 08:29

kmilos force-pushed the jxl_brotli branch 4 times, most recently from 3b69da0 to 6c707b0 Compare October 21, 2022 12:03

piponazo previously approved these changes Oct 21, 2022

View reviewed changes

cmake/FindBrotli.cmake Show resolved Hide resolved

cmake/findDependencies.cmake Outdated Show resolved Hide resolved

cmake/findDependencies.cmake Show resolved Hide resolved

cmake/findDependencies.cmake Outdated Show resolved Hide resolved

kmilos force-pushed the jxl_brotli branch from b5382d7 to d0ad39a Compare October 21, 2022 14:37

kevinbackhouse reviewed Oct 21, 2022

View reviewed changes

.github/workflows/codeql-analysis.yml Show resolved Hide resolved

kmilos added the CI Issues related with CI jobs label Oct 21, 2022

kevinbackhouse reviewed Oct 21, 2022

View reviewed changes

src/bmffimage.cpp Outdated Show resolved Hide resolved

kevinbackhouse mentioned this pull request Oct 21, 2022

Use the install_dependencies.sh script from the exiv2 repo google/oss-fuzz#8835

Merged

kevinbackhouse reviewed Oct 21, 2022

View reviewed changes

src/bmffimage.cpp Outdated Show resolved Hide resolved

kmilos and others added 8 commits October 24, 2022 12:32

Exif start can be at any byte in payload, not word aligned

ee69868

Support brotli compressed boxes in JPEG XL

9a6ee59

Update README for brotli dependency

1e395ff

Match Conan's cmake find module case for Brotli

eada606

Use local cmake module path the last

c334fa3

Make brotli dependency controllable

557a5ce

Install deps for and bump CodeQL action

23cdcc4

Use safe add to prevent overflow

a58e52e

Co-authored-by: Kevin Backhouse <[email protected]> Use safe add to prevent overflow Co-authored-by: Kevin Backhouse <[email protected]>

kmilos force-pushed the jxl_brotli branch from 89143ac to a58e52e Compare October 24, 2022 10:33

kevinbackhouse approved these changes Oct 24, 2022

View reviewed changes

kmilos merged commit 1ae3a83 into main Oct 24, 2022

kmilos deleted the jxl_brotli branch October 24, 2022 11:14

This was referenced Nov 21, 2022

Is a new release happening soon? #2406

Closed

Memory leak in BmffImage::brotliUncompress #2427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support brotli compressed boxes in JPEG XL #2381

Support brotli compressed boxes in JPEG XL #2381

kmilos commented Oct 18, 2022 •

edited

Loading

codecov bot commented Oct 18, 2022 •

edited

Loading

1div0 left a comment

postscript-dev commented Oct 18, 2022

kmilos commented Oct 19, 2022

1div0 left a comment

kmilos commented Oct 20, 2022

piponazo left a comment

piponazo commented Oct 21, 2022

kmilos commented Oct 21, 2022 •

edited

Loading

kmilos commented Oct 21, 2022 •

edited

Loading

kevinbackhouse commented Oct 21, 2022

kmilos commented Oct 21, 2022

kevinbackhouse commented Oct 21, 2022

kmilos commented Oct 24, 2022

kevinbackhouse commented Oct 24, 2022

kmilos commented Oct 24, 2022

Support brotli compressed boxes in JPEG XL #2381

Support brotli compressed boxes in JPEG XL #2381

Conversation

kmilos commented Oct 18, 2022 • edited Loading

codecov bot commented Oct 18, 2022 • edited Loading

Codecov Report

1div0 left a comment

Choose a reason for hiding this comment

postscript-dev commented Oct 18, 2022

kmilos commented Oct 19, 2022

1div0 left a comment

Choose a reason for hiding this comment

kmilos commented Oct 20, 2022

piponazo left a comment

Choose a reason for hiding this comment

piponazo commented Oct 21, 2022

kmilos commented Oct 21, 2022 • edited Loading

kmilos commented Oct 21, 2022 • edited Loading

kevinbackhouse commented Oct 21, 2022

kmilos commented Oct 21, 2022

kevinbackhouse commented Oct 21, 2022

kmilos commented Oct 24, 2022

kevinbackhouse commented Oct 24, 2022

kmilos commented Oct 24, 2022

kmilos commented Oct 18, 2022 •

edited

Loading

codecov bot commented Oct 18, 2022 •

edited

Loading

kmilos commented Oct 21, 2022 •

edited

Loading

kmilos commented Oct 21, 2022 •

edited

Loading