Bug 76450

The issue reported there seems to be also what makes evince fails to render some eps files on e.g recent Ubuntu versions

That has been reported on
https://bugzilla.gnome.org/show_bug.cgi?id=710957
https://bugs.launchpad.net/libspectre/+bug/1242678
http://bugs.ghostscript.com/show_bug.cgi?id=694979

The ghostscript bugs has details

"...
Basically, this isn't a bug. If you want to rotate an EPS you absolutely should NOT be doing so by using /Orientation, which is a media selection parameter. You should instead use '90 rotate' or similar to set up the CTM before you render the EPS."

@Marek: I tried your patch on some of the files from the Ubuntu bug, e.g https://bugs.launchpad.net/libspectre/+bug/1242678/+attachment/3886169/+files/test.eps ... it fixes the "nothing is displayed" but the orientation seems wrong (rotated from 90° compared to gs and 180° compared to what portrait should be (e.g axis/labels are wrong)). Not sure if that's a bug in the documents or in the change though

(In reply to comment #1)
> The issue reported there seems to be also what makes evince fails to render
> some eps files on e.g recent Ubuntu versions
> 
> That has been reported on
> https://bugzilla.gnome.org/show_bug.cgi?id=710957
> https://bugs.launchpad.net/libspectre/+bug/1242678
> http://bugs.ghostscript.com/show_bug.cgi?id=694979
> 
> The ghostscript bugs has details
> 
> "...
> Basically, this isn't a bug. If you want to rotate an EPS you absolutely
> should NOT be doing so by using /Orientation, which is a media selection
> parameter. You should instead use '90 rotate' or similar to set up the CTM
> before you render the EPS."
> 
> @Marek: I tried your patch on some of the files from the Ubuntu bug, e.g
> https://bugs.launchpad.net/libspectre/+bug/1242678/+attachment/3886169/
> +files/test.eps ... it fixes the "nothing is displayed" but the orientation
> seems wrong (rotated from 90° compared to gs and 180° compared to what
> portrait should be (e.g axis/labels are wrong)). Not sure if that's a bug in
> the documents or in the change though

I think that the problem is in evince. It sets wrong angle for rendering with spectre_render_context_set_rotation() from my point of view. evince rotates to left by setting new angle to "original_angle - 90". If you reverse the value passed by evince to libspectre (360 - angle) then it works as expected.

Seb, can you do an appropriate patch on evince and report a bug upstream?

Seb, you could use https://bugzilla.gnome.org/show_bug.cgi?id=710957 for this.

@Till, yes using https://bugzilla.gnome.org/show_bug.cgi?id=710957 seems fine to me.

@Marek, do you plan to follow up with the evince team about that rotation issue? 
It seems like both libspectre and evince need to be updated together to give a correct result there

@Carlos, what do you think about those changes?

(In reply to comment #5)
> @Marek, do you plan to follow up with the evince team about that rotation
> issue? 
> It seems like both libspectre and evince need to be updated together to give
> a correct result there

Sure, I can continue on this in evince too. I'll try look for a solution.

Comment on attachment 96165 [details] [review]
Rotate documents correctly

Review of attachment 96165 [details] [review]:
-----------------------------------------------------------------

Thanks for the patch!

::: libspectre/spectre-device.c
@@ +206,5 @@
>  		return SPECTRE_STATUS_RENDER_ERROR;
>  	}
>  
> +	scaled_width = (int) ((width * rc->x_scale) + 0.5);
> +	scaled_height = (int) ((height * rc->y_scale) + 0.5);

I think it would be clearer if we set these values depending on the orientation, and then we just use scale_width, scaled_height below.

@@ +226,5 @@
>  							   rc->text_alpha_bits);
>  	args[arg++] = graph_alpha = _spectre_strdup_printf ("-dGraphicsAlphaBits=%d",
>  							    rc->graphic_alpha_bits);
> +
> +	if (rc->orientation == 0 || rc->orientation == 2) {

Don't use magic numbers here, use NONE and LANDSCAPE instead.

@@ +232,5 @@
> +		args[arg++] = resolution = _spectre_strdup_printf ("-r%fx%f",
> +								   rc->x_scale * rc->x_dpi,
> +								   rc->y_scale * rc->y_dpi);
> +	}
> +	else {

} else {

::: libspectre/spectre-gs.c
@@ +237,5 @@
>  
> +	switch (rotation) {
> +		default:
> +			tmp_xoffset = xoffset + x;
> +			tmp_yoffset = yoffset + y;

I don't think we need the tmp_ variables, we can just modify the existing xoffset/yoffset.

@@ +277,5 @@
>  				 doc->endsetup))
>  		return FALSE;
>  
> +	if (rotation != 0) {
> +		set = _spectre_strdup_printf ("%d rotate", rotation);

Where does this end up? After the setup and before the pages? Isn't it recommended to apply the rotation after the translation? I think we could move this to spectre_gs_process, after the translate. We pass the rotation to spectre_gs_process and when it's != NONE we inject the rotate command there. What do you think?

Created attachment 99100 [details] [review]
Rotate documents correctly

Thank you for the review.

(In reply to comment #7)
> Comment on attachment 96165 [details] [review] [review]
> Rotate documents correctly
> 
> Review of attachment 96165 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch!
> 
> ::: libspectre/spectre-device.c
> @@ +206,5 @@
> >  		return SPECTRE_STATUS_RENDER_ERROR;
> >  	}
> >  
> > +	scaled_width = (int) ((width * rc->x_scale) + 0.5);
> > +	scaled_height = (int) ((height * rc->y_scale) + 0.5);
> 
> I think it would be clearer if we set these values depending on the
> orientation, and then we just use scale_width, scaled_height below.

Done.


> @@ +226,5 @@
> >  							   rc->text_alpha_bits);
> >  	args[arg++] = graph_alpha = _spectre_strdup_printf ("-dGraphicsAlphaBits=%d",
> >  							    rc->graphic_alpha_bits);
> > +
> > +	if (rc->orientation == 0 || rc->orientation == 2) {
> 
> Don't use magic numbers here, use NONE and LANDSCAPE instead.

Done.


> @@ +232,5 @@
> > +		args[arg++] = resolution = _spectre_strdup_printf ("-r%fx%f",
> > +								   rc->x_scale * rc->x_dpi,
> > +								   rc->y_scale * rc->y_dpi);
> > +	}
> > +	else {
> 
> } else {

Done.


> ::: libspectre/spectre-gs.c
> @@ +237,5 @@
> >  
> > +	switch (rotation) {
> > +		default:
> > +			tmp_xoffset = xoffset + x;
> > +			tmp_yoffset = yoffset + y;
> 
> I don't think we need the tmp_ variables, we can just modify the existing
> xoffset/yoffset.

I removed them but I had to reintroduce them back because of the next item.


> @@ +277,5 @@
> >  				 doc->endsetup))
> >  		return FALSE;
> >  
> > +	if (rotation != 0) {
> > +		set = _spectre_strdup_printf ("%d rotate", rotation);
> 
> Where does this end up? After the setup and before the pages?

Yes, it ends up there.


> Isn't it recommended to apply the rotation after the translation? I think we could
> move this to spectre_gs_process, after the translate. We pass the rotation
> to spectre_gs_process and when it's != NONE we inject the rotate command
> there. What do you think?

I moved it there.


Unfortunately I've found a PostScript file on which the patch doesn't work. It shows the file with the original rotation for each rotation. It is due to "setpagedevice" and "PageSize" are employed there (see next attachment). I'm not sure what to do with it yet.

Regards

Marek

Created attachment 99101 [details]
The problematic PostScript file.

Created attachment 99156 [details] [review]
Rotate documents correctly

This patch solves the problem with the problematic PostScript file for me but it is kind of a hack. It replaces occurrences of "setpagedevice" with "pop          " so that its parameters don't influence the rendering to the "display" device. I have looked into PostScript LANGUAGE REFERENCE and it seems to me that the parameters which are set by "setpagedevice" are not important for rendering to "display" device (but I'm not really sure here).

Marek

I'm not a PostScript expert either.

Yes, you can replace setpagedevice with pop. It is not clear to me how your patch is doing this. Something like "/setpagedevice { pop } bind def" should work.

(In reply to comment #12)
> Yes, you can replace setpagedevice with pop. It is not clear to me how your
> patch is doing this. Something like "/setpagedevice { pop } bind def" should
> work.

Thanks Adrian. It seems Marek submitted the same patch again instead of the updated one.

Created attachment 99331 [details] [review]
Rotate documents correctly

(In reply to comment #13)
> (In reply to comment #12)
> > Yes, you can replace setpagedevice with pop. It is not clear to me how your
> > patch is doing this. Something like "/setpagedevice { pop } bind def" should
> > work.

Thank you.

> It seems Marek submitted the same patch again instead of the updated one.

You are right, I somehow selected wrong file, I'm sorry. The attached patch is the right one with the hint from Adrian incorporated (I also tried to pass it as a "-c" option to gs but it didn't work).

Comment on attachment 99331 [details] [review]
Rotate documents correctly

Review of attachment 99331 [details] [review]:
-----------------------------------------------------------------

This looks good to me, except the confusing NONE/LANDSCAPE thing. Adrian, does the PostScript part looks good to you?

::: libspectre/spectre-device.c
@@ +206,4 @@
>  		return SPECTRE_STATUS_RENDER_ERROR;
>  	}
>  
> +	if (rc->orientation == NONE || rc->orientation == LANDSCAPE) {

Why NONE and LANDSCAPE? shouldn't we invert width/height when orientation is LANDSCAPE and SEASCAPE? Ah, I know what's going on, you are using the internal values of the parser (my fault, I think I suggested it) but rc->orientation is actually a SpectreOrientation enum value, so you should use SPECTRE_ORIENTATION_PORTRAIT || SPECTRE_ORIENTATION_REVERSE_PORTRAIT that are 0 and 2 like NONE and LANDSCAPE in the internal parser.

@@ +269,4 @@
>  		args[arg++] = "-dNOPLATFONTS";
>  
>  	if (rc->width != -1 && rc->height != -1) {
> +		if (rc->orientation == NONE || rc->orientation == LANDSCAPE) {

Same here.

(In reply to comment #15)
> Adrian, does the PostScript part looks good to you?

Looks good.

Created attachment 100723 [details] [review]
Rotate documents correctly

Hi,

I'm sorry for my late response, I was super busy with something else.

(In reply to comment #15)
> Comment on attachment 99331 [details] [review] [review]
> Rotate documents correctly
> 
> Review of attachment 99331 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, except the confusing NONE/LANDSCAPE thing. Adrian,
> does the PostScript part looks good to you?
> 
> ::: libspectre/spectre-device.c
> @@ +206,4 @@
> >  		return SPECTRE_STATUS_RENDER_ERROR;
> >  	}
> >  
> > +	if (rc->orientation == NONE || rc->orientation == LANDSCAPE) {
> 
> Why NONE and LANDSCAPE? shouldn't we invert width/height when orientation is
> LANDSCAPE and SEASCAPE? Ah, I know what's going on, you are using the
> internal values of the parser (my fault, I think I suggested it) but
> rc->orientation is actually a SpectreOrientation enum value, so you should
> use SPECTRE_ORIENTATION_PORTRAIT || SPECTRE_ORIENTATION_REVERSE_PORTRAIT
> that are 0 and 2 like NONE and LANDSCAPE in the internal parser.
> 
> @@ +269,4 @@
> >  		args[arg++] = "-dNOPLATFONTS";
> >  
> >  	if (rc->width != -1 && rc->height != -1) {
> > +		if (rc->orientation == NONE || rc->orientation == LANDSCAPE) {
> 
> Same here.

My brain was probably on a vacation :). I've fixed this so that it uses the SPECTRE_ORIENTATION_PORTRAIT and SPECTRE_ORIENTATION_REVERSE_PORTRAIT.

Regards

Marek

(In reply to comment #17)
> Created attachment 100723 [details] [review] [review]
> Rotate documents correctly
> 
> Hi,
> 
> I'm sorry for my late response, I was super busy with something else.

No problem!

> (In reply to comment #15)
> > Comment on attachment 99331 [details] [review] [review] [review]
> > Rotate documents correctly
> > 
> > Review of attachment 99331 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good to me, except the confusing NONE/LANDSCAPE thing. Adrian,
> > does the PostScript part looks good to you?
> > 
> > ::: libspectre/spectre-device.c
> > @@ +206,4 @@
> > >  		return SPECTRE_STATUS_RENDER_ERROR;
> > >  	}
> > >  
> > > +	if (rc->orientation == NONE || rc->orientation == LANDSCAPE) {
> > 
> > Why NONE and LANDSCAPE? shouldn't we invert width/height when orientation is
> > LANDSCAPE and SEASCAPE? Ah, I know what's going on, you are using the
> > internal values of the parser (my fault, I think I suggested it) but
> > rc->orientation is actually a SpectreOrientation enum value, so you should
> > use SPECTRE_ORIENTATION_PORTRAIT || SPECTRE_ORIENTATION_REVERSE_PORTRAIT
> > that are 0 and 2 like NONE and LANDSCAPE in the internal parser.
> > 
> > @@ +269,4 @@
> > >  		args[arg++] = "-dNOPLATFONTS";
> > >  
> > >  	if (rc->width != -1 && rc->height != -1) {
> > > +		if (rc->orientation == NONE || rc->orientation == LANDSCAPE) {
> > 
> > Same here.
> 
> My brain was probably on a vacation :). I've fixed this so that it uses the
> SPECTRE_ORIENTATION_PORTRAIT and SPECTRE_ORIENTATION_REVERSE_PORTRAIT.

:-) 

> Regards
> 
> Marek

Pushed to git master, thanks!

Thanks Marek! Do you still plan to look at the evince side as well (since the rotation seems to still be wrong when using it, as mentioned in previous comments)

(In reply to comment #19)
> Thanks Marek! Do you still plan to look at the evince side as well (since
> the rotation seems to still be wrong when using it, as mentioned in previous
> comments)

Hi Sebastien,

thank you for reminding me this. I'll look at that but not now since I have several tasks I have to finish before I can jump on this.
Maybe at the end of the next week.

Regards

Marek

I've been trying this and I'm getting the opposite results, the attached ps file is rotated without the patch but nothing is rendered when rotated with the patch.

(In reply to comment #20)
> (In reply to comment #19)
> > Thanks Marek! Do you still plan to look at the evince side as well (since
> > the rotation seems to still be wrong when using it, as mentioned in previous
> > comments)
> 
> Hi Sebastien,
> 
> thank you for reminding me this. I'll look at that but not now since I have
> several tasks I have to finish before I can jump on this.
> Maybe at the end of the next week.

I've filled a bug for this here: https://bugzilla.gnome.org/show_bug.cgi?id=731786. A patch is attached there which solves it for me.


(In reply to comment #21)
> I've been trying this and I'm getting the opposite results, the attached ps
> file is rotated without the patch but nothing is rendered when rotated with
> the patch.

I've just tried it with evince from master with patched libspectre and it works for me as expected :(.


Marek

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Thanks Marek! Do you still plan to look at the evince side as well (since
> > > the rotation seems to still be wrong when using it, as mentioned in previous
> > > comments)
> > 
> > Hi Sebastien,
> > 
> > thank you for reminding me this. I'll look at that but not now since I have
> > several tasks I have to finish before I can jump on this.
> > Maybe at the end of the next week.
> 
> I've filled a bug for this here:
> https://bugzilla.gnome.org/show_bug.cgi?id=731786. A patch is attached there
> which solves it for me.
> 
> 
> (In reply to comment #21)
> > I've been trying this and I'm getting the opposite results, the attached ps
> > file is rotated without the patch but nothing is rendered when rotated with
> > the patch.
> 
> I've just tried it with evince from master with patched libspectre and it
> works for me as expected :(.

Maybe it has to do with the gs version?

$ gs --version
9.05

From Debian testing.

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > Thanks Marek! Do you still plan to look at the evince side as well (since
> > > > the rotation seems to still be wrong when using it, as mentioned in previous
> > > > comments)
> > > 
> > > Hi Sebastien,
> > > 
> > > thank you for reminding me this. I'll look at that but not now since I have
> > > several tasks I have to finish before I can jump on this.
> > > Maybe at the end of the next week.
> > 
> > I've filled a bug for this here:
> > https://bugzilla.gnome.org/show_bug.cgi?id=731786. A patch is attached there
> > which solves it for me.
> > 
> > 
> > (In reply to comment #21)
> > > I've been trying this and I'm getting the opposite results, the attached ps
> > > file is rotated without the patch but nothing is rendered when rotated with
> > > the patch.
> > 
> > I've just tried it with evince from master with patched libspectre and it
> > works for me as expected :(.
> 
> Maybe it has to do with the gs version?
> 
> $ gs --version
> 9.05
> 
> From Debian testing.

I've just tried it with the ghostscript 9.05 and the documents are rotated in opposite direction compared to the libspectre without the patch but it renders the documents correctly otherwise.
I'll prepare a patch which will change the direction of rotation.

Created attachment 101589 [details] [review]
Change direction of rotation

The promised patch.

Could somebody review the new patch there?

(In reply to comment #25)
> Created attachment 101589 [details] [review] [review]
> Change direction of rotation
> 
> The promised patch.

I'm not sure this should be done here or in evince. Libspectre should do what the PS spec says, if evince wants the opposite, evince should do the conversion. Or am I misunderstanding the patch?

(In reply to comment #27)
> (In reply to comment #25)
> > Created attachment 101589 [details] [review] [review] [review]
> > Change direction of rotation
> > 
> > The promised patch.
> 
> I'm not sure this should be done here or in evince. Libspectre should do
> what the PS spec says, if evince wants the opposite, evince should do the
> conversion. Or am I misunderstanding the patch?

in any case we should document the expected behaviour in the documentation of spectre_render_context_get/set_rotation

hum, https://bugs.launchpad.net/bugs/1348384 is a new bug that claims that the change that got commited to git creates some bg issues on some documents... should that be a new bug report or discussed here as well?

we can discuss them here, if this patch has caused regressions we might revert it if we don't find a new solution for the regressions.

Created attachment 105606 [details] [review]
send setpagedevice

I've just bisected where the behaviour changes. I found that placing a setpagedevice to the place of where Orientation was originally set helps. But I still don't know why.

Created attachment 105607 [details]
minimal version of the reproducer

(In reply to comment #31)
> Created attachment 105606 [details] [review] [review]
> send setpagedevice
> 
> I've just bisected where the behaviour changes. I found that placing a
> setpagedevice to the place of where Orientation was originally set helps.
> But I still don't know why.

Maybe the page needs some kind of initialization which is performed in setpagedevice. I've just tried to place there just "erasepage" and it works too.

Still doesn't work here, it seems to rotate the first page only of a multipage document.

I've reverted the patch in master for now, until we find a proper solution.

(In reply to Carlos Garcia Campos from comment #35)
> I've reverted the patch in master for now, until we find a proper solution.

Do you know when a new tarball is planned including this and other fixes? Thanks

(In reply to Pacho Ramos from comment #36)
> (In reply to Carlos Garcia Campos from comment #35)
> > I've reverted the patch in master for now, until we find a proper solution.
> 
> Do you know when a new tarball is planned including this and other fixes?
> Thanks

No plans, but I can make a new release once this problem is fixed, for example.

(In reply to Carlos Garcia Campos from comment #34)
> Still doesn't work here, it seems to rotate the first page only of a
> multipage document.

I can not reproduce this. Could you point me to a document which triggers this problem?

Created attachment 111047 [details] [review]
erase page before its drawing

This patch is simplified version of the one with setpagedevice, it just erase the page instead of calling whole setpagedevice.

Hi,

the problem we are trying to fix here was introduced by this ghostscript commit: http://git.ghostscript.com/?p=ghostpdl.git;a=commit;h=7a8612112478196d8f047a556fdbb15d17aec7d0 (Add PSFitPage and FitPage options to allow fitting PS files to a page).
It is present in ghostscript since 9.08.

Created attachment 111925 [details] [review]
Rotate documents correctly

I've prepared a simple patch which also solves this problem. It just rotates what we get from ghostscript at pixel level. It takes more time than rendering it directly to the format we want but it works.
I'm just not sure about slice rendering. Should the result be slice from rotated page or rotated slice from page?

*** Bug 87588 has been marked as a duplicate of this bug. ***

could anyone review the suggested fix there?

Carlos, any hope of getting a review on Marek latest patch ?

Created attachment 120955 [details]
This files result eventually in a pixman crash

Hi,

this rotation issue seems to be also causing a crash in pixman in some cases:

  https://bugs.freedesktop.org/show_bug.cgi?id=87588

I am attaching a file which makes the crash reproducible here.

BTW another test-case for the invalid rotation is the output of kxbprint, reproducible like this:

  xkbprint :0.0 keyboard.ps
  evince keyboard.ps

The latter sometimes crashes and sometimes don't, at least on my system.

Ciao ciao,
   Antonio

As far as I remember the fix only worked for some versions of GS. Anyway, I'll try to find some time too look at this in detail again.

Comment on attachment 111925 [details] [review]
Rotate documents correctly

Review of attachment 111925 [details] [review]:
-----------------------------------------------------------------

Ok, I like this idea, it will be a bit slower, but it should work. However, I think we should only do this for libgs >= 9.08, so I've moved the rotation to a function and called it only for newer libgs versions. I've just pushed the patch to git master, please try it out and if everybody is happy now, I'll make a new release.

Nobody said anything, so I've released 0.2.8 with this patch, closing.

*** Bug 96615 has been marked as a duplicate of this bug. ***