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

pgraster input plugin #2288

Closed
wants to merge 54 commits into
base: 2.3.x
from

Conversation

Projects
None yet
3 participants
@strk
Contributor

strk commented Jun 30, 2014

A new input plugin for postgis raster, see #1660
In its current state the plugin only supports 8BPP rasters of either 1 (grayscale) or 3 (RGB) bands.
No automated tests are included yet, but manual test has been performed with default imports
of datasets from http://www.naturalearthdata.com

No attempt is made at supporting "nodata" values.

Sandro Santilli added some commits Jun 26, 2014

Sandro Santilli
Fix colorspace interpretation in raster reading
The colorspace is really RGBA, I was confused by ImageData.set
accepting a 4-bytes integer and thus affected by machine endianness
(on little-endian it becomes ABGR)
Sandro Santilli
Add support for "prescale_rasters" option
The option calls ST_Resize on the input rasters so they are scaled
before getting transferred to the renderer. This reduces the amount
of data transferred but increases the time it takes to get a result
(useful if data transfers are more expensive than db CPU).
Sandro Santilli
Add support for raster overviews
Adds "use_overviews" parameter, which defaults to false.
Will error out if using a subquery.
@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 2, 2014

Contributor

Latest commit adds support for raster overviews.

Contributor

strk commented Jul 2, 2014

Latest commit adds support for raster overviews.

Sandro Santilli
Add support for "band" parameter
The parameter has the same semantic than the GDAL one,
except 0 is used for "all" instead of -1.
This parameter is needed in order to RasterColorizer to work.
@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 4, 2014

Contributor

Latest commit adds support for "classified" rasters, using the same approach of the GDAL plugin: accepting a "band" parameter that when specified forces color interpretation to float data.
See https://groups.google.com/forum/#!topic/mapnik/Zq8c8SKB4fE

Contributor

strk commented Jul 4, 2014

Latest commit adds support for "classified" rasters, using the same approach of the GDAL plugin: accepting a "band" parameter that when specified forces color interpretation to float data.
See https://groups.google.com/forum/#!topic/mapnik/Zq8c8SKB4fE

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 8, 2014

Contributor

With the commits of today I've started testing small extent fetches.

For some reason if the plugin provides a mapnik::raster with an extent larger than the extent requested by the map configuration, a lot of RAM is used at rendering time.

Here's the exact data:

Mapnik LOG> 2014-07-08 18:57:09: agg_renderer: -- query_extent=box2d(1634712, 4809821, 1634712, 4809831)
Mapnik LOG> 2014-07-08 18:57:09: pgraster_featureset: raster of 256x241 pixels covering extent box2d(1634712, 4809821, 1637272, 4812231)

The output image size is the nik2img default (600x400).
Ideas about what can be triggering all the ram use ?
I verified that pre-clipping the raster so to get closer in extent to the requested extent fixes the excessive RAM usage.

Contributor

strk commented Jul 8, 2014

With the commits of today I've started testing small extent fetches.

For some reason if the plugin provides a mapnik::raster with an extent larger than the extent requested by the map configuration, a lot of RAM is used at rendering time.

Here's the exact data:

Mapnik LOG> 2014-07-08 18:57:09: agg_renderer: -- query_extent=box2d(1634712, 4809821, 1634712, 4809831)
Mapnik LOG> 2014-07-08 18:57:09: pgraster_featureset: raster of 256x241 pixels covering extent box2d(1634712, 4809821, 1637272, 4812231)

The output image size is the nik2img default (600x400).
Ideas about what can be triggering all the ram use ?
I verified that pre-clipping the raster so to get closer in extent to the requested extent fixes the excessive RAM usage.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 8, 2014

Contributor

One thing that may be related: agg_renderer logs a Scale=0 when the large ram is used. In normal conditions (fetching the whole dataset) I get to read Scale=236.425. Dunno if this is about numerical stability...

Contributor

strk commented Jul 8, 2014

One thing that may be related: agg_renderer logs a Scale=0 when the large ram is used. In normal conditions (fetching the whole dataset) I get to read Scale=236.425. Dunno if this is about numerical stability...

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 8, 2014

Contributor

Using gdb I see the memory growth happens during mapnik::scale_image_agg:

0x00007ffff780a5ee in void mapnik::scale_image_agg<mapnik::ImageData<unsigned int> >(mapnik::ImageData<unsigned int>&, mapnik::ImageData<unsigned int> const&, mapnik::scaling_method_e, double, double, double, double, double) () from /usr/local/lib/libmapnik.so.2.3

Dunno why there are no debugging symbols in there, the library is not stripped:

/usr/local/lib/libmapnik.so.2.3.0: ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, BuildID[sha1]=0x628b31d7f769fe2e146ceb08bf5228b2a818b7ad, not stripped
Contributor

strk commented Jul 8, 2014

Using gdb I see the memory growth happens during mapnik::scale_image_agg:

0x00007ffff780a5ee in void mapnik::scale_image_agg<mapnik::ImageData<unsigned int> >(mapnik::ImageData<unsigned int>&, mapnik::ImageData<unsigned int> const&, mapnik::scaling_method_e, double, double, double, double, double) () from /usr/local/lib/libmapnik.so.2.3

Dunno why there are no debugging symbols in there, the library is not stripped:

/usr/local/lib/libmapnik.so.2.3.0: ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, BuildID[sha1]=0x628b31d7f769fe2e146ceb08bf5228b2a818b7ad, not stripped
@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 8, 2014

Contributor

I confirm the memory growth, and excessive pause, is within calling scale_image_agg with image_ratio_x=10 and image_ratio_y=400

Contributor

strk commented Jul 8, 2014

I confirm the memory growth, and excessive pause, is within calling scale_image_agg with image_ratio_x=10 and image_ratio_y=400

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 8, 2014

Contributor

The math is not clear to me:

Mapnik LOG> 2014-07-08 20:16:54: pgraster_featureset: raster of 256x241 pixels covering extent box2d(1634712.0000000000000000,4809821.0000000000000000,1637272.0000000000000000,4812231.0000000000000000)
ext.width=2560 sourcedata.width=256                  
ext.height=96400 sourcedata.height=241

calling scale_image_agg with image_ratio_x=10 and image_ratio_y=400
done calling scale_image_agg

The number 2560 of "ext.width" matches the geographical units width of the input extent: 1637272-1634712 and sourcedata.width matches indeed the width of the input raster.

But the input extent height would be 2410 (4812231-4809821), not 96400.
Not sure where that 96400 comes out from.

Contributor

strk commented Jul 8, 2014

The math is not clear to me:

Mapnik LOG> 2014-07-08 20:16:54: pgraster_featureset: raster of 256x241 pixels covering extent box2d(1634712.0000000000000000,4809821.0000000000000000,1637272.0000000000000000,4812231.0000000000000000)
ext.width=2560 sourcedata.width=256                  
ext.height=96400 sourcedata.height=241

calling scale_image_agg with image_ratio_x=10 and image_ratio_y=400
done calling scale_image_agg

The number 2560 of "ext.width" matches the geographical units width of the input extent: 1637272-1634712 and sourcedata.width matches indeed the width of the input raster.

But the input extent height would be 2410 (4812231-4809821), not 96400.
Not sure where that 96400 comes out from.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 8, 2014

Contributor

The difference seems to come from CoordTrans:

target_ext.width=2560
target_ext.height=2410
 after transform: 
ext.width=2560
ext.height=96400
Contributor

strk commented Jul 8, 2014

The difference seems to come from CoordTrans:

target_ext.width=2560
target_ext.height=2410
 after transform: 
ext.width=2560
ext.height=96400
@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 8, 2014

Contributor

Got it, it's because the requested extent was 1634712,4809821,1634712,4809831, thus 0 (or 1) unit wide and 10 units high. Setting the requested extent (Map's maximum-extent) to have the same dimensions (0) both sides fixes the issue.

Contributor

strk commented Jul 8, 2014

Got it, it's because the requested extent was 1634712,4809821,1634712,4809831, thus 0 (or 1) unit wide and 10 units high. Setting the requested extent (Map's maximum-extent) to have the same dimensions (0) both sides fixes the issue.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 11, 2014

Contributor

All band types for "data" rasters are now supported, and all integer ones for grayscales.
RGB/RGBA ones are still only supporting 8 bit per channel

Contributor

strk commented Jul 11, 2014

All band types for "data" rasters are now supported, and all integer ones for grayscales.
RGB/RGBA ones are still only supporting 8 bit per channel

Sandro Santilli added some commits Jul 11, 2014

Sandro Santilli
Add test for rgba from subquery
NOTE: only tests 8BUI. Interpretation of other pixel sizes is indeed
undefined yet
Sandro Santilli
Improve "prescale_rasters" query to reduce function calls
Calls ST_ScaleX and ST_ScaleY once per raster rather than twice
Sandro Santilli
Allow using overviews with simple queries
Simple queries are queries like: "( SELECT * FROM mytable ) as foo".
Actually with this commit there are more cases supported, all the
ones containing a "FROM <tablename>" substring, which may or may
not break the code.

Also, the "use_overviews" parameter is not fatal anymore if the
code cannot derive a table name from the subquery.
@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 23, 2014

Contributor

You can see the plugin in action here:
http://strk.keybit.net/tmp/windshaft_viewer/index2.html
Hit Go :)

Contributor

strk commented Jul 23, 2014

You can see the plugin in action here:
http://strk.keybit.net/tmp/windshaft_viewer/index2.html
Hit Go :)

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 23, 2014

Contributor

Also see http://strk.keybit.net/tmp/windshaft_viewer/index3.html, for an example of RasterColorizer usage

Contributor

strk commented Jul 23, 2014

Also see http://strk.keybit.net/tmp/windshaft_viewer/index3.html, for an example of RasterColorizer usage

@artemp

This comment has been minimized.

Show comment
Hide comment
@artemp

artemp Jul 23, 2014

Member

Cool, looks great!

Member

artemp commented Jul 23, 2014

Cool, looks great!

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 24, 2014

Contributor

What does it take to see this merged @artemp ?

Contributor

strk commented Jul 24, 2014

What does it take to see this merged @artemp ?

@artemp

This comment has been minimized.

Show comment
Hide comment
@artemp

artemp Jul 24, 2014

Member

@strk - https://github.com/mapnik/mapnik/tree/pgraster (updated pgraster plugin for 3.x )
Looks good to me to push into master, let me just test locally.

@springmeyer - OK to merge into 2.3.x ?

Member

artemp commented Jul 24, 2014

@strk - https://github.com/mapnik/mapnik/tree/pgraster (updated pgraster plugin for 3.x )
Looks good to me to push into master, let me just test locally.

@springmeyer - OK to merge into 2.3.x ?

@springmeyer

This comment has been minimized.

Show comment
Hide comment
@springmeyer

springmeyer Jul 24, 2014

Member

👍 on merging into both 2.3.x and master at the same time. @strk - this ready to merge?

Member

springmeyer commented Jul 24, 2014

👍 on merging into both 2.3.x and master at the same time. @strk - this ready to merge?

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 25, 2014

Contributor

Yes @springmeyer, this is ready to merge in 2.3.x

Contributor

strk commented Jul 25, 2014

Yes @springmeyer, this is ready to merge in 2.3.x

@springmeyer

This comment has been minimized.

Show comment
Hide comment
@springmeyer

springmeyer Jul 28, 2014

Member

thanks @strk - one more question: are you able to maintain in both 2.3.x and master? If not then I would prefer to just merge into master. Let me know what you think. Both are close to a releasable state.

Member

springmeyer commented Jul 28, 2014

thanks @strk - one more question: are you able to maintain in both 2.3.x and master? If not then I would prefer to just merge into master. Let me know what you think. Both are close to a releasable state.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Jul 29, 2014

Contributor

I've no problem maintaining the code in both 2.3.x and master. Thank you.

Contributor

strk commented Jul 29, 2014

I've no problem maintaining the code in both 2.3.x and master. Thank you.

@springmeyer

This comment has been minimized.

Show comment
Hide comment
@springmeyer

springmeyer Jul 31, 2014

Member

Noting, before merge:

  1. Copyright needs updated as per https://github.com/mapnik/mapnik/blob/master/docs/contributing.markdown#copyright

  2. I would greatly prefer this be re-created in a single commit before merging.

Member

springmeyer commented Jul 31, 2014

Noting, before merge:

  1. Copyright needs updated as per https://github.com/mapnik/mapnik/blob/master/docs/contributing.markdown#copyright

  2. I would greatly prefer this be re-created in a single commit before merging.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 1, 2014

Contributor

No problem, will do. Only needed for the 2.3.x branch ?

Contributor

strk commented Aug 1, 2014

No problem, will do. Only needed for the 2.3.x branch ?

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 1, 2014

Contributor

Please refer to #2329 for a single-commit pull for 2.3.x branch

Contributor

strk commented Aug 1, 2014

Please refer to #2329 for a single-commit pull for 2.3.x branch

@strk strk closed this Aug 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment