Bug 690444

The command line -r option works because (as documented) it locks in the
resolution and ignores attempts to change it.

I dunno, I drag giant windows around sometimes. Depending on the system anyway,
some servers/window managers seem to clip to the display size.

What would you like us to do? The x11 device is quite primitive; applications
like gv pass in a target size.

[Apologies for the delay in replying -JimC]

> What would you like us to do?

Not blow up the box. ☺

I’d be happy were not specifying -r at all to be the same in every respect as
specifying it with its default value (100, I believe), just like what gs used to do.

IOW, I suppose, ignore /HWResolution setpagedevice calls when using GUI terminals.

AFAIK, Ghostscript has always honored the HWResolution option for all devices.

That is what is documented.

If a spcific device has a limitation, it can examine the parameters and reject
the 'put_params' call. For instance, some devices that have a max length or
width will do this (e.g. 32767 limit for some file formats).

If there is a way to determine the maximum page size (due to either the PageSize
or HWResolution) for x11 or x11alpha devices, then the device can handle it.

The x11 devices never used to change their window size once it had opened until
*very* recently.  I want to say 8.64 was the first version (it was the first I
saw it in), but I’m not sure whether I ever used 8.63 to read a document which
used setpagedevice.  Probably, but I cannot be certain.

Certainly 7.x kept a static window size.  And, I think, 8.5x.

The fact that it now does change its window size — including to sizes so large
as to crash the whole box — is the regression.

Print resolutions should simply not get used by display devices.

> If there is a way to determine the maximum page size (due to either the
> PageSize or HWResolution) for x11 or x11alpha devices, then the device
> can handle it.

X supports requests for the pixel and mm dimensions of each screen and the root
window.  Plus modern window managers configure the _WIN_WORKAREA and
_NET_WORKAREA properties on the root window to specify the area available to apps.

I find that the current default of 100dpi windows works well, though it wouldn’t
hurt to be able to set the default resolution via xrdb, so that it could vary
per server.

Posting a bounty for a patch (which Ralph will review) for the methods
described in comment #5 (using _WIN_WORKAREA and/or _NET_WORKAREA and the
xrdb value for default resolution) -- or some better idea that anyone that wants
to work on this has.

Created attachment 5329 [details]
bug690444.patch

Proposed patch to fix #690444.

Sorry if context diff is a wrong format for Ghostscript patches, I'm new to this
subject and couldn't find any info about suggested patch style in Ghostscript
docs...

BTW resolution setting for x11 driver via xrdb is already implemented (resources
"xResolution" and "yResoltion" as documented in Use.htm) if I not mistaking...

Thanks! We generally prefer unified diff, but I'll cope.

Created attachment 5738 [details]
same patch, in unified format

converting the posted patch to unified diff for review purpose.

(I just patch it in to a local git-maintained repository and run git diff to
spit it out; can be achieve with and svn checkout and svn diff as well) 

There is a compiler warning at line 1009 of base/gdevxini.c for dw and dh being
used unintialized - indeed this seems to be an error in the patch: the
submitted patch removed the needed dw, dh initialization which is needed later.


Suggest the poster fix this and re-submit.

The patch poster didn't add himself to CC:.

See comment above and suggest the poster fix the problem and re-submit.

Created attachment 5743 [details]
bug690444.patch

Sorry for that flaw.
Fixed.

Grabbing a Ralph's bugs.

And I can confirm that testing on a 64bit f11 box using ghostscript-8.71-6.fc11.x86_64 also succeeds for me.

Further testing on the laptop confirms that stack size was the issue.  When I raised the stack size from the default of 8192 to 10240 (matching my default on that 64 bit server box) the file worked with non-x11 devices.

It still crashes, though, with any of the x11 devices, even with a stacksize of 65536, eight times the default.

But not with freetype, even with large values of -r, such as -r500.

SO it is probably not worth further investigation, given that fapi is the way forward.

But if you would like to see one of the core files, I can post one.

Comment #15 was meant for a different bug; please ignore.

(In reply to comment #13)
> Created an attachment (id=5743) [details]
> bug690444.patch

Apologies for taking so long to review again, and thanks for the continual work on this bug. A few more trivial and not-so-trivial comments:

- The XA_WIN_WORKAREA/XA_NET_WORKAREA are only used in x_get_work_area() in base/gdevxini.c so probably should not in base/gdevx.h . In fact I can't think of a reason for needing those defines, because using the strings directly ( x_get_win_property(..., "_WIN_WORKAREA")) probably works, and both of them are only used exactly once.

- I would probably initialize "int workarea_width, workarea_height;" explicitly to say, 32767/32767 ( a reasonably large/impossible screen size) as the combination of 0/0 and x_get_work_area() failing or getting moved in a future change, for them to be accidentally 0/0 due to other reasons is a bit unpleasant.

- why is "static long *x_get_win_property()" and not "static unsigned char *x_get_win_property()"? There is no need for the explicit cast in "return (long *)prop;" if that's the case. Looking this more carefully, it is because the long is eventually cast'ed to an int in '*pwidth = area[2]; *pheight = area[3];'

- r_type, r_format, count should be initialized to something that's not 
r_type == XA_CARDINAL , r_format == 32 , count == 4, and check for bytes_remain  to be zero on return may be nice.

- a few brackets around "dev->width = dev->width <= area_width ?..." would be nice.

- it appears that xsize/ysize are only calculated to be used in "xsize * xdev->x_pixels_per_inch", which could be simplified as just "dev->width".

(In reply to comment #17)
> - why is "static long *x_get_win_property()" and not "static unsigned char
> *x_get_win_property()"? There is no need for the explicit cast in "return (long
> *)prop;" if that's the case. Looking this more carefully, it is because the
> long is eventually cast'ed to an int in '*pwidth = area[2]; *pheight =
> area[3];'

Please ignore this part of the comment - I forgot to delete it as I wrote along, when I got to writing the next part about checking r_type/etc and realise the "long" is correct as long as all the r_type/etc are checked.

Created attachment 6440 [details]
Proposed patch

> - The XA_WIN_WORKAREA/XA_NET_WORKAREA are only used in x_get_work_area() in
> base/gdevxini.c so probably should not in base/gdevx.h . In fact I can't think
> of a reason for needing those defines, because using the strings directly (
> x_get_win_property(..., "_WIN_WORKAREA")) probably works, and both of them are
> only used exactly once.

Fixed.

> - I would probably initialize "int workarea_width, workarea_height;" explicitly
> to say, 32767/32767 ( a reasonably large/impossible screen size) as the
> combination of 0/0 and x_get_work_area() failing or getting moved in a future
> change, for them to be accidentally 0/0 due to other reasons is a bit
> unpleasant.

Current implementation of x_get_work_area can't fail leaving its output vars
uninitialized: they'll be assigned WidthOfScreen and HeightOfScreen of
xdev->scr respectively.
Future changes may of course rearrange the things, so if you insists on inline
initialization in calling functions, here you are. :)

> - r_type, r_format, count should be initialized to something that's not 
> r_type == XA_CARDINAL , r_format == 32 , count == 4, and check for bytes_remain
>  to be zero on return may be nice.

Fixed.

> - a few brackets around "dev->width = dev->width <= area_width ?..." would be
> nice.

Entire expression has been simplified.

> - it appears that xsize/ysize are only calculated to be used in "xsize *
> xdev->x_pixels_per_inch", which could be simplified as just "dev->width".

Fixed (in gdev_x_put_params).

I have tested the patch and it does what it claims to do with attachment #4954 [details] from bug #690435 . This is a useful change to have and I hope this makes the next release.

I'll pass it along for somebody else to review or commit.

Boris, thank you very much for fixing this annoying bug. For many users it is irrelevant, but for me as printing workflow developer it is very useful for debugging.

I have tested it and it works for me and Hin-Tak has reviewed it as a Ghostscript expert. So I committed it now as rev. 11556.