Skip to content

Commit

Permalink
Fix WMS 1.3. compliance: distort image if width/height ratio of bbox …
Browse files Browse the repository at this point in the history
…is different to WIDTH/HEIGHT of requested image

Not decrease but increase image
  • Loading branch information
rldhont committed Dec 13, 2016
1 parent f8e2b85 commit d44f1eb
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions src/server/qgswmsserver.cpp
Expand Up @@ -2006,13 +2006,13 @@ QImage* QgsWMSServer::createImage( int width, int height, bool useBbox ) const
{
if ( mapWidthHeightRatio >= imageWidthHeightRatio )
{
//decrease image height
height = width / mapWidthHeightRatio;
//increase image height
height = width * mapWidthHeightRatio;
}
else
{
//decrease image width
width = height * mapWidthHeightRatio;
//increase image width
width = height / mapWidthHeightRatio;
}
}
}
Expand Down

12 comments on commit d44f1eb

@rldhont
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mhugent and @pka, I have made this commit to fix an issue in QGIS Server. We test it in QGIS as a WMS client with @elpaso.
It's seems the fix introduces an issue with OpenLayers3. I have tested it with OpenLayers2 and don't have any issue.
Do you have any other client which use distortion ?

@mhugent
Copy link
Contributor

Choose a reason for hiding this comment

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

This 'fix' breaks the OGC compliance of QGIS server! You can run the OGC test suite to see so, there is a visual test showing two pictures with the following question: 'The two pictures above should depict the same scene, but the picture on the right should be stretched so it is twice as tall as the picture on the left. Are the pictures correct?'. With the original changes, this is the case. With this 'fix', the picture on the right is only half the size of the left picture (but it should be stretched to be double size).

@rldhont
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mhugent but without this fix, QGIS Desktop can't use QGIS Server as a WMS 1.3.0 provider. Do we have an issue in QGIS Desktop WMS provider ?

With @yjacolin, we produced the same request with MapServer and GeoServer, and with my fix we get the same rendering.

During the hackfest, @vmora has working on reproduce the cite tests. But don't have enough time to do it.

If QGIS Desktop WMS provider has no issue with distort images, my fix is not enough, if we want QGIS Server compliance and rendering as others.

@mhugent
Copy link
Contributor

Choose a reason for hiding this comment

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

QGIS desktop (WMS client) sometimes makes requests where width/height ratio and bbox do not match. So yes, this is a bug and it should be fixed.

@luipir
Copy link
Contributor

@luipir luipir commented on d44f1eb Dec 22, 2016

Choose a reason for hiding this comment

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

would make sense to have two set of tests?

  1. one set to test custom qgis server WMS parameters
  2. one set to test standard WMS parametes

the 2 test suite would be good to test against at leaset two map servers (e.g. qgis server and mapserver/geoserver). In this way we could be able to find "endogamy" related errors like this one.

@rldhont
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The project https://github.com/camptocamp/ms_perfs can help to compare images produces by QGIS Server 2.18, QGIS Server 3, MapServer and GeoServer. It provides the same images from the same layers.
During the French QGIS User meetings, @yjacolin has presented this project http://www.agrotic.org/blog/wp-content/uploads/2016/12/02_performance_qgis_server.pdf. On slide 11 to 13, there are examples of distort images provided by MapServer, GeoServer and QGIS Server before this commit. You can see that QGIS Server does not provide an image in the same way as others.

So to pass CITE tests, we have to enhance this commit and not just revert it.

@giohappy
Copy link
Contributor

Choose a reason for hiding this comment

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

@rldhont for the OL3 part I've suggested a patch that seems to mitigate the problem.
We were facing serious issues caused by roundings and the new QGIS strict behaviour. Changing tis rounding strategy has solved it.

@giohappy
Copy link
Contributor

Choose a reason for hiding this comment

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

just as a followp @rldhont I've committed a PR to OL3 that is going to be merged that solves the problem at least for this client + QGIS Server

@rldhont
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @giohappy I have updated the way QGIS Server stretched image #3906
We used the same method as MapServer, and I fixed an issue due to reverse axis in EPSG:4326. I hope all will be ok.

@giohappy
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rldhont no problem, other clients will benefit from it. OL3 will behave correctly (coherent axpect ratio for BBOX and image sizes) independently of the server strategy.

@giohappy
Copy link
Contributor

Choose a reason for hiding this comment

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

PS: and we needed it to work for QGIS Server LTR (2.14.10) which is affected by the issue.

@gioman
Copy link
Contributor

@gioman gioman commented on d44f1eb Jan 12, 2017

Choose a reason for hiding this comment

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

Will this fix trigger new point releases?

Please sign in to comment.