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

DEMData: save one CPU copy and do decode on GPU #8694

Merged
merged 1 commit into from
Aug 28, 2019
Merged

Conversation

astojilj
Copy link
Contributor

@astojilj astojilj commented Aug 27, 2019

Save one CPU copy and avoid decoding DEM data on CPU (do it on GPU).

Pad DEMData, for backfill, in context.getImageData. This avoids the need to pad it in DEMData constructor.

DEM decode to height is a single dot operation - no need to do it on CPU since we already have another decode (from internal format) in src/shaders/hillshade_prepare.fragment.glsl.

Time spent, averaged in milliseconds, in getImageData and new DEMData is measured using this patch.

When opening debug/hillshade.html, page loads 7 256x256 tiles, with total 368ms (~52ms per tile) spent in new DEMData. This is reduced to 1 ms per tile with the patch.

                    master branch       |          with this patch
----------------------------------------|-------------------------------           
           new DEMData  | getImageData  |  new DEMData  | getImageData     
------------------------|---------------|---------------|---------------           
iPhoneX       45 ms     |     12 ms     |       1 ms    |     12 ms 

cc @mapbox/gl-native

@astojilj astojilj self-assigned this Aug 27, 2019
@astojilj astojilj added the performance ⚡ Speed, stability, CPU usage, memory usage, or power usage label Aug 27, 2019
@andrewharvey
Copy link
Collaborator

Oh hey it looks like this partially fixes #6027 🎉 This is awesome. If you look closely you can still see some artefacts described in #6027 with this PR, but they don't appear for a few extra zooms further than currently in master, and without more testing I can't say if that's due to a limitation in the data or in the codebase... I can help try to answer that.

Two screen shots below, exact same data and style:

master

Screenshot from 2019-08-27 23-53-07

this PR

Screenshot from 2019-08-27 23-52-41

@andrewharvey
Copy link
Collaborator

This performance improvement is awesome.

Though I wanted to point some tangential analysis I did a while back at mapbox/rio-rgbify#3 (comment). I know this is a bit off topic and not something that can be addressed on the client side, but might be worth investigating.

For a large map that takes 25 256x256 terrain tiles for the map, that's 3.3MB and took about 4.3 seconds just to download those tiles, this would be worse over a bad mobile connection.

Asking for terrain-rgb webp tiles, Mapbox still only serves pngs anyway.

https://a.tiles.mapbox.com/v4/mapbox.terrain-rgb/9/85/197@2x.webp
> identify 197@2x.webp
PNG 512x512 512x512+0+0 8-bit sRGB

I tried a few compression options to see what affect that would have:

> for f in tiles/*; do b=`basename $f`; pngcrush -brute $f pngcrush/$b; done
> for f in tiles/*; do b=`basename $f`; cwebp -lossless -q 100 -m 6 $f -o cwebp/$b; done
method total size
original 3.3M
pngcrush 2.4M ~30% improvement
cwebp 1.8M ~50% improvement

Keep in mind both of these compressions are lossless, so there's big gains to be made by spending more compute on the API side to reduce the size sent over the wire.

@astojilj
Copy link
Contributor Author

@andrewharvey

Thanks for checking #6027 and letting me know.

src/render/draw_hillshade.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

This is great! I just have that one suggestion about type checking but other than that this looks ready. Thanks

Save one CPU copy and avoid decoding DEM data on CPU (do it on GPU).

Pad DEMData, for backfill, in context.getImageData. This avoids the need to pad it in DEMData constructor.

DEM decode to height is a single dot operation - no need to do it on CPU since we already have another decode (from internal format) in src/shaders/hillshade_prepare.fragment.glsl.
@astojilj astojilj merged commit 11c5c82 into master Aug 28, 2019
@astojilj astojilj deleted the astojilj-hillshade branch August 28, 2019 06:36
astojilj added a commit that referenced this pull request Aug 30, 2019
astojilj added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 30, 2019
This is first part of work on porting mapbox/mapbox-gl-js#8694 - in follow up patch(es) it is required to remove CPU side copy using 2d canvas support on all supported platforms, similar to approach taken in gl.js  https://github.com/mapbox/mapbox-gl-js/pull/8694/files#diff-34dbe5f7de34dc4b9a8745dcde9bdc37R48

Decoding on CPU removed.
Padding is still done in DEMData() but, instead od doing it wwhile decoding, it is using memcpy to pad original values.

Rebase to latest mapbox-gl-js master and re-generate shaders.

Partly fixes: #15503
astojilj added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 30, 2019
This is first part of work on porting mapbox/mapbox-gl-js#8694 - in follow up patch(es) it is required to remove CPU side copy using 2d canvas support on all supported platforms, similar to approach taken in gl.js  https://github.com/mapbox/mapbox-gl-js/pull/8694/files#diff-34dbe5f7de34dc4b9a8745dcde9bdc37R48

Decoding on CPU removed.
Padding is still done in DEMData() but, instead od doing it wwhile decoding, it is using memcpy to pad original values.

Rebase to latest mapbox-gl-js master and re-generate shaders.

Partly fixes: #15503
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants