fix for issue 647

[vincentsarago]

This PR should closes #647

The main problem is that we are creating a WarpedVRT aligned with the desired bounds (to make sure we fetch the overview) but also with the output size, which result in the resampling_method having no effect because there is not downscaling/upscaling happening at read step.

To ✅ the solution we will need to add tests proving the resampling effects but also still make sure that we are fetching the overviews!!!

[vincentsarago]

🥳 we already had tests to make sure we were fetching the right overview https://github.com/cogeotiff/rio-tiler/blob/main/tests/test_overview.py

I've added some and they are ✅ so we should be good to go 🚢

[vincentsarago]

I've updated the assets to avoid resampling to have a role (because depending on GDAL versions the pixel values can slightly change)

[vincentsarago]

We now construct a VRT aligned with the reading bounds but which has the size defined by its projected resolution.

[vincentsarago]

TODO

• add tests to show the different resampling options have influence

[DanSchoppe]

Awesome! Glad you got to the bottom of this @vincentsarago... not sure whether anyone else on earth would have had a chance 😅

[DanSchoppe]

@vincentsarago I pulled the newly released 6.2.4 into TiTiler and am seeing some wicked tile edge / border effects:

Before (rio-tiler 4.1.10, TiTiler 0.11.6)

After (rio-tiler 6.2.4, TiTiler 0.15.1)

Of course my test was not just testing changes from 6.2.3 -> 6.2.4, because of course 6.2.3 didn't have working resampling. The rio-tiler and TiTiler versions from the two test cases are listed.

Do you think this change is responsible?

[vincentsarago]

😭

[vincentsarago]

@DanSchoppe can you share how you created this?

[vincentsarago]

😬 I see them too 😓

This is happening when we are overzooming (at zoom 17 it looks fine)

When you add padding=1 (not available by default in titiler, but in rio-tiler) it disappear

rio-tiler/rio_tiler/reader.py

Line 309 in 63f1a3f

padding (int, optional): Padding to apply to each bbox edge. Helps reduce resampling artefacts along edges. Defaults to `0`.

[vincentsarago]

I kinda think that's a better behaviour in general. what do you think?

Feel free to add padding in titiler 😅

we could also switch to nearest when doing upsampling https://github.com/DHI-GRAS/terracotta/blob/main/terracotta/raster.py#L361-L363

[DanSchoppe]

When you add padding=1 (not available by default in titiler, but in rio-tiler) it disappear

I kinda think that's a better behaviour in general. what do you think?

This would fetch an extra pixel on each edge (eg. 258x258 resolution) to eliminate edge effects, correct? Are there any resampling algorithms that operate on a kernel larger than this padding (for example, would benefit from 260x260)?

Sure seems like default padding would be a great improvement, especially if 1px optimizes every resampling algorithm. Even when you're fetching just one tile, the padding would give you higher quality data near the edges.

[vincentsarago]

This would fetch an extra pixel on each edge (eg. 258x258 resolution) to eliminate edge effects, correct?

yes

Are there any resampling algorithms that operate on a kernel larger than this padding (for example, would benefit from 260x260)?

I don't know

Sure seems like default padding would be a great improvement, especially if 1px optimizes every resampling algorithm.

It will only improve for non-nearest.

Even when you're fetching just one tile, the padding would give you higher quality data near the edges.

Changing the default from 0 to something else is a huge breaking change IMO.

This is something that could be set by default at titiler level 🤷

[DanSchoppe]

It will only improve for non-nearest.

Even nearest might find that the padded pixel is actually nearer, right? So it'd benefit nearest as well.

Changing the default from 0 to something else is a huge breaking change IMO.

Oh, I was thinking the suggested change was to fetch with padding, perform any required operations (eg. resampling), then return a non-padded tile. If the suggestion was to return 258x258 tiles by default, that seems like a non-starter 😅

This is something that could be set by default at titiler level 🤷

That's fine for my personal needs, but leaving this tile edge artifact in rio-tiler seems pretty bad, doesn't it?

[vincentsarago]

If the suggestion was to return 258x258 tiles by default, that seems like a non-starter

no, no. You suggested to set padding to something different than 0. with the padding option the output tile will always be 256 (the pixels are removed automatically)

I agree it might not be a big thing, but in some case having padding=1 or 2 might result in additional blocks to be fetched (and thus slow down the process) which is mostly why I think keeping 0 as default is a safer choice.

That's fine for my personal needs, but leaving this tile edge artifact in rio-tiler seems pretty bad, doesn't it?

the edges will be visible only in some case (mostly oversampling and for non-nearest resampling) so I feel rio-tiler is 👌 not setting default for the users.

[vincentsarago]

We can start a new issue and see if other people have opinions! 🙏

[vincentsarago]

On the bright side, rio-tiler is now two time faster 😱

[DanSchoppe]

We can start a new issue and see if other people have opinions! 🙏

Sounds good!

On the bright side, rio-tiler is now two time faster 😱

🚀 Plus resampling is working again! 💯

Thanks @vincentsarago !

[vincentsarago]

@DanSchoppe here is a small analysis of the impact of the resampling and padding https://gist.github.com/vincentsarago/ff73ea5dbf128949d951851cc8454b4f

the TLDR, padding is useful mostly when overzooming. When in the dataset's zoom range there is not highly visible edges so padding do not have huge impact.