-
Notifications
You must be signed in to change notification settings - Fork 44
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
Use bilinear interpolation for rasters #114
Comments
👍 Also, while you are at it can you remove the |
@yhahn brought up the fact that this might have some negative impact on performance. Has anyone done the benchmarks to see how much impact bilinear vs nearest has? Clarifying question: is this RasterSymbolizer only put to use for overzoomed rasters? |
Yes, bilinear scaling is more expensive than nearest as we have an optimized path in Mapnik for nearest: https://github.com/mapnik/mapnik/blob/master/src/image_scaling.cpp#L148-L166. So it is worth benchmarking. However I would not be surprised if benchmarking would not pick up a measurable difference.
Best answered by @yhahn. Another question to keep in mind is "What is the impact when raster layers are composited serverside into a new vector tile with other layers". In this case we default to bilinear scaling when overzooming: https://github.com/mapnik/node-mapnik/blob/master/src/mapnik_vector_tile.cpp#L922. When compositing without overzooming (all z/x/y match) then the original image is preserved untouched. |
👍 for bilinear on aesthetics grounds. The perceived quality is considerably higher. People don’t get as mad at blur as they get at pixely-ness. |
Based on @yhahn's
Here's the results for the current default Here's the diff in case we decide to apply this (in
|
@dnomadb this smells like it's related what's going on in https://github.com/mapbox/unpacker/issues/752: pixelated overview tiles. |
I am going to test this out for performance + quality increase. Here are what I see are the steps:
I'll update this hitlist as I find all the parts I missed :D |
Some node version problems with tilelive-vector tests: https://travis-ci.org/mapbox/tilelive-vector/builds/109968314 |
|
Tested on api-maps@bilinear - no bueno. Trying round 2 by adding bilinear to |
Alright, so all the tests are broken and I'm confused about the distinction between layer.xml and map.xml but we've got some preliminary results Looking at the mapbox.satellite tilejson we see a So a standard 256 z19 tile should be the same, no overzoom. ✔️ while the 2x versions should differ. ✔️
The difference is due to the bilinear interpolation. So the question: is bilinear interpolation worth it from the visual perspective? |
In terms of benchmarking for speed, I'm not sure EDIT URL needed an access token. Still, bench makes it easy enough to run them so I started two stacks and ran bench: It's hard to say how much is due to difference in network/infrastructure/availability-zones, etc but they're pretty close. Next step we may want to force z19 @2x as a next step? Here's the invocations:
|
Per chat, ^^ the benchmarks above are benching the endpoint when it returns a 401 unauthorized response. Try rerunning the benchmarks with |
Forcing z19 2x pngs
So, discounting extraneous variables, it looks like bilinear adds about 5% to the response time. Not horrible. We could go deeper to try to really isolate the rendering times sans network but let's stick with that rough assumption for now. |
Y, I think it's an improvement and it certainly smooths out a lot of the artifacts of compression and overzooming. The 💰 question: Is the visual improvement big enough to justify moving forward? Pros:
Cons:
|
Currently we are using nearest neighbor (the mapnik default) interpolation for the raster symbolizer:
https://github.com/mapbox/tilelive-vector/blob/master/templates/layer.xml#L21
Changing it to
scaling="bilinear"
will give better looking output tiles, which is especially important if we move towards resolution rounding as detailed here: mapbox/mapnik-omnivore#133cc @yhahn @springmeyer @perrygeo
The text was updated successfully, but these errors were encountered: