Skip to content

Conversation

@joa-quim
Copy link
Member

It has always annoyed me (e., g. #1435) that for running psconvert (and before, ps2raster) we must create a temporary EPS file that is the one we sent through the Ghostscript guts to get our raster file (I hate solutions based on temporary disk files). No big deal if the PS file is small, but far from ideal if it's a hundreds MB file.

This PR adds an option to psconvert that avoids that by editing the original PS turning it into what before we obtained with EPS temporary file. Due to shortage of letters, excluding the global options we have no more alphabet letters available, that option was made -!, which inspired in the Julia convention that bang (!) symbol is used for data modifying functions.

I have also added run time reports via the -Vt option and, a bit disappointingly, it takes big PS files to show small run times differences between master and this PR (I do have a fast SSD disk, is it it?).

Run the tests with an hard-coded -! so that also the modern mode scripts use it and saw no obvious failures, but certainly not all psconvert paths were tested. So I would like to ask for more testing on this. If it proves reliable, the idea is to make this PR the default behavior and add some flag as a modifier of an existing option to keep the ancient behavior.

PLEASE TEST

@joa-quim joa-quim requested a review from a team March 27, 2024 15:34
@anbj
Copy link
Contributor

anbj commented Mar 27, 2024

May I ask why things are done this way? (Doing the EPS-stuff, instead of using the PS, as is the goal here)

@joa-quim
Copy link
Member Author

There are things I don't know. Namely, why GMT PS have a setpagedevice (is it needed?) and why the EPS should not have those (but that's what Artifex people say). But one thing is simple to explain. Given that PMT PSs are made with layers, there is no way that GMT can know the final figure BoundingBox, and without that knowledge we cannot do a correct -A. So we have to find it by asking the Ghost to calculate it and after insert the right numbers plus offsets to put the figure origin at (0,0). And there is the GeoPDF stuff, transparencies etc. Not all of this is possible to do by editing the original file and not changing its size by a single byte. In those cases the new -! option is deactivated.

@anbj
Copy link
Contributor

anbj commented Mar 27, 2024

Thanks @joa-quim.

@seisman
Copy link
Member

seisman commented Mar 28, 2024

Not sure if I'm using it correctly, but it crashes for the following command:

$ gmt --version
6.6.0_f571a71_2024.03.27
$ gmt psbasemap -R0/10/0/10 -JM10c -Baf > map.ps
$ gmt psconvert -A -P -Tj -! map.ps
ERROR: Caught signal number 11 (Segmentation fault) at
/lib64/libc.so.6(+0x61b21)[0x7fd9ce87fb21]
[0xc0]
Stack backtrace:
/home/seisman/opt/GMT-master/lib64/libgmt.so.6(gmt_sig_handler_unix+0xbf)[0x7fd9d2f582ef]
/lib64/libc.so.6(+0x3e9a0)[0x7fd9ce85c9a0]
/lib64/libc.so.6(+0x61b21)[0x7fd9ce87fb21]
/lib64/libc.so.6(_IO_fprintf+0x9c)[0x7fd9ce87437c]
/home/seisman/opt/GMT-master/lib64/libgmt.so.6(GMT_psconvert+0x21c5)[0x7fd9d3199885]
/home/seisman/opt/GMT-master/lib64/libgmt.so.6(GMT_Call_Module+0x335)[0x7fd9d2e59595]
gmt[0x401734]
/lib64/libc.so.6(+0x2814a)[0x7fd9ce84614a]
/lib64/libc.so.6(__libc_start_main+0x8b)[0x7fd9ce84620b]
gmt[0x4024a5]

@joa-quim
Copy link
Member Author

Oh ghrrr. That works fine for me on Windows. Can you try it with a debug build. Maybe the stack trace will be more informative.

@seisman
Copy link
Member

seisman commented Mar 28, 2024

Can you try it with a debug build. Maybe the stack trace will be more informative.

Just tried the debug build. The stack trace is the same.

One question: even if it doesn't crash, does it mean that the original PS file on the disk will be broken since it's edited when calling psconvert? Or the editing is done to the PS file in memory?

@joa-quim
Copy link
Member Author

The -! should not break the original PS file but changes it to become equal to what we get with -Te. No in-memory processing though that is already possible when called from Matlab & Julia but I have not tried this in long time.

GMT_LOCAL int psconvert_pipe_HR_BB(struct GMTAPI_CTRL *API, struct PSCONVERT_CTRL *Ctrl, char *gs_BB, double margin, double *w, double *h) {
	/* Do what we do in the main code for the -A (if used here) option but on an in-memory PS 'file' */

One idea is that would be useful to the wrappers that don't really care about the original PS.

@joa-quim
Copy link
Member Author

Can you check the beginning of the map.ps file and see if any of the BoungingBoxes have been changed?

Original

%%BoundingBox: 0 0 595 842
%%HiResBoundingBox: 0 0 595.0000 842.0000  

After change

%%BoundingBox: 0 0 321 338
%%%HiResBoundingBox: 0 0 337.9680 320.7420  

@seisman
Copy link
Member

seisman commented Mar 28, 2024

There are no changes in the PS file.

@joa-quim
Copy link
Member Author

So it must have crashed on lines 2366 or 2368

if (Ctrl->O.active) {
	fseek(fp, (off_t)-strlen(line), SEEK_CUR);	/* Seek back to start of line */
	if (got_BB && !Ctrl->A.round)
		sprintf(line, "%%%%BoundingBox: 0 0 %ld %ld", lrint (psconvert_smart_ceil(w_t)), lrint (psconvert_smart_ceil(h_t)));

I'm opening the PS file with fp = fopen (ps_file, "r+"). Could it be the problem for unix?

@seisman
Copy link
Member

seisman commented Mar 28, 2024

So it must have crashed on lines 2366 or 2368

if (Ctrl->O.active) {
	fseek(fp, (off_t)-strlen(line), SEEK_CUR);	/* Seek back to start of line */
	if (got_BB && !Ctrl->A.round)
		sprintf(line, "%%%%BoundingBox: 0 0 %ld %ld", lrint (psconvert_smart_ceil(w_t)), lrint (psconvert_smart_ceil(h_t)));

I'm opening the PS file with fp = fopen (ps_file, "r+"). Could it be the problem for unix?

r+ should work on Unix (https://man7.org/linux/man-pages/man3/fopen.3.html).

I tried to add a print statement in the while (psconvert_file_line_reader (GMT, &line, &line_size, fp) != EOF && n_read_PS_lines < max_PS_lines) loop. It seems only the first line %!PS-Adobe-3.0 is read and then it crashes.

@joa-quim
Copy link
Member Author

joa-quim commented Apr 1, 2024

Please try again. It was a dumb error in the null file name.

@seisman
Copy link
Member

seisman commented Apr 2, 2024

Please try again. It was a dumb error in the null file name.

Yes, now it works on Linux, but as I asked before, now I can confirm that the original PS file is actually a EPS file after editing.

I'm wondering can we just read the PS file and send it to Ghostscript via stdin, like https://stackoverflow.com/questions/70706765/passing-stdin-to-ghostscript-in-php-proc-open.

@anbj
Copy link
Contributor

anbj commented Apr 2, 2024

How to test this? Just running a bunch of scripts?

@joa-quim
Copy link
Member Author

joa-quim commented Apr 2, 2024

How to test this? Just running a bunch of scripts?

Well, yes but what I did was to hardwire the -! option by adding a line (~line 1735 of psconvert.c)

Ctrl->O.active = true;

and run all the tests. But even all the tests do not probably test everything.

@joa-quim
Copy link
Member Author

joa-quim commented Apr 2, 2024

I'm wondering can we just read the PS file and send it to Ghostscript via stdin

The psconvert_pipe_ghost() function on psconvert tries to do that but for some reason it only works on Windows and is slow. Note that the interest here is to be able to return the raster image directly in memory. That was made for the Matlab wrapper (an later for GMT.jl) using the fact that we can return the PS data in a memory address and not writing it on disk. On CLI that is not possible so sending in by stdin doesn't look better than what we do (a system call).

The still-in-the-air idea is to one day be able to call the Ghostscript API directly to do these operations. This would free us from the system calls and popen/popen2 troubles.

@anbj
Copy link
Contributor

anbj commented Apr 2, 2024

The still-in-the-air idea is to one day be able to call the Ghostscript API directly to do these operations. This would free us from the system calls and popen/popen2 troubles.

And not using popen/popen2 have what benefits?

@joa-quim
Copy link
Member Author

joa-quim commented Apr 2, 2024

The popen/popen2 functions we have for some mysterious reasons do not work on unix and are slow on Windows.

@joa-quim joa-quim mentioned this pull request May 28, 2025
51 tasks
@joa-quim joa-quim changed the title WIP: Add option to not create a temporary EPS file but instead edit the original PS. Add option to not create a temporary EPS file but instead edit the original PS. Jun 2, 2025
@joa-quim
Copy link
Member Author

joa-quim commented Jun 4, 2025

This can now also crop a pure PS file, but only in classic mode, and needs that the original PS has the wall paper size (32767x32767). Because we are creating another PS from an input PS, the output, when file name was not set via -F, will be named by appending a _2 to the input name. e.g. lixo.ps will become lixo_2.ps. The other option is to use the -! option, and than input file is cropped.

Tried to extend this to modern mode but I lack a syntax for that. Tried to introduce a small breakage of creating a wall sized PS by default, instead of A4 bit it breaks all modern mode tests.

@joa-quim
Copy link
Member Author

joa-quim commented Jun 8, 2025

Ping.

I would like to merge this branch. All tests are not affected and recall that without the -! explicitly set this PR should behave as before.

@joa-quim joa-quim requested a review from a team June 8, 2025 12:17
@joa-quim
Copy link
Member Author

Ping

@anbj
Copy link
Contributor

anbj commented Jun 11, 2025

How can I test this, if necessary?

@joa-quim
Copy link
Member Author

If built with -DPS_NO_DUP all calls to psconvert will use the new feature. But for some unknown reason (high concurrency in processing may files?) tests run considerably slower. Not recommended as a permanent setting. Otherwise use the option -! to test it on a one-by-one case. But above all, do nothing specific as then this branch should then behave like master.

@anbj
Copy link
Contributor

anbj commented Jun 12, 2025

Ok, so should I just approve? 😄

@joa-quim
Copy link
Member Author

O would like also a confirmation that the pygmt tests are not affected, but if that doesn’t come …

@anbj
Copy link
Contributor

anbj commented Jun 12, 2025

Alright, I can't help with pythonstuff. Let's wait some more..

@joa-quim
Copy link
Member Author

@seisman

@seisman
Copy link
Member

seisman commented Jun 13, 2025

Generally speaking, this PR is good to me.

When I run the following test, the jpeg version is good, but for the PDF output, it's not perfectly cropped (there are some extra spaces at the top).

$ gmt psbasemap -R0/10/0/10 -JM10c -Baf > map.ps
$ gmt psconvert -A -P -Tj -! map.ps
$ gmt psconvert -A -P -Tf -! map.ps

@joa-quim
Copy link
Member Author

Thanks, I will want to fix this somehow. The issue here is that the second time one runs the psconvert command, it runs over the modified PS version created by first run. That should not modified it no further but apparently it does.

…no subsequent.

Problem was that dimension were modified to Portrait but Orientation in header remained unchanged saying it was Landscape.
@joa-quim
Copy link
Member Author

OK, fixed that one too. It was an issue with Landscape files that were left in a contradictory state after first run.

@joa-quim joa-quim merged commit 4904aff into master Jun 16, 2025
12 of 13 checks passed
@joa-quim joa-quim deleted the psconvert-no-file-dup branch June 16, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants