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

Add LERC compression support #206

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Conversation

zakjan
Copy link
Contributor

@zakjan zakjan commented Mar 16, 2021

Until GDAL 3.3 is released, test files currently must be generated with GDAL compiled with internal libtiff. I used osgeo/gdal in Docker.

Fixes #205

@zakjan
Copy link
Contributor Author

zakjan commented Mar 16, 2021

While I'm at it, I also added LERC_DEFLATE compression support. Additional compression is defined in a pseudo field inside of LercParameters field. Tests are passing, but maybe there is a better way to read it?

https://github.com/OSGeo/gdal/blob/master/gdal/frmts/gtiff/libtiff/tif_lerc.c#L1046-L1048

@zakjan
Copy link
Contributor Author

zakjan commented Mar 16, 2021

I'm not sure why, but all LERC files render narrowed in the dev page 🤔

Screen Shot 2021-03-16 at 8 28 31

@constantinius
Copy link
Member

@zakjan

Nice work, thanks a lot!

I think the culprit is this function:

  decodeBlock(buffer) {
    if (this.addCompression === LercAddCompression.Deflate) {
      buffer = inflate(new Uint8Array(buffer)).buffer;
    }

    return Lerc.decode(buffer).pixels[0].buffer;
  }

Specifically the pixels[0]. I'm no expert in how LERC works, but my guess is that it already splits up the pixel-interleaved structure in band separated ones (which happens later in the image reading for other compression methods). If you force band interleaved layout in the TIFF (-co INTERLEAVE=BAND) this should not occur.

I'm not sure how to fix this though. Maybe there is an option in the lerc library?

@zakjan
Copy link
Contributor Author

zakjan commented Mar 16, 2021

Good catch, -co INTERLEAVE=BAND indeed fixes the rendering. However, Lerc.decode(buffer).pixels.length is always 1.

@constantinius
Copy link
Member

Hmm, that is odd. Hard to say what is going wrong here. Can you maybe check the sizes of the arrays? Width, height, and so on?

@zakjan
Copy link
Contributor Author

zakjan commented Mar 16, 2021

width: 539
height: 1
pixels.length: 1
pixels[0].length: 8085

There is numDims=15 stored inside LERC header, it matches 8085 / 15 = 539, but it is not used to split the result. It seems that LERC JS is not properly updated to LERC 2.4 to handle this configuration :(

However, geotiff.js compression decoder probably currently can't return an array of bands anyway, or can it? I'm not sure how do other compressions handle interleaving?

@zakjan zakjan marked this pull request as draft March 16, 2021 09:25
@zakjan zakjan force-pushed the lerc branch 3 times, most recently from e8f53a0 to 0279530 Compare March 16, 2021 09:39
@constantinius
Copy link
Member

There is numDims=15 stored inside LERC header, it matches 8085 / 15 = 539, but it is not used to split the result. It seems that LERC JS is not properly updated to LERC 2.4 to handle this configuration :(

So the support is halfway there? 🤔 It reads the right size but then only the first pixel? Maybe we can report an issue on the LERC project.

However, geotiff.js compression decoder probably currently can't return an array of bands anyway, or can it? I'm not sure how do other compressions handle interleaving?

Currently interleaving was not handled by the decoding/decompressing part as they usually treated it as a raw byte stream and this had to be handled outside anyways (apart maybe from JPEG, but that is another story).

@zakjan
Copy link
Contributor Author

zakjan commented Mar 16, 2021

Yes, it seems that the fix needed in LERC JS is to add support for numDims param passed from GeoTIFF SamplesPerPixel, similarly to GDAL implementation:
https://github.com/OSGeo/gdal/blob/master/gdal/frmts/gtiff/libtiff/tif_lerc.c#L445
https://github.com/OSGeo/gdal/blob/master/gdal/frmts/gtiff/libtiff/tif_lerc.c#L504
This should be straightforward to contribute.

Then we'll need geotiff.js to support returning an array of bands from the decoder, right? Could you point me to the parts of code where this could belong?

@zakjan
Copy link
Contributor Author

zakjan commented Mar 16, 2021

It reads the right size but then only the first pixel?

Currently it reads all bands and returns them as one in this case.

@zakjan
Copy link
Contributor Author

zakjan commented Apr 1, 2021

I got closer by forcing to update bytesPerPixel for LERC. It fixes the rendering of the test file. I'm not sure if it is a good solution though?

if (this.planarConfiguration === 2 || this.fileDirectory.Compression === 34887) {
bytesPerPixel = this.getSampleByteSize(sampleIndex);
}

Screen Shot 2021-04-02 at 1 00 05

@zakjan
Copy link
Contributor Author

zakjan commented Apr 1, 2021

Hi @rouault @avalentino, would you be able to advise on this? It would be great if geotiff.js can support reading all possible LERC configurations produced by the upcoming GDAL 3.3. Any suggestions are appreciated.

@zakjan
Copy link
Contributor Author

zakjan commented Apr 2, 2021

I got closer by forcing to update bytesPerPixel for LERC. It fixes the rendering of the test file.

This is incorrect as expected. It helps only with the first band. Other bands render the first band instead, columns shifted to the left.

Screen Shot 2021-04-02 at 9 46 08

@rouault
Copy link
Contributor

rouault commented Apr 2, 2021

would you be able to advise on this?

I'm not sure to have fully understood the issue, but my understanding is that the LERC JS library you use should have an extra argument to specify it the number of bands contained in the buffer.

@zakjan
Copy link
Contributor Author

zakjan commented Apr 2, 2021

Yes, it seems so, but the extra argument is missing. Is it required in GDAL to pass the number of bands to lerc_decode, for the decoder to correctly decode both interleaving configs, even when reading a single band? Do you know if the number passed from GeoTIFF only overrides the number decoded from LERC header, or is there more to that?

@rouault
Copy link
Contributor

rouault commented Apr 2, 2021

Is it required in GDAL to pass the number of bands to lerc_decode, for the decoder to correctly decode both interleaving configs, even when reading a single band?

There's no optional argument in C, so yes :-)

Do you know if the number passed from GeoTIFF only overrides the number decoded from LERC header, or is there more to that?

As far as I can see, the ndim argument passed to lerc_decode() is used internally in LercLib for consistency check against what is stored in the Lerc header ( https://github.com/OSGeo/gdal/blob/978dbc5d280bbc5f5cea5ab319e4157edd2d7beb/gdal/third_party/LercLib/Lerc.cpp#L375 ) . So technically it could be avoided.

@zakjan
Copy link
Contributor Author

zakjan commented Apr 2, 2021

@rouault Interesting, this is a great insight, thanks! It means that reading it from the header in LERC JS should be enough.

Does GDAL LERC integration do any significant post-processing to support decoding pixel interleaved files? This is the result when I decode with LERC JS currently:

Screen Shot 2021-04-02 at 17 39 01

@rouault
Copy link
Contributor

rouault commented Apr 2, 2021

Does GDAL LERC integration do any significant post-processing to support decoding pixel interleaved files?

it does none. This is a regular TIFF compression codec. All the LERC specific logic is in tif_lerc.c

@zakjan
Copy link
Contributor Author

zakjan commented Apr 3, 2021

@constantinius I have a solution. It was a combination of three factors:

Finishing this PR currently depends only on merging and releasing the PR in LERC JS.

All files, all bands seem to work now.

Screen Shot 2021-04-03 at 11 43 17
Screen Shot 2021-04-03 at 11 43 35

@zakjan zakjan marked this pull request as ready for review April 3, 2021 09:52
@constantinius
Copy link
Member

Very well done, thank you @zakjan!

@constantinius constantinius merged commit 0e4f32a into geotiffjs:master Apr 6, 2021
@zakjan
Copy link
Contributor Author

zakjan commented Apr 6, 2021

@constantinius we still need that LERC JS PR to be merged and released, or inline the decoder. Otherwise, this PR doesn’t work properly

@zakjan zakjan deleted the lerc branch April 8, 2021 09:46
@zakjan
Copy link
Contributor Author

zakjan commented Apr 8, 2021

@constantinius I re-tested the current state, and interestingly, with explicit transforming from band-interleaved to pixel-interleaved if expected by geotiff.js, this PR works even with the old LERC version released on npm. So it seems that geotiff.js can be safely released.

There is more discussion about this topic in Esri/lerc#146 though. LERC JS might be updated to return the original format soon.

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.

Support for LERC compression
3 participants