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 for wms requests with width or height of one #4629

Closed
wants to merge 1 commit into
base: branch-6-2
from

Conversation

Projects
None yet
4 participants
@mkofahl
Contributor

mkofahl commented Apr 10, 2013

Fix an issue for WMS requests with width or height of 1, which made MapServer throwing an exception 'Invalid image extent, minx=nan, miny=nan, maxx=nan, maxy=nan', because of division by null.

Fix an issue for WMS requests with width or height of 1, which made M…
…apServer throwing an exception 'Invalid image extent, minx=nan, miny=nan, maxx=nan, maxy=nan', because of division by null.
@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Apr 10, 2013

Member

I'm not sure this is the correct fix for this issue, as from what I understand a 1 pixel request is going to be undefined given mapserver's extent model. see also #2843, #2175, #3697, #1916 , #2015 and probably a few others :)
however, as this PR is to fix exceptions to requests that are being sent by existing clients, I think we can afford to be non rigourous, knowing that this will only affect 1px requests.
cc @sdlime

Member

tbonfort commented Apr 10, 2013

I'm not sure this is the correct fix for this issue, as from what I understand a 1 pixel request is going to be undefined given mapserver's extent model. see also #2843, #2175, #3697, #1916 , #2015 and probably a few others :)
however, as this PR is to fix exceptions to requests that are being sent by existing clients, I think we can afford to be non rigourous, knowing that this will only affect 1px requests.
cc @sdlime

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Apr 10, 2013

Member

I'm thinking the right fix is to simply trap the case width=1, height=1 and then set ox=oy=0. The result will be an extent where minx=maxx and miny=maxy, a point (or a single pixel). An extent like that is valid elsewhere in the code. @dmorissette

Steve

Member

sdlime commented Apr 10, 2013

I'm thinking the right fix is to simply trap the case width=1, height=1 and then set ox=oy=0. The result will be an extent where minx=maxx and miny=maxy, a point (or a single pixel). An extent like that is valid elsewhere in the code. @dmorissette

Steve

@mkofahl

This comment has been minimized.

Show comment
Hide comment
@mkofahl

mkofahl Apr 11, 2013

Contributor

Now I know where the requests come from: at least QGis 1.8.0 and ArcMap 10 use the max extent from the capabilities document when zooming out and scale down the image size instead - from the view size down to 1x1 pixels. So this behavior won't happen for worldwide dataets, but for small-scaled datasets this error willhappen now and then.

Contributor

mkofahl commented Apr 11, 2013

Now I know where the requests come from: at least QGis 1.8.0 and ArcMap 10 use the max extent from the capabilities document when zooming out and scale down the image size instead - from the view size down to 1x1 pixels. So this behavior won't happen for worldwide dataets, but for small-scaled datasets this error willhappen now and then.

@mkofahl

This comment has been minimized.

Show comment
Hide comment
@mkofahl

mkofahl Apr 11, 2013

Contributor

@sdlime Setting ox,oy=0 doesn't work, unfortunately. It's because msAdjustExtent does return nan as cellsize.

Contributor

mkofahl commented Apr 11, 2013

@sdlime Setting ox,oy=0 doesn't work, unfortunately. It's because msAdjustExtent does return nan as cellsize.

@mkofahl

This comment has been minimized.

Show comment
Hide comment
@mkofahl

mkofahl Apr 11, 2013

Contributor

Even if the case of 1 pixel is not defined in the current box model, a single pixel will have a cellsize. Actually, min-max, isn't it?

Contributor

mkofahl commented Apr 11, 2013

Even if the case of 1 pixel is not defined in the current box model, a single pixel will have a cellsize. Actually, min-max, isn't it?

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Apr 11, 2013

Cellsize should be 0 plus minx=maxx and miny=maxy coming out of this function. If set explicitly does that cause problems elsewhere?

Steve

sdlime commented on bcee559 Apr 11, 2013

Cellsize should be 0 plus minx=maxx and miny=maxy coming out of this function. If set explicitly does that cause problems elsewhere?

Steve

@dmorissette

This comment has been minimized.

Show comment
Hide comment
@dmorissette

dmorissette Apr 11, 2013

Contributor

I don't think cellsize should be 0 as this is a WMS request in which minx != maxx, so it does have a cellsize that could be computed from the original WMS BBOX before switching to the MapServer coordinate model as @mkofahl pointed out. Could we not force the return value to be the cellsize computed from the WMS BBOX?

Contributor

dmorissette commented Apr 11, 2013

I don't think cellsize should be 0 as this is a WMS request in which minx != maxx, so it does have a cellsize that could be computed from the original WMS BBOX before switching to the MapServer coordinate model as @mkofahl pointed out. Could we not force the return value to be the cellsize computed from the WMS BBOX?

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Apr 11, 2013

Member

The problem is with this extent model (borrowed from ERDAS back in the day), a cellsize of 0 is correct. If the goal is to get an response then I"m curious what happens with cellsize 0.


From: Daniel Morissette [notifications@github.com]
Sent: Thursday, April 11, 2013 8:22 AM
To: mapserver/mapserver
Cc: Lime, Steve D (MNIT)
Subject: Re: [mapserver] Bugfix for wms requests with width or height of one (#4629)

I don't think cellsize should be 0 as this is a WMS request in which minx != maxx, so it does have a cellsize that could be computed from the original WMS BBOX before switching to the MapServer coordinate model. Could we not force the return value to be the cellsize computed from the WMS BBOX?


Reply to this email directly or view it on GitHubhttps://github.com//pull/4629#issuecomment-16233820.

Member

sdlime commented Apr 11, 2013

The problem is with this extent model (borrowed from ERDAS back in the day), a cellsize of 0 is correct. If the goal is to get an response then I"m curious what happens with cellsize 0.


From: Daniel Morissette [notifications@github.com]
Sent: Thursday, April 11, 2013 8:22 AM
To: mapserver/mapserver
Cc: Lime, Steve D (MNIT)
Subject: Re: [mapserver] Bugfix for wms requests with width or height of one (#4629)

I don't think cellsize should be 0 as this is a WMS request in which minx != maxx, so it does have a cellsize that could be computed from the original WMS BBOX before switching to the MapServer coordinate model. Could we not force the return value to be the cellsize computed from the WMS BBOX?


Reply to this email directly or view it on GitHubhttps://github.com//pull/4629#issuecomment-16233820.

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Apr 11, 2013

Member

I don't see how cellsize=0 along with minx=maxx and miny=maxy can be anything other than undefined, as you'd be going through the same codepaths if your request is WIDTH=1&HEIGHT=1&BBOX=-180,-90,180,90 and WIDTH=1&HEIGHT=1&BBOX=-1,-1,1,1

Member

tbonfort commented Apr 11, 2013

I don't see how cellsize=0 along with minx=maxx and miny=maxy can be anything other than undefined, as you'd be going through the same codepaths if your request is WIDTH=1&HEIGHT=1&BBOX=-180,-90,180,90 and WIDTH=1&HEIGHT=1&BBOX=-1,-1,1,1

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Apr 11, 2013

Member

stepping through the code...
the failing step may actually be at https://github.com/mapserver/mapserver/blob/master/mapwms.c#L1684 , where the extent is set such that minx=maxx and miny=maxy. From then on it is impossible to compute a valid scale or cellsize.

Member

tbonfort commented Apr 11, 2013

stepping through the code...
the failing step may actually be at https://github.com/mapserver/mapserver/blob/master/mapwms.c#L1684 , where the extent is set such that minx=maxx and miny=maxy. From then on it is impossible to compute a valid scale or cellsize.

tbonfort added a commit to tbonfort/mapserver that referenced this pull request Apr 11, 2013

@mkofahl

This comment has been minimized.

Show comment
Hide comment
@mkofahl

mkofahl Apr 12, 2013

Contributor

#4632 looks fine to me regarding the reported error.

Contributor

mkofahl commented Apr 12, 2013

#4632 looks fine to me regarding the reported error.

@dmorissette

This comment has been minimized.

Show comment
Hide comment
@dmorissette

dmorissette Apr 12, 2013

Contributor

I didn't test #4632 but it looks like a better fix at first sight. Thank you @tbonfort

Contributor

dmorissette commented Apr 12, 2013

I didn't test #4632 but it looks like a better fix at first sight. Thank you @tbonfort

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Apr 13, 2013

Member

applied to 6-2 and master in 3d37bd8

Member

tbonfort commented Apr 13, 2013

applied to 6-2 and master in 3d37bd8

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