Feature #8725: Fast rendering of geometries

[ahuarte47]

Implements fast rendering of LineStrings and Polygons pre-applying a view threshold filter to the geometries to render in qgis.

Also disable 'Antialiasing' when it is possible.

View Table of test results in 'http://hub.qgis.org/issues/8725'

[m-kuhn]

I cannot compile:

CMake Error at src/core/CMakeLists.txt:599 (ADD_LIBRARY):
  Cannot find source file:

  qgsrenderstats.cpp

Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp
.hxx .in .txx

Instead of this file you could also use the included tool qgis_bench which renders a .qgs project file a couple of times and measures the timings.

[ahuarte47]

Sorry, the real location is....

.../src/core/symbology-ng/qgsrenderstats.cpp

now it is unused with a disabled #define. I have added this "temporal" class for demostrate the % of simplification of the geometries without visible changes. Finally it must be deleted.

Do you want that fix CMakeLists.txt ?
If you prefer I remove that class and their references be definitively

[m-kuhn]

If this was code which is specific just for the development of this particular feature, please remove it.

[ahuarte47]

ok, I change as you say

[m-kuhn]

FYI: You can just push to a branch for which you have a pull request open and the new commits will appear.

[ahuarte47]

I've removed the class for rendering statistics
I reopen the pull

[m-kuhn]

Hi.
I did a short test and indeed noticed a considerable speed improvement. But it seems, that anti-aliasing is disabled and therefore the speed improvement comes (at least partly) at the cost of a reduced quality. Please fix this issue.

[ahuarte47]

The QPainter::Antialiasing is disabled only when the geometry is so far from the point of view that can be replaced by a false frame, I disable the antialiasing because I is not perceived.

View ... QgsFillSymbolLayerV2::_renderPolygon
or
QgsSimpleLineSymbolLayerV2::renderPolyline

Before in "QgsFeatureRendererSimplifier::SimplifyGeometry" is calculated if the geometry can be replaced by a false frame, or by a simplified geometry

[m-kuhn]

I did not review the request in detail. It is probable, that the algorithm itself does not do any antialiasing, so any line which is not horizontal or vertical will have stairs effects. For comparison two screenshots, without and with your patch:

[ahuarte47]

Ummm, I check the code for line generalization, thanks!

[wonder-sk]

Hi Alvaro

it is great to see some optimizations coming for the rendering of vectors!

I have looked at the patch and there are some very nice ideas.

I have some comments and questions:

• antialiasing - I think the simplification must not have any impact on antialiasing settings. Even small features may have a style that would render them with ugly alias if antialiasing gets turned off. It should be the user to decide whether speed or quality is preferred.
• regarding OGR feature iterator change; OGR docs say that OGR_L_GetExtent() can be expensive operation - so for some drivers this may introduce performance penalty.
• threshold - I'm curious why you have chosen 1.3 for the threshold. In my understanding, this means that a new point as far as 1.3px can still be dismissed - isn't that too much? :-) Maybe this magic number should be customizable?
• clipping of lines has been removed - I am afraid we may still need it. If I remember correctly, the clipping is useful for large features when zoomed in - internal Qt clipper is/was quite inefficient and causing large delays. Maybe you can find more details in git history.

Finally some style notes:

• there are some invalid characters in simplifier's cpp file (lines 90, 159)
• function names should start with lowercase letter
• preferably code should be correctly indented (scripts/astyle.sh)

[ahuarte47]

Hello Martin, thank you very much for the tips, lessons are for me.
I will answer:

My time tests shows that the 'Antialiasing' is expensive, if the geometriy is far enough the user does not perceive its true outline, and I replace it with his BBOX and then I disable antialiasing. Another thing is that as Matthias says I has an error in the decision of when to do this trick.

About of the cost of OGR_L_GetExtent, you're right, but depending on the format. If the full envelope is stored in the data layer, (ie shape, dgn, las ...) is better not to apply the filter, but if it must be calculated (eg, postgis, sql, xyz ...) is better apply the filter. I delete this change.

The thresold 1.3 is experimental, I would like play with this value to check results.

I re-insert the clipping of lines, I don't know this issue. Sorry.

I will have your notes in mind, thank you very much!

Later I would like to change the GDAL-OGR shape reader, I have some speed improvements, especially because it does not use FileMapping.

[ahuarte47]

Newer commit with the suggested changes,

Matthias I don't get your stairs effects, can I test your data ?

This is the code when the alialiasing state is changed for a "generalized" line, and You can see the render state is restored after the drawing.

QgsSimpleLineSymbolLayerV2::renderPolyline()....

// Disable 'Antialiasing' if the geometry was generalized in the current RenderContext.
if ( context.generalizedByBoundingBox() && p->renderHints() & QPainter::Antialiasing )
{
p->setRenderHint( QPainter::Antialiasing, false );
p->drawPolyline( points );
p->setRenderHint( QPainter::Antialiasing, true );
return;
}

When the geometry is only simplified, the 'Antialiasing' remains unedited.
For polygons, the code is equivalent.

[m-kuhn]

Hi Alvaro,

The problem still exists with the current patchset. I would be very surprised, if you could just turn AA off, without any effect. I guess you could only turn it off if a feature actually is 1 pixel or smaller.

Your patch is simplifying lines as far if I am not mistaken? You could also have a look at this very small patch for another part of the codebase doing something similar in a very easy way: e04fc53. What exactly is the difference of your patch?

Would your patch also bring benefits to geometries already simplified by e.g. postgis' ST_Simplify? I guess we should take this simplification rather further up the pipeline (into the provider code), so a provider can choose if it wants to use local simplification code or fetch simplified geometries if available.

[alexbruy]

@ahuarte47 what dou you think about using (adopted) simplify.js from http://mourner.github.io/simplify-js/ for simplification? Seems alg used in this library fast and provides more control on quality/speed.

[ahuarte47]

Hi Matthias, you are right, my intention is to turn off the AA just for 1-pixel features. Can I test your shape ? wich is your spatial reference system ?

The algorithm does two main tasks, test if the BBOX is 1-pixel size, an then draw this BBOX with the AA disabled, or simplify as fast possible the geometry with the map2pixel factor. The patch that you say is it exactly, but my code saves cpu and memory callocs doing everything in a simple function (wkb reading and output simplification).

For for these purposes I humbly think that ST_Simplify() and other similar algorithms should not be used, as the aim is to optimize as much as possible the massive data rendering, not the geometric quality of them.

[m-kuhn]

Alvaro,

The dataset is the default QGIS test dataset of alaska http://download.osgeo.org/qgis/data/qgis_sample_data.zip
EPSG:2964

Can you elaborate a bit more on the difference between "massive data rendering optimization" and "geometric quality".

[ahuarte47]

The Douglas–Peucker algorithm (ST_Simplify...) creates a new geometry based on the maximum distance between the original curve and the simplified curve. The result is more is more accurate, the difference between the original and the result is smaller, but has a higher cost of processing.

If what we want is just paint geometry (many many geometries) other simplification can be used more directly by considering the distances between consecutive points without the user noticing. If this result were for geo-process this information, it would not be.

[m-kuhn]

So the main point is, that the database simplification implementation may have a worse performance than one we can build in-code optimized for drawing only. A valid point.
On the other hand, simplifying in the database has also a few advantages like

• Reduces the amount of transferred data
• The database machine may have more power to do calculations than the client

and last but not least, other simplification algorithms may be implemented there as well. Moving the code to the provider level would at least leave the option open to make use of these advantages.

[alexbruy]

Also we should remember that database (at least PostGIS) can simplify not only lines, but also polygons preserving topology. So IMO it makes sense to provide for users choice what to use

[ahuarte47]

Hi matthias, I have stairs effects in alaska! :-) I think because of there are multi-polygons. I review my code, thank you very much.

About the simplification of geometries, we can do rendering tests using ST_Simplify in the database (Some formats as postgis supports this, but others as shape no but I think that will use the Geometry class), or using the post-simplification. It seems perfect me.

[ahuarte47]

alexbruy, matthias, ... thanks for your good advice, I will investigate also the simplification at OGR provider, using build-in mode and DB-side.

:-)

[ahuarte47]

I have analyzed the code for implement the vector simplification in the vector-providers.

I found that it must be in the QgsAbstractFeatureIterator class. Each vector provider (qgsogrfeatureiterator, qgsmssqlfeatureiterator, qgspostgresfeatureiterator, ...) inherits from this base class. As usual, each provider read the feature in one manner, but all finally has a WKB byte stream, a QgsFeature class or a OgrFeature class.

I can implement three similar helper simplification methods for each geometry representation (wkb, QgsFeature or OgrFeature) adding previously to the QgsFeatureRequest class the info need for simplify the geometry and a flag entry for really force the simplification.

There are vector-providers could simplify the geometries using ST_Simplify in the request query (ie; postgres, mysql) and other would use one of the three new helper simplification methods.

These changes will make me change the code of the providers, or those for accelerate with simplification (OGR, postgis, ....). And now I get the question of whether this effort, the acceleration is secured, then will be accepted for this branch be merged the the main branch qgis. What do you think?

I am going to develop a new branch 8725-OGR.

[m-kuhn]

Hi Alvaro,

to me this sounds good. I'm not sure, if you need three different simplification methods. In the end, all iterators should yield a QgsFeature with a QgsGeometry (which manages a geometry and can convert this to a GEOS geometry or wkb geometry on request), so at the end, they all provide the same kind of data. I did not check, but I would be surprised to find an OGR feature/geometry there.

IMO, the provider itself should not have to explicitly call the simplification method but rather have the possibility to overwrite the method and handle the simplification in another way. I.e. local simplification code would be added to a provider by default as long as it is not overwritten.

But apart from these few comments, I really like your approach.

[ahuarte47]

Hi Matthias, I'm glad to read you.

It is right that all in the end yield a QgsFeature, but there is a critical point where unnecessarily qgis spent much cpu and ram, is on ....

bool QgsOgrFeatureIterator::readFeature( OGRFeatureH fet, QgsFeature& feature )
{
...
OGRGeometryH geom = OGR_F_GetGeometryRef( fet );
...
// get the wkb representation !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
unsigned char *wkb = new unsigned char[OGR_G_WkbSize( geom )];
OGR_G_ExportToWkb( geom, ( OGRwkbByteOrder ) QgsApplication::endian(), wkb );
...
}

I would like insert one "event" or "method" for pre-simlipfy the temporal OGR-geometry prior export it to a WKB stream. This WKB stream later is saved to the QsgFeature for paint.

I am sure that insert the simplification at this point give me further speed!

I had thought of adding a new optional argument in the method 'readFeature' in the QgsAbstractFeatureIterator class (eg QsgFeatureRequestArgs) which specifies information usable for the generalization, map2pixel info and CoordinateTransfromation of the layer) and is the provider that decides how to do it (using ST_simplify or methods ad-hoc). Of course, I will a flag indicating that the simplifcation must be executed. This will maintain the current operation of the "not accelerated" providers

I am developing this in 8725-OGR branch to show more clearly (My English you see that is quite poor :-()

On the other hand, I have append a new resolved ticket in gdal with a first optimization of the shape reader.
http://trac.osgeo.org/gdal/ticket/5272
I hope that they accept me!

The three methods of simplification is likely to be reduced to two, one for the OGR-geometry (the provider most used) and one for the QGS-Geometry to help developers of other providers.

[m-kuhn]

This is an internal thing for OGR only and therefore the layout of the abstract iterator should not change because of this. If simplification code is handled in the OGR provider, post-fetching generic simplification should be turned off instead (if my proposed method using inheritance is used, by overwriting the simplification class with code that does not change the geometry) just like it is done for ST_Simplify or anything else. I would start with the generic code (post-fetching) which will have the biggest impact, as it works on all iterators and then fine-tune the OGR iterator and check if there is an additional performance gain.

Please also note, that normally a feature runs through 2 different iterators, the QgsVectorLayerIterator and the provider's iterator. It would be nice to ensure in the QgsVectorLayerIterator that features already simplified by provider code are not simplified again.

[ahuarte47]

OK, thanks, so I'm developing, I keep informed

[ahuarte47]

Done, new branch...
https://github.com/ahuarte47/QGIS/tree/Issue_8725-OGR
ahuarte47@db8eaf0

As we speak, I have implemented the improvement of rendering in a new ad-hoc featureIterator with the possibility that each vector-provider implements its own method of pre-simplification.

The performance is the same as the first version I developed (~3x faster :-)).

Now I will implement the pre-simplification for OGR-provider, hopefully improve performance even more!
I hope I like it!

[ahuarte47]

Done:
https://github.com/ahuarte47/QGIS/tree/Issue_8725-OGR
ahuarte47@3ddcdac

It also implements the simplification of geometries in the OGR-provider pre-fetch the features. It improves the post-simplification around 5%.

I think that this branch is ready por test! :-)

[m-kuhn]

Hi Alvaro,

Can you push your changes to the Issue_8725 branch please (I guess this one is obsolete now?) so there is no need to add a new remote for testing because the current code is linked in this pull request.

Just a short feedback of what I noticed when skimming the code.

All in all I'm still in favor of the current approach (adding required details to the request + default simplification implementation in abstract iterator + possibility to override)

What I don't like is, that the QgsFeatureRequest inherits from QgsMapRequest. If inheritance is involved, it should be the other way round, because not every feature request is a map request (i.e. no map request for geometry-less tables)

Another minor thing is, that the simplification code should probably not be in the request, but rather in the place where the simplification is done, so in the iterator or rather in its own simplification class (maybe other parts of the code need it...)

As I said, this is just from a very very short and rough look at the code.

[ahuarte47]

Hi Matthias

I currently use the brach:
https://github.com/ahuarte47/QGIS/tree/Issue_8725-OGR

The last commit removes the QgsMapRequest class, and the simplification is configurable by vector layer.
I have implemented the "simplify" methods in QgsFeatureRequest so it can be used on other vector-providers, other feature iterators or other drawing code.

I have pending show in the properties form of layer a check for enable/disable the rendering simplification.
Regards

[mhugent]

I tested with a polygon layer and exported to pdf in the print compolser. There are well visible gaps in the printout. You probably need to adapt the simplification tolrance because in print composer, units are mm, not pixels.

[gioman]

Hi Alvaro, I made a few simple tests to see the speed to open a few quite large shapefiles

http://hub.qgis.org/issues/8725#note-20

Cheers!

[ahuarte47]

Hi Giovanni, Some shapes...

http://idena.navarra.es/descargas/CARTO1_Lin_6CNivelD.zip
http://idena.navarra.es/descargas/GEOLOG_Pol_Litologia.zip

I am going to test your shapes!
Thanks!

[ahuarte47]

@mhugent, I will test this bug, I will fix it!

Thanks!

[ahuarte47]

New commit fixing the bug reported by @mhugent
Also contains a minor improvement

[ahuarte47]

Hi, I made tests with 'qgis_bench' to see the speed.

http://hub.qgis.org/issues/8725#note-23

Regards

[mhugent]

Are those tests always zoomed on the full dataset extent? How is it if you zoom further in?

[ahuarte47]

Yes, all test always zoomed on the full dataset extent, when the you zoom further in the map the simplification is lower, but then the number of geometries is lower. Both aspects compensate the result.

[ahuarte47]

The drawing simplification can be configurable each vector layer.
I propose to add a new option in the properties layer's form so that the user can configure this feature.

http://hub.qgis.org/issues/8725#note-25

Please, call for discussion for approval

[ahuarte47]

Hi, I have implemented a new tab called 'Rendering' in the options form of the layer where the user can configure the drawing simplification.

http://hub.qgis.org/issues/8725#note-32

Regards

[ahuarte47]

Umm, I want to test an experiment.

I see that qgis draws the geometries (LineStrings or Polygons) in independent calls ("painter->drawPolyline" or "painter->drawPolygon()" calls). I have experience using GDI++ as renderer and it is very faster joining n-geometries in one geometry-path and then flush this complex object to the renderer.

I want check how qt-painter (or qt-image) behaves using this trick (It not is applicable in all layer styles but it is for the simple layer style). Someone knows something about it?

Thank you very much.

[ahuarte47]

Hi! My experiment was failed. I have not gained any speed improved joining the individual geometries (e.g. polygons) together in a complex QPainterPath for later paint it.

So I think this patch is finished for approval. I've missed something? or am I too naive?

NOTE:
Now, I am going to optimize in gdal the access to files (shapefiles, ....) using "FileMapping" with the library "boost". Although a developer this library believes that Linux will not perceptible improvement in speed, I think in windows I bet it does, and by far. I will implement this, perform a speed test with shapefiles and if all goes well to see if it is approved.

Regards

[gioman]

Hi Alvaro, I guess we just need to make a lot of testing. I know many guys that are willingly to give it a try, but it would be needed a Windows installer. I can just compile (and create packages) on Linux. Can you create a Windows installer that includes your work? cheers!

[ahuarte47]

Hi Giovanni, I am going to generate the windows installer for test this patch.
Thank you very much!

[alexbruy]

Hi Alvaro. Please close this pull-request and submit a new one, that contains all your work, including updated GUI, as this seems outdated. Having this pull instead of correct one is confusing

[ahuarte47]

Done, new pull request:

#980

Thanks!