gdal icon indicating copy to clipboard operation
gdal copied to clipboard

JP2Grok: new Grok JPEG 2000 driver based on Grok JPEG 2000 Library

Open boxerab opened this issue 1 year ago • 8 comments

Let's try this again !

What does this PR do?

This PR adds a new driver for the JPEG 2000 format using Grok library.

Design Strategy

In a previous merged PR, common features supporting both OpenJPEG and Grok libraries were refactored into a template driver named JP2DatasetBase. This PR creates a new Grok driver inheriting from JP2DatasetBase.

Performance

Basic performance testing of gdal_translate with default settings on a large HiRISE image shows this driver outperforming the existing open source driver by 6 times.

What are related issues/pull requests?

https://github.com/OSGeo/gdal/pull/3449 https://github.com/OSGeo/gdal/pull/5117 https://github.com/OSGeo/gdal/pull/8133

Tasks

  • [ ] Support encode (currently disabled)
  • [ ] Address issues raised by @rouault in previous Grok PRs
  • [ ] Add test case(s)
  • [ ] Add documentation
  • [ ] Updated Python API documentation (swig/include/python/docs/)
  • [ ] Review
  • [ ] Adjust for comments
  • [ ] All CI builds and checks have passed

boxerab avatar Jun 25 '24 03:06 boxerab

  • tests to be fixed
  • some code formatting tunings: cf https://github.com/OSGeo/gdal/actions/runs/9655962852/job/26632692508?pr=10294 , and https://gdal.org/development/dev_practices.html#commit-hooks how to set-up pre-commit
  • the driver would need to adapt for https://gdal.org/development/rfc/rfc96_deferred_plugin_loading.html changes. See for inspiration: https://github.com/OSGeo/gdal/blob/master/frmts/openjpeg/CMakeLists.txt, https://github.com/OSGeo/gdal/blob/master/frmts/openjpeg/openjpegdrivercore.h, https://github.com/OSGeo/gdal/blob/master/frmts/openjpeg/openjpegdrivercore.cpp, https://github.com/OSGeo/gdal/commit/f254bed998c36fd57308718937d7d505f86a3e46
  • it would be appropriate to add Grok build and testing into .github/workflows/ubuntu_24.04/Dockerfile.ci and .github/workflows/ubuntu_24.04/test.sh

rouault avatar Jun 25 '24 18:06 rouault

@boxerab Just looking at https://github.com/GrokImageCompression/grok/commits/master/ , I'm puzzled to see that Grok git history has been completely reset, and that the released packages are only the latest one. This isn't super re-assuring for audit purposes, especially after the XZ story.

rouault avatar Jun 26 '24 15:06 rouault

@boxerab Just looking at https://github.com/GrokImageCompression/grok/commits/master/ , I'm puzzled to see that Grok git history has been completely reset, and that the released packages are only the latest one. This isn't super re-assuring for audit purposes, especially after the XZ story.

@rouault the situation is not really analogous - no majour tools such as openssh rely on Grok, and the XZ Utils backdoor was not found via audit or code review, but rather by sheer luck.

The library has entered its maintenance phase - feature complete, stable and performant. The only changes I anticipate are bug fixes, but we haven't had many bugs for the past couple of years. The history has been reset, issues tracker has been cleared and only the latest release will be hosted on github.

The code continues to pass all unit tests and the OSS-Fuzz fuzzers continue to run. Anyone is free to audit the code, and to report issues.

boxerab avatar Jun 27 '24 02:06 boxerab

Anyone is free to audit the code

good luck when you have zero history... Was there something in the history that wasn't appropriate and had to be removed? But in that case there are less drastic ways of removing a commit than just erasing the whole history, which is an important asset of open source projects. I'm not the only one to find that there's something fishy here: https://mastodon.social/@EvenRouault/112687807835976568 . I can't name a project that has reset their history like that. This is an important asset to understand the rationale of parts of the code and having an audit trail. My 2 cents if there's nothing to hide and you've kept a copy of your initial git repo before the big reset is that you re-push it, and re apply the later modifications on top of if

rouault avatar Jun 27 '24 10:06 rouault

Yes, a few people weren't happy about the changes. Others are not happy with the AGPL license. They are free to use other open source or commercial offerings. As many continue to benefit from the project, it continues on its current path.

boxerab avatar Jun 28 '24 11:06 boxerab

As many continue to benefit from the project, it continues on its current path.

That applies to GDAL as well. We need to consider if and how our paths will meet.

jratike80 avatar Jun 28 '24 12:06 jratike80

That applies to GDAL as well. We need to consider if and how our paths will meet.

Indeed. I will note that GDAL has drivers for completely opaque black box software such as ERDAS ECW/JP2 SDK, while every line of a Grok release can be scrutinized by anyone, although I doubt anyone outside of the project will ever do so.

boxerab avatar Jun 28 '24 13:06 boxerab

I will note that GDAL has drivers for completely opaque black box software such as ERDAS ECW/JP2 SDK

indeed, that's what we mentioned during yesterday's monthly GDAL maintainer meeting. Given that we won't ship Grok itself as part of deliverables of the GDAL project, this isn't a blocking point for us to consider including the source code of the GDAL driver part. This is the responsibility of the end user to decide whether it fits their criteria

rouault avatar Jun 28 '24 13:06 rouault

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular link to any issues which this pull request fixes

  • that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in 2 weeks.

github-actions[bot] avatar Jul 27 '24 02:07 github-actions[bot]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 6 weeks. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the GDAL project can do to help push this PR forward please let us know how we can assist.

github-actions[bot] avatar Aug 10 '24 02:08 github-actions[bot]