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

[BUGFIX][Server] WMS compliance: stretched, distort, increase, decrease #3906

Merged
merged 2 commits into from Jan 3, 2017

Conversation

Projects
None yet
5 participants
@rldhont
Contributor

rldhont commented Dec 26, 2016

The commit d44f1eb seems to break some WMS 1.3.0 client and WMS compliancy.
The commit d44f1eb has been written to fix an issue with QGIS WMS Client and to render image like other WMS Server.

This commit has been written to fix issue introduce by d44f1eb.
It is based on MapServer code:

@rldhont rldhont added this to the 2.14 milestone Dec 26, 2016

@rldhont rldhont requested review from elpaso, mhugent, sbrunner and mhugo Dec 26, 2016

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Dec 26, 2016

Contributor

Since this seems to be a tricky calculation I'd suggest adding unit tests to protect this logic.

Contributor

nyalldawson commented Dec 26, 2016

Since this seems to be a tricky calculation I'd suggest adding unit tests to protect this logic.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Dec 26, 2016

Contributor

@nyalldawson, this commit comes from this discussion d44f1eb#commitcomment-20259007
Tests has to be updated but unit test are not enough. We need to verify that QGIS WMS client and other WMS client are not broken.

Contributor

rldhont commented Dec 26, 2016

@nyalldawson, this commit comes from this discussion d44f1eb#commitcomment-20259007
Tests has to be updated but unit test are not enough. We need to verify that QGIS WMS client and other WMS client are not broken.

@mhugent

This comment has been minimized.

Show comment
Hide comment
@mhugent

mhugent Dec 30, 2016

Contributor

As the ms calculation uses a mixture of both ratios, I don't think it is 100% correct.
But it does not matter. The image distortion is IMHO a bad idea of the OGC spec. We should disable it completely or use the ms calculation in this PR to not have any troubles with WMS clients.

Contributor

mhugent commented Dec 30, 2016

As the ms calculation uses a mixture of both ratios, I don't think it is 100% correct.
But it does not matter. The image distortion is IMHO a bad idea of the OGC spec. We should disable it completely or use the ms calculation in this PR to not have any troubles with WMS clients.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 2, 2017

Contributor

Thanks @mhugent

@elpaso can you test this PR ?

Contributor

rldhont commented Jan 2, 2017

Thanks @mhugent

@elpaso can you test this PR ?

@elpaso

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Jan 2, 2017

Contributor

@rldhont you read my mind, I've just built your branch ... :)

Contributor

elpaso commented Jan 2, 2017

@rldhont you read my mind, I've just built your branch ... :)

@elpaso

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Jan 2, 2017

Contributor

@rldhont tested this PR with QGIS server and QGIS client and there is exactly the same problem we noticed in Lyon, in the attached image you can see the same layer (world shapefile EPSG:4326) rendered directly into QGIS (grey outline) and served through QGIS Server (the coloured polygons).
You can see how the grey outline is the correct one.
stretch_error

Contributor

elpaso commented Jan 2, 2017

@rldhont tested this PR with QGIS server and QGIS client and there is exactly the same problem we noticed in Lyon, in the attached image you can see the same layer (world shapefile EPSG:4326) rendered directly into QGIS (grey outline) and served through QGIS Server (the coloured polygons).
You can see how the grey outline is the correct one.
stretch_error

@elpaso

@rldhont, I've tested master again without this PR and it seems to me the right behavior. This PR re-introduces the bug that we've seen in Lyon.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 2, 2017

Contributor

hi @elpaso, can you provide the URL to the data and the URL generated by QGIS desktop to reprodiuce the issue?

Contributor

rldhont commented Jan 2, 2017

hi @elpaso, can you provide the URL to the data and the URL generated by QGIS desktop to reprodiuce the issue?

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 2, 2017

Contributor

@mhugent can you provide some example which describe the issue with openlayers that @pka has explain on Twitter?

Contributor

rldhont commented Jan 2, 2017

@mhugent can you provide some example which describe the issue with openlayers that @pka has explain on Twitter?

@elpaso

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Jan 2, 2017

Contributor

@rldhont you can easily reproduce it:

  1. create a QGIS project with a single shapefile layer EPSG:4326 whole world
  2. serve that project through QGIS Server

then ...

  1. open QGIS and load the EPSG:4326 whole world shapefile layer directly inside QGIS
  2. add the WMS layer created in the previous steps.
  3. enjoy the difference

I checked the QGIS Desktop created URLs and they seems correct: both the BBOX and
HEIGHT/WIDTH seem correct.

Example URL generated by QGIS WMS provider (untiled):
?MAP=/opt/qgis-server/projects/world_qt5.qgs&SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&BBOX=-40.42095738741319622,-189,71.67899928492427364,189&CRS=EPSG:4326&WIDTH=1046&HEIGHT=310&LAYERS=world&STYLES=&FORMAT=image/jpeg&DPI=220&MAP_RESOLUTION=220&FORMAT_OPTIONS=dpi:220

See what happens here with current master (not your PR):
immagine

See what happens with your PR (same call):
immagine

Contributor

elpaso commented Jan 2, 2017

@rldhont you can easily reproduce it:

  1. create a QGIS project with a single shapefile layer EPSG:4326 whole world
  2. serve that project through QGIS Server

then ...

  1. open QGIS and load the EPSG:4326 whole world shapefile layer directly inside QGIS
  2. add the WMS layer created in the previous steps.
  3. enjoy the difference

I checked the QGIS Desktop created URLs and they seems correct: both the BBOX and
HEIGHT/WIDTH seem correct.

Example URL generated by QGIS WMS provider (untiled):
?MAP=/opt/qgis-server/projects/world_qt5.qgs&SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&BBOX=-40.42095738741319622,-189,71.67899928492427364,189&CRS=EPSG:4326&WIDTH=1046&HEIGHT=310&LAYERS=world&STYLES=&FORMAT=image/jpeg&DPI=220&MAP_RESOLUTION=220&FORMAT_OPTIONS=dpi:220

See what happens here with current master (not your PR):
immagine

See what happens with your PR (same call):
immagine

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 2, 2017

Contributor

@elpaso thanks!
@mhugent any idea about the difference?

Contributor

rldhont commented Jan 2, 2017

@elpaso thanks!
@mhugent any idea about the difference?

@sbrunner

Why should we change the width and height, shouldn't they simply be unchanged?

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 3, 2017

Contributor

@sbrunner the image has to be stretched by the server to be WMS 1.3.0 compliant

Contributor

rldhont commented Jan 3, 2017

@sbrunner the image has to be stretched by the server to be WMS 1.3.0 compliant

@sbrunner

This comment has been minimized.

Show comment
Hide comment
@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 3, 2017

Contributor

@mhugent and @elpaso this issue is due to the fact that the image ratio does not take account of the Change x- and y- of BBOX for WMS 1.3.0 if axis inverted.

I'll enhance the code to do so!

Contributor

rldhont commented Jan 3, 2017

@mhugent and @elpaso this issue is due to the fact that the image ratio does not take account of the Change x- and y- of BBOX for WMS 1.3.0 if axis inverted.

I'll enhance the code to do so!

if ( crs.compare( "CRS:84", Qt::CaseInsensitive ) == 0 )
{
crs = QString( "EPSG:4326" );
mapExtent.invert();

This comment has been minimized.

@sbrunner

sbrunner Jan 3, 2017

Contributor

Don't you have unwanted double invert? (with the line 1991)

@sbrunner

sbrunner Jan 3, 2017

Contributor

Don't you have unwanted double invert? (with the line 1991)

This comment has been minimized.

@rldhont

rldhont Jan 3, 2017

Contributor

no crs84 is an invertion of EPSG:4326.
This code has been copy from the copyMapSettings.

@rldhont

rldhont Jan 3, 2017

Contributor

no crs84 is an invertion of EPSG:4326.
This code has been copy from the copyMapSettings.

This comment has been minimized.

@sbrunner

sbrunner Jan 3, 2017

Contributor

OK, thanks :-)

@sbrunner

sbrunner Jan 3, 2017

Contributor

OK, thanks :-)

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 3, 2017

Contributor

@elpaso and @mhugent the code has been updated to take account of axis invertion.

So with a world countries data we have:
world_countrie
SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&BBOX=-40.42095738741319622,-189,71.67899928492427364,189&CRS=EPSG:4326&WIDTH=1046&HEIGHT=310&LAYERS=countries&STYLES=&FORMAT=image/jpeg&DPI=220&MAP_RESOLUTION=220&FORMAT_OPTIONS=dpi:220

And I have tested with the OGC CITE ratio-aspect request and we have:
without stretched
cite-wms-aspect-ratio
SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&LAYERS=cite:Streams,cite:Lakes,cite:Ponds,cite:Bridges,cite:RoadSegments,cite:DividedRoutes,cite:Buildings,cite:MapNeatline&STYLES=&CRS=CRS:84&BBOX=-0.005,-0.0025,0.005,0.0025&WIDTH=200&HEIGHT=100&FORMAT=image/jpeg
with stretched
cite-wms-aspect-ratio-stretched
SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&LAYERS=cite:Streams,cite:Lakes,cite:Ponds,cite:Bridges,cite:RoadSegments,cite:DividedRoutes,cite:Buildings,cite:MapNeatline&STYLES=&CRS=CRS:84&BBOX=-0.005,-0.0025,0.005,0.0025&WIDTH=200&HEIGHT=200&FORMAT=image/jpeg

Are you agree with the code now ?

Contributor

rldhont commented Jan 3, 2017

@elpaso and @mhugent the code has been updated to take account of axis invertion.

So with a world countries data we have:
world_countrie
SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&BBOX=-40.42095738741319622,-189,71.67899928492427364,189&CRS=EPSG:4326&WIDTH=1046&HEIGHT=310&LAYERS=countries&STYLES=&FORMAT=image/jpeg&DPI=220&MAP_RESOLUTION=220&FORMAT_OPTIONS=dpi:220

And I have tested with the OGC CITE ratio-aspect request and we have:
without stretched
cite-wms-aspect-ratio
SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&LAYERS=cite:Streams,cite:Lakes,cite:Ponds,cite:Bridges,cite:RoadSegments,cite:DividedRoutes,cite:Buildings,cite:MapNeatline&STYLES=&CRS=CRS:84&BBOX=-0.005,-0.0025,0.005,0.0025&WIDTH=200&HEIGHT=100&FORMAT=image/jpeg
with stretched
cite-wms-aspect-ratio-stretched
SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&LAYERS=cite:Streams,cite:Lakes,cite:Ponds,cite:Bridges,cite:RoadSegments,cite:DividedRoutes,cite:Buildings,cite:MapNeatline&STYLES=&CRS=CRS:84&BBOX=-0.005,-0.0025,0.005,0.0025&WIDTH=200&HEIGHT=200&FORMAT=image/jpeg

Are you agree with the code now ?

@elpaso

elpaso approved these changes Jan 3, 2017

@elpaso

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Jan 3, 2017

Contributor

@rldhont tested the latest commit and it looks good to me now, it also stretches correctly. Good job!

Contributor

elpaso commented Jan 3, 2017

@rldhont tested the latest commit and it looks good to me now, it also stretches correctly. Good job!

rldhont added some commits Dec 26, 2016

[BUGFIX][Server] WMS compliance: stretched, distort, increase, decrease
The commit d44f1eb seems to break some WMS 1.3.0 client and WMS compliancy.
The commit d44f1eb has been written to fix an issue with QGIS WMS Client and to render image like other WMS Server.

This commit has been written to fix issue introduce by d44f1eb.
It is based on MapServer code:
* https://github.com/mapserver/mapserver/blob/master/mapdraw.c#L115
* https://github.com/mapserver/mapserver/blob/master/HISTORY.TXT#L3768

And take account of axis invertion for output CRS.

@rldhont rldhont merged commit 9fd65a2 into qgis:master Jan 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

dmarteau added a commit to dmarteau/QGIS that referenced this pull request Jan 7, 2017

dmarteau added a commit to dmarteau/QGIS that referenced this pull request Jan 7, 2017

dmarteau added a commit to dmarteau/QGIS that referenced this pull request Jan 9, 2017

dmarteau added a commit to dmarteau/QGIS that referenced this pull request Jan 9, 2017

dmarteau added a commit to dmarteau/QGIS that referenced this pull request Jan 10, 2017

rldhont referenced this pull request Jan 11, 2017

Fix WMS 1.3. compliance: distort image if width/height ratio of bbox …
…is different to WIDTH/HEIGHT of requested image

Not decrease but increase image

@rldhont rldhont added the Server label Jan 11, 2017

@rldhont rldhont deleted the rldhont:server-wms-stretch-compliancy branch Jan 19, 2017

sbrunner added a commit to sbrunner/QGIS that referenced this pull request Jan 20, 2017

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