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

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

Closed
wants to merge 1 commit into from
Closed

Conversation

boxerab
Copy link
Contributor

@boxerab boxerab commented Jun 25, 2024

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?

#3449
#5117
#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

@@ -0,0 +1,552 @@

.. _raster.JP2Grok:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. _raster.JP2Grok:
.. _raster.jp2grok:



This driver is an implementation of a JPEG2000 reader/writer based on
Grok library **v2**.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Grok library **v2**.
Grok library.

@rouault
Copy link
Member

rouault commented Jun 25, 2024

@boxerab boxerab changed the title JPGrok: new Grok JPEG 2000 driver based on Grok JPEG 2000 Library JP2Grok: new Grok JPEG 2000 driver based on Grok JPEG 2000 Library Jun 25, 2024
@rouault
Copy link
Member

rouault commented Jun 26, 2024

@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.

@boxerab
Copy link
Contributor Author

boxerab commented Jun 27, 2024

@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.

@rouault
Copy link
Member

rouault commented Jun 27, 2024

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

@boxerab
Copy link
Contributor Author

boxerab commented Jun 28, 2024

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.

@jratike80
Copy link
Collaborator

jratike80 commented Jun 28, 2024

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.

@boxerab
Copy link
Contributor Author

boxerab commented Jun 28, 2024

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.

@rouault
Copy link
Member

rouault commented Jun 28, 2024

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

Copy link

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 github-actions bot added the stale label Jul 27, 2024
Copy link

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 github-actions bot closed this Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants