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: add new driver for JPEG 2000 format using Grok library #8133

Closed
wants to merge 1 commit into from

Conversation

boxerab
Copy link
Contributor

@boxerab boxerab commented Jul 21, 2023

What does this PR do?

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

Design Strategy

The existing JP2OpenJPEG driver was copied and refactored to isolate the OpenJPEG-specific bits
in a header file opj_context while keeping most of the generic logic intact. Then, a similar header file
grk_context was created with Grok-specific bits. So, this refactor could be used for both open source JPEG 2000
drivers.

Testing

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

Tasklist

  • 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

@coveralls
Copy link
Collaborator

coveralls commented Jul 21, 2023

Coverage Status

coverage: 67.069% (-0.001%) from 67.07% when pulling 6c25303 on boxerab:grok3 into 4dcb8ef on OSGeo:master.

Copy link
Member

@rouault rouault left a comment

Choose a reason for hiding this comment

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

Some CI testing of the driver would be desirable. It should be normally sufficient to have Grok installed in .github/workflows/ubuntu_20.04/Dockerfile.ci

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

My comments on the doc done in the previous iteration #5117 haven't been addressed

--------------

The following open option is available:

Copy link
Member

Choose a reason for hiding this comment

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

We have now dedicated syntax ( https://gdal.org/development/dev_documentation.html#define-and-reference-configuration-options ) to define open and creation options. But better would be to try to commonalize that with the openjpeg driver through .rst inclusion

cmake/helpers/CheckDependentLibraries.cmake Show resolved Hide resolved
autotest/gdrivers/jp2grok.py Outdated Show resolved Hide resolved
autotest/gdrivers/jp2grok.py Outdated Show resolved Hide resolved
autotest/gdrivers/jp2grok.py Outdated Show resolved Hide resolved
autotest/gdrivers/jp2grok.py Outdated Show resolved Hide resolved
import gdaltest

pytestmark = pytest.mark.require_driver('JP2Grok')

Copy link
Member

Choose a reason for hiding this comment

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

As I raised previously in https://github.com/OSGeo/gdal/pull/5117/files#r793891898 , there's a lot of similarity between that file and jp2openjpeg.py . I presume using pytest fixtures we could factor a lot and most differences are perhaps in checksums for lossy compression (I didn't check) . Any opinion @dbaston if that's worth it ? The potential downsides would be if the behavior of drivers would have to diverge in significant ways, but I don't anticipate that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rouault any ideas on how we get to a common test base for both drivers ? Also, can I run the python driver test locally ?

Copy link
Member

Choose a reason for hiding this comment

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

any ideas on how we get to a common test base for both drivers ?

presumably using pytest.fixture() in test_jp2openjpeg.py. The driver_name fixture in autotest/ogr/ogr_rfc35py (recent master) could serve as inspiration. Presumably something along (untested...):

@pytest.fixture(autouse=True, params=["JP2OpenJPEG", "JP2Grok"])
def driver(request):
    driver_name = request.param
    jp2_drv = gdal.GetDriverByName(driver_name)
    if jp2_drv is None:
        pytest.skip(f"{driver_name} not available")
    gdaltest.deregister_all_jpeg2000_drivers_but(driver_name)
    yield jp2_drv
    gdaltest.reregister_all_jpeg2000_drivers()

def test_1(driver):
     ... do something ....

def test_2(driver):
     ... do something ....

Also, can I run the python driver test locally ?

Sure, you need to have the swig and python-dev packages installed. Then:

  • cmake -DBUILD_PYTHON_BINDINGS=ON ..
  • make python_binding
  • source ../scripts/setenv.sh
  • pytest autotest/gdrivers/jp2openjpeg.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

frmts/grok/jp2dataset.h Outdated Show resolved Hide resolved
@boxerab boxerab force-pushed the grok3 branch 5 times, most recently from 4d54430 to c9562e5 Compare July 22, 2023 15:09
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 22, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 22, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 22, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 22, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 22, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 22, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 22, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 22, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 23, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 24, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
@rouault
Copy link
Member

rouault commented Jul 25, 2023

@boxerab I'm currently on vacations until August 7th, so just to warn you to be patient.
Ah, please use git rebase instead of git merge, otherwise reviewing is more complicated. And as I must currently approve all CI checks everytime you push, as you have never had a contribution accepted here, you may need to check to CI check status in the Actions tab of your github account

boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 25, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 26, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
@boxerab boxerab force-pushed the grok3 branch 2 times, most recently from bacb567 to aebafa8 Compare July 28, 2023 21:46
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 29, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 29, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
@boxerab boxerab force-pushed the grok3 branch 4 times, most recently from 46469ef to b715c9d Compare July 29, 2023 14:31
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 29, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
boxerab added a commit to boxerab/gdal_rep that referenced this pull request Jul 29, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
@rouault
Copy link
Member

rouault commented Aug 5, 2023

you also need to format your code according to our formatting rules. See https://gdal.org/development/dev_practices.html#commit-hooks how to install pre-commit. You 'll need to run "pre-commit run --all-files" to fix the formatting of files

and git rebase on top of latest master to get rid off unrelated CI failures and clean the commit history

boxerab added a commit to boxerab/gdal_rep that referenced this pull request Aug 6, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
@rouault
Copy link
Member

rouault commented Aug 8, 2023

should now be rebased on top of latest master now that #8142 has been merged

@boxerab boxerab force-pushed the grok3 branch 4 times, most recently from e0b45b8 to 6c25303 Compare August 17, 2023 03:07
elpaso pushed a commit to elpaso/gdal that referenced this pull request Sep 4, 2023
Once this PR is validated, it will be re-used in
OSGeo#8133
@jdesboeufs
Copy link

Is there some kind of table with availables drivers and their corresponding licences?
Grok is under AGPL 3.0.

@rouault
Copy link
Member

rouault commented Oct 4, 2023

Is there some kind of table with availables drivers and their corresponding licences?
Grok is under AGPL 3.0.

Drivers themselves are MIT. But underlying libraries have their own licence indeed. In the JPEG2000 world, only the JP2OpenJPEG driver relies on a permissive license (openjpeg is 2-clause BSD licensed). Others are either proprietary (MrSID, ECW, Kakadu, Luratech) or Grok AGPL

@jratike80
Copy link
Collaborator

The license of the Grok driver is MIT. The license of the Grok library is AGPL. The licenses that apply to GDAL source tree are listed in LICENSE.TXT (contents can be seen for example with gdalinfo --license) and they are not many. The libraries that are optionally used as dependencies have their own licenses. There is no easy way to check those licenses. For example the ECW SDK license is mentioned in the documentation https://gdal.org/drivers/raster/ecw.html but different SDK versions have different licenses. Good practice is to include at least the non-open licenses within the delivery. OSGeo4W gathers them into directory \OSGeo4W\etc\licenses.

I fear it is up to the one who builds GDAL to check what they add into the mixturea and under which licenses, and decide how to report the licenses for their users.

@rouault
Copy link
Member

rouault commented Jan 16, 2024

Is there some news regarding this PR ?

@boxerab
Copy link
Contributor Author

boxerab commented Jan 17, 2024

No time to work on this PR for foreseeable future. Closing.

@boxerab boxerab closed this Jan 17, 2024
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

5 participants