Created attachment 96165 [details] [review] Rotate documents correctly I've tried to rotate a PostScript file from https://bugzilla.redhat.com/show_bug.cgi?id=1031838 in evince and the rotation was not quite correct. It doesn't show anything for 90° and 270°, just warnings about matrices. So I've tested this also with some other PostScript files and I've seen that the rotation doesn't work for any of them. I've prepared a patch which performs the rotation by inserting command "rotate" to the PostScript program right after %%EndSetup and adjusting of offsets accordingly. I've tested the patch for rendering of slices and it works. But I'm not entirely sure whether it will work in every situation. Marek
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. ***
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.