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

MRF: JPEG-XL (brunsli) support #3945

Merged
merged 9 commits into from
Jun 22, 2021
Merged

MRF: JPEG-XL (brunsli) support #3945

merged 9 commits into from
Jun 22, 2021

Conversation

lucianpls
Copy link
Contributor

@lucianpls lucianpls commented Jun 8, 2021

What does this PR do?

brunsli (part of JPEG-XL) is an open source licensed lossless packer for JFIF files from Google.
The brunsli blobs are usually 22% smaller than the source JFIF JPEG on the average, while increasing the compression and decompression time. The source JFIF form is fully restored, thus brunsli requires only minor changes to the existing code and does not affect the image quality or any other JPEG features. It can only handle the most common 8bit JFIF format, but it does pass-through all JFIF chunk, brotli compressed.
This PR includes the changes to make the MRF driver capable of encoding and decoding tiles in the brunsli format, when the brunsli support is enabled. Since brunsli/brotli libraries are not usually available as packages, support for this feature has to be enabled manually at this point.

I intend to add a standalone JPEG-XL (brunsli for starters) driver shortly, with read/write capabilities.
I would appreciate any help or advice on adding brunsli/brotli libraries to the CI workflow so these changes can be fully tested.

What are related issues/pull requests?

Tasklist

  • ADD YOUR TASKS HERE
  • Add test case(s)
  • Add documentation
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

Environment

Provide environment details, if relevant:

  • OS:
  • Compiler:

By default, JPEGs are written as JPEG-XL, except if noJXL option is set
The compiler complains if a variable is not used
@rouault
Copy link
Member

rouault commented Jun 8, 2021

CC'ing @tbonfort

@lucianpls I wasn't aware of https://github.com/google/brunsli, but in https://github.com/rouault/gdal/commits/jxl I have preliminary work to use JPEG-XL as a TIFF codec, based on the https://gitlab.com/wg1/jpeg-xl implementation. I'm not completely clear if a google/brunsli blob is a valid JPEG-XL one ? It would be best to converge on a single JPEG-XL implementation for GDAL.

@lucianpls
Copy link
Contributor Author

@rouault

Brunsli is a subset of JPEG-XL. JPEG-XL lite if you want.
From what I've read, brunsli output is a fully compliant JPEG-XL file, so at least there should not be format issues.
The brunsli has the advantage that it can be seen as a JPEG add-on, while the other features of JPEG XL are much more far reaching. It is very simple to change existing code that uses JPEG to accept brunsli, it is a pre-processing filter. That is not the case if the full JPEG XL is expected.

There is also the issue of dependencies. libbrunsli only depends on brotli, and even that dependency is non-essential and could be removed, although I'm not sure that it can be considered JPEG-XL in that case. The full JPEG XL code is much more complex AFAICT.

In general, I prefer the simplicity and clarity of brunsli over the full JPEG-XL. It does one thing and it does it well.

@tbonfort
Copy link
Member

tbonfort commented Jun 8, 2021

@lucianpls from a user's point of view, supporting brunsli requires bumping the gdal version and adding some new dependencies, whereas adding full jxl support requires bumping the gdal version and adding a different set of dependencies. Not very different in my book. The full jxl suite has some very attractive support for n>3 bands along with more than 8bit and lossless which rivals j2k, and should see mainstream support in distros, it would seem to be a pity to only support brunsli, even though it would mean more effort to implement in the mrf driver.

@lucianpls
Copy link
Contributor Author

@tbonfort
Sure, both full JXL and the brunsli subset make sense.
Brunsli is an incremental JPEG optimization, easy to implement and very attractive by itself, especially for tile services. As you say, full JXL is a different compression, with it's own set of options.

@rouault
Copy link
Member

rouault commented Jun 8, 2021

@lucianpls What I would wish to avoid is to have both Brunsli and libjxl as dependencies of GDAL. Would it be possible to leverage libjxl to deal with the Brunsli payload for MRF ?

@lucianpls
Copy link
Contributor Author

@rouault
I'm sure JXL includes brunsli as a dependency. So there should be no problem. In the worst case, these bits of code would only need a bit of tweaking to use the brunsli that comes with JXL instead of the stand alone, without any change in the user observable behavior. I need this capability now, no reason to wait as far as I can tell! I already have an apache httpd on-the-fly brunsli transcoder for tile serving.

gdal/frmts/mrf/marfa.h Outdated Show resolved Hide resolved
gdal/frmts/mrf/marfa.h Outdated Show resolved Hide resolved
gdal/frmts/mrf/JPEG_band.cpp Outdated Show resolved Hide resolved
gdal/nmake.opt Show resolved Hide resolved
gdal/GDALmake.opt.in Show resolved Hide resolved
@rouault
Copy link
Member

rouault commented Jun 8, 2021

I'm sure JXL includes brunsli as a dependency.

yes, seeing https://github.com/libjxl/libjxl/tree/main/third_party . But as far as I remember, the install of the JXL library doesn't make brunsli headers available. They're likely considered as an implementation detail

@rouault
Copy link
Member

rouault commented Jun 8, 2021

doc/source/drivers/raster/marfa.rst would probably need some updates too

@lucianpls
Copy link
Contributor Author

doc/source/drivers/raster/marfa.rst would probably need some updates too

The MRF detailed documentation is in the MUG, the rst is just a pointer to that document. I will update the MUG to discuss the brunsli implications.

@lucianpls lucianpls closed this Jun 14, 2021
@lucianpls lucianpls reopened this Jun 14, 2021
Assume brunsli include files are preinstalled and that libraries are in the library path
@lucianpls lucianpls merged commit e344246 into OSGeo:master Jun 22, 2021
@lucianpls lucianpls deleted the brunsli branch July 11, 2021 20:00
BRUNSLI_ENABLED="yes"
AC_SUBST(BRUNSLI_ENABLED, $BRUNSLI_ENABLED)
AC_SUBST(BRUNSLI_INCLUDE, -I$prefix/include)
AC_SUBST(BRUNSLI_LIB, "-lbrunslicommon -lbrunslienc -lbrunslidec -lbrotlicommon -lbrotlienc -lbrotlidec")
Copy link
Contributor

@SpaceIm SpaceIm Jan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BRUNSLI_LIB is super fragile:

  • lib ordering is wrong: brotlicommon is a dependency of brotlienc and brotlidec, same for brunsli.

  • Lib names of brunsli seem wrong: https://github.com/google/brunsli/blob/v0.1/brunsli.cmake#L50-L85. Installation of brunsli is not neat, but CMakeLists suggests it's always shared and lib names are brunslidec-c and brunslienc-c.

  • brotli lib names are different if static (-static suffix)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll probably not put too much effort in autoconf support for Brunsli, but I've filed #5068 to add support for it to CMake builds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SpaceIm

* Lib names of brunsli seem wrong: https://github.com/google/brunsli/blob/v0.1/brunsli.cmake#L50-L85. Installation of brunsli is not neat, but CMakeLists suggests it's always shared and lib names are `brunslidec-c` and `brunslienc-c`.

Those are for the C API, which is much more limited than the C++ one if I recall.

Copy link
Contributor

@SpaceIm SpaceIm Jan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, but it's worth noting that the very few package managers having this lib don't even package the static libs (C++), only the shared C lib. I don't know whether C++ headers are intended to be used by consumers (there is no install target nor API documentation so it's unclear, but at least the folder of C/C++ files is called c).
Anyway, static lib names have -static suffix as you can see in CMakeLists.

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

Successfully merging this pull request may close these issues.

None yet

4 participants