MapStore2 icon indicating copy to clipboard operation
MapStore2 copied to clipboard

#9588: Add support to multi-band color mapping for COG layers

Open dsuren1 opened this issue 2 years ago • 11 comments

Description

This PR allows multi band color mapping of COG layers along modification of min and max source data value

Please check if the PR fulfills these requirements

  • [x] The commit message follows our guidelines: https://github.com/geosolutions-it/MapStore2/blob/master/CONTRIBUTING.md
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x", remove the others)

  • [x] Enhancement

Issue

What is the current behavior?

  • #9588

What is the new behavior? image

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • [ ] Yes, and I documented them in migration notes
  • [x] No

Other useful information

dsuren1 avatar Jan 09 '24 13:01 dsuren1

Looks like GitHub experiencing outage causing actions to fails image

dsuren1 avatar Jan 09 '24 13:01 dsuren1

Hi, from a brief discussion about it with @dromagnoli I understood that Samples count should be the samples count + sope possible extra samples

Ideally I'd transfer to the layer the information about the tiff as plain as possible to the layer metadata, so if there is some error in interpretation, we can fix the code and we don't have to fix the layer.

Theoretically even if data can suggest you the style to use, you may decide to show 1 gray scale, RGB or RGBA independently from the origial bands list. But let's decide with tobia if this should be added now or later.

Notice that photometric interpretation can be WhiteIsZero or BlackIsZero that are grays too.

So I suggest to :

  1. Isolate the metadata extraction function utility so we can allow also refresh of this data in the layer properties in the future.
  2. Save the important metadata as they are, not saving aggregated properties like isRGB that was wrongly calculated. In particular you may need to save:
    • samples. I don't know if you need to check if they are 1 byte long. The count should provide the number of "bands" (extra-samples excluded I think).
    • the fileDirectory including PhotometricInterpretation value
    • probably the extra-samples (look at this PR in geotiff.js to see if it is included or not in samples)
  3. From these information, you can desume the "isRGB" information and provide the proper styler.

From my understanding if samples are 1, 3 (or 4, adding the extra one) you may desume the number of "bands" so you can correctly initialize the band selector. If sample is only 1, it should be gray. (I asked confirm about this to @dromagnoli ).

offtherailz avatar Jan 17 '24 10:01 offtherailz

@offtherailz

  • [ ] Sometimes it switches to one band randomly

This is because, on Search COG layer from the catalog when the cache is expired, the metadata is not fetched as the operation is restricted to adding the COG layer as a service. This could also result in a projection warning in the catalog as the metadata information holding projection information is missing.

In the future enhancement, the fetch metadata operation can also be performed from layer settings ex: style editor, which should help us handle this case. For now, can I just add a warning message in Style Editor saying The metadata information is missing and hence default to single band editor, this banding could be incorrect. Please add the layer from the catalog by fetching metadata, something like this?

Apart from this, the rest is ready for review. Thanks!

dsuren1 avatar Jan 18 '24 08:01 dsuren1

confirmed the format with @allyoucanmap , let me talk about min and max with tobia.

offtherailz avatar Jan 22 '24 13:01 offtherailz

@offtherailz

  • [ ] In particular I noticed that these values are anyway applied, even if I deactivate the layer.

From the video attached, I see the band styling is only disabled, meaning the bands selected from the channels will be ignored but not min and max, as these are independent. Min/max can be cleared if the user chooses to apply only source data values. Additionally, there are cases where only either is needed, for this reason, I have provided an option to disable band styling separately.

  • [ ] saving whole fileDirectory looks to be potentially an issue. It contains a lot of data that may potentially make the JSON explode (see TileOffsets , TileByteCounts, GeoKeyDirectory). Please fix and save only the data you effectively need, not relying on them.

My thoughts exactly and I wanted to discuss when PR is reviewed. Glad you noticed. I will add only the PhotometricIntepretation prop for now. The other props can be added later based on the need in the future enhancements

dsuren1 avatar Jan 23 '24 13:01 dsuren1

@offtherailz some test maps you can use: https://dev-mapstore.geosolutionsgroup.com/mapstore/#/viewer/46403 https://dev-mapstore.geosolutionsgroup.com/mapstore/#/context/COG/47132

tdipisa avatar Mar 11 '24 15:03 tdipisa

@dsuren1 can you please fix conflicts?

tdipisa avatar Mar 11 '24 15:03 tdipisa

Resolved

dsuren1 avatar Mar 12 '24 08:03 dsuren1

I performed the tests on this map:

http://localhost:8081/#/context/COG/46403

Here my findings:

Bugs

  • The stile toggle is enabled, but if I turn off and on, the style do not work.

screencast-mail.google.com-2024.03.14-18_01_02.webm

Things to discuss

It is not easy to review the PR because of this regression not related to the PR (verified also on master):

If I add layers to the map, now I don't have the information for zoom etc.. anymore. Bolivia_LC08_L1TP_001069_20190719_MSf layer do not seems to work on the map, and I have some errors of incompatibility too, when adding layers to the map. @tdipisa we should scheduele a fix for this.

offtherailz avatar Mar 14 '24 17:03 offtherailz

@offtherailz

It is not easy to review the PR because of this regression not related to the PR (verified also on master):

If I add layers to the map, now I don't have the information for zoom etc.. anymore. Bolivia_LC08_L1TP_001069_20190719_MSf layer do not seems to work on the map, and I have some errors of incompatibility too, when adding layers to the map. @tdipisa we should scheduele a fix for this.

I think it is better if you open a dedicated issue to report the regression you have identified with proper steps to reproduce it, mentioning also if it is affecting only master or also the release.

tdipisa avatar Mar 14 '24 17:03 tdipisa

Here the issue @tdipisa https://github.com/geosolutions-it/MapStore2/issues/10072

offtherailz avatar Mar 14 '24 17:03 offtherailz

@offtherailz @tdipisa

Bugs

  • The stile toggle is enabled, but if I turn off and on, the style do not work.

screencast-mail.google.com-2024.03.14-18_01_02.webm

The problem is caused by missing metadata as explained here

dsuren1 avatar Mar 19 '24 07:03 dsuren1

@offtherailz

  • [ ] "enableBandStyling" property should not be used, but the style should depend only on the style object. I think the presence of the body.color section is enough to determine if it is enabled or not, but if you need more, add a flag and proper document it.

You are right. This prop is not needed. Might have skipped the coffee :wink:

  • [ ] For certain cogs is still unclear to me how to style. For instance this https://cogeo.craig.fr/opendata/ortho/2018_25cm_ain+isere.tif the styler do not look to be working at all. Maybe because of photometryic interpretation : 6 ?

At times, the alpha channel may prevent the visualization of certain TIFF sources, and the composition of some TIFF sources may disregard the alpha channel. To mitigate this issue and to eliminate no-data tiles surrounding the image, I have incorporated the nodata attribute into the source (TODO: should made configurable in the future enhancement), which introduces an alpha channel and facilitates the better visualization of the TIFF image.

2018_25cm_ain+isere.tif image

a final note: Having to save catalogs again and again is counter-intuitive at the end. If we need more metadata, we need to think about a way to update/auto-update layers source metadata.

At least after this change, all the required metadatas are fetched and persisted. But yes, maybe a force fetch metadata option somewhere in the layer settings is needed in the future.

dsuren1 avatar Mar 21 '24 07:03 dsuren1

@ElenaGallo, could you please test this on DEV ? Thank you

offtherailz avatar Mar 21 '24 13:03 offtherailz

Test passed, @dsuren1 please backport to 2024.01.xx. Thanks

ElenaGallo avatar Mar 21 '24 15:03 ElenaGallo