Created attachment 46889 [details] PDF with thin grid lines The attached file has a background grid. Poppler "randomly" renders the thin lines as solid 1-px thick lines. It would be great if such thin lines were instead rendered as gray, or just not rendered at all, or something. Anything better than rendering random ones as solid black lines.
Created attachment 46890 [details] What I see
Created attachment 46891 [details] What I see after pressing ctrl+"+" once These two screenshots show what I see (just in case you can't reproduce). In the first screenshot, we see a certain irregular set of horizontal and vertical lines. In the second screenshot we have zoomed in one step, and now see a completely different set of horizontal and vertical lines, seemingly random. It is also so dark that it really detracts from viewing the PDF. When printed, these lines are very thin and gives a very subtle effect. It would be great if very thin lines were either not drawn at all, or always drawn but done in gray, depending on their thickness.
Someone else had a look at my pdf and told me that: *) This bug is reproducible in poppler 0.15.3 *) The line widths are very thin - 0.06 *) It works fine in Adobe. With and without the "enhance thin line" option.
It works for me in Evince, so I believe this bug is in the splash backend.
(In reply to comment #5) > It works for me in Evince, so I believe this bug is in the splash backend. What version of poppler please? Could you attach a screenshot please?
Created attachment 46920 [details] evince rendering Here is a portion of evince's rendering (which uses cairo backend), poppler version 0.16.5
Thank you very much. I ran evince and can also confirm that it looks just fine there. So.. a bug in okular maybe? Or is there an option in popplar to do this maybe?
Should this be assigned the -qt renderer maybe?
Jose already assigned it to the correct component.
Created attachment 47549 [details] horizontal lines zathura I can confirm this bug in xpdf and zathura using poppler-0.16.4.
Created attachment 47550 [details] horizontal lines xpdf This happens with every pdf I've tried, and varies with zoom level.
Now I understand the reason for GlobalParams::setMinLineWidth :-) Albert, the output of pdftoppm is much better with this PDF if we call globalParams->setMinLineWidth(1.0). Please give it a try, perhaps we should think about to use 1.0 as default value....
Thomas, have you tried running a regtest and seeing if all the changes this surely mean make sense to you?
(In reply to comment #14) > Thomas, have you tried running a regtest and seeing if all the changes this > surely mean make sense to you? No, I was so surprised about that easy solution that I first want to hear Your opinion about it. But as far as I understand the implementation of min linewidth it could be an idea. I'll regtest it coming weekend. If it will not make sense in general, we could think about a command line option. But I would prefer to have reasonable default values.
A command line option doesn't really make much sense, it's giving the user a power over something he has no clue how to control
Of course You're true once again, even if I like the effects of setting a minimum line width in this case. But when regtesting it, I encountered such a lot of changes, most of them look better, but some of them look worse, that I broke the regtest and had a deeper look in the code. And I found the problem: Even if pathAllOutside in Splash::fillWithPattern says, that the line is partial inside, when the XPath is created and antialiased, state->clip->testRect later on says that the line is all outside. The problem seems to be in SplashXPath::SplashXPath, where adjust and strokeAdjust moves the line out of the clip area, if the line is very thin. But I need more time to solve it completely and don't break other renderings, so I'll come back when I find a clean solution.
Created attachment 60900 [details] [review] 2. approach of solving thin lines I'm talking about a second approach and don't mean the min line width option. My first approach I haven't uploaded: There I changed the xpatch generation in general. But I broke regtesting it after several files, because it changed output in general. And why changing already accepted rendering. In this second approach I tried only to let thin lines inside the clipping area. The detection of thin horizontal and vertical lines is quite easy: Then the clipping area has no height or no width. I'm very interested if You accept the changes.
Created attachment 61019 [details] Rendered with my 3. approach and 150 dpi I think, I found a better detection of lines and line handling. But I've to regtest it. Here the result of that solution.
3rd approach screenshot looks very nice
Yeah, but too much regressions. I'm still working on this, but I fear it will still take a while.
Created attachment 62123 [details] Rendered with my final approach I'm now satisfied with my solution of this bug, s. attached output. This is now comparable with the output of acrobat when the preference "Enhance thin lines" is on. I decided also to spend a new GlobalParams value, enhanceThinLines, which, when setting it to gFalse, produces once again the output of comment 19. So it's up to the calling application if it wants this output or the output of comment 19. So setting it to gFalse (gTrue is the default) has the same output as if You switch off "Enhance thin lines" in acrobat. I'm not able to upload a patch right now, because it's just the fourth patch in my queue, so it will take probably a few weeks until the patch will be available. And if I tell that even with this already fine tuned patch I got 6299 differences in my regression test, it will probably take some additional time until the patch will be committed. So round about 36 % of my tests failed, but this is okay, because the patch changes the output of a basic function in PDF, and it solves the bug in our code where at least for vertical lines it is dependent from the y position if a line is painted at all and when, also the shape is dependent on the position.
(In reply to comment #22) > I decided also to spend a new GlobalParams value, enhanceThinLines, which, when > setting it to gFalse, produces once again the output of comment 19. So it's up Isn't that what the strokeAdjust GlobalParams value is for?
(In reply to comment #23) > (In reply to comment #22) > > I decided also to spend a new GlobalParams value, enhanceThinLines, which, when > > setting it to gFalse, produces once again the output of comment 19. So it's up > > Isn't that what the strokeAdjust GlobalParams value is for? Setting "stroke adjustment" to gTrue means that lines are rounded to an integer number of pixels thickness, to make sure all vertical or horizontal lines appear the same weight, no matter how they align with the pixel grid. enhanceThinLines now means that lines (I implemented only vertical and horizontal lines) that all lines with a width less than one pixel will always shown with shape 255, if it is set to gFalse they are painted with a shape corresponding to their width. Therefore I think there is a difference.
(In reply to comment #24) > Setting "stroke adjustment" to gTrue means that lines are rounded to an integer > number of pixels thickness, to make sure all vertical or horizontal lines > appear the same weight, no matter how they align with the pixel grid. > enhanceThinLines now means that lines (I implemented only vertical and > horizontal lines) that all lines with a width less than one pixel will always > shown with shape 255, if it is set to gFalse they are painted with a shape > corresponding to their width. > Therefore I think there is a difference. ISO32000 page 319: "If stroke adjustment is enabled and the requested line width, transformed into device space, is less than half a pixel, the stroke shall be rendered as a single-pixel line." It looks to me like stroke adjust does both.
(In reply to comment #25) > ISO32000 page 319: > "If stroke adjustment is enabled and the requested line width, transformed into > device space, is less than half a > pixel, the stroke shall be rendered as a single-pixel line." > > It looks to me like stroke adjust does both. You speak about the PDF spec, I speak about the xpdf introduce of stroke adjust: Implemented stroke adjustment (always enabled by default, ignoring the SA parameter, to match Adobe's behavior), and added the strokeAdjust xpdfrc command. And I agree with arguement of xpdf: it seems as if acrobat reader ignores the stroke adjust parameter SA, but have the preference "enhance thin lines". Please have a look at the PDF of this call with acrobat reader and change this preference. You'll see a comparable output tp comment 19 and comment 22, either You switch it on or off, and if has nothing to do if You change the SA parameter in the PDF itself.
Thomas want me to have a look at this patch now?
(In reply to comment #27) > Thomas want me to have a look at this patch now? I'll create a patch in near futur, probably next weekend!
Created attachment 65140 [details] [review] Promised patch Here now my promised patch. As I already mentioned, it changes the output of a lot of pages with splash, and I must admit that I hadn't looked in every changed output. But I made a lot of spot checks, and I like the changed output.
Created attachment 68838 [details] [review] rebased patch Here the promised rebased patch. If You think, that it is useful but change too much in rendered output, I'll change enhanceThinLines to a three state value with a) either change nothing or b) paint thin lines 1 pixel solid or c) paint thin lines 1 pixel but non solid But please have first a look at it as it is...
Ok, as you suggested this seems like a big change that needs a lot of regtesting and looking at files to make sure that the change is acceptable. We'll move it to a 0.24 feature (next version). Is that OK for you Thomas?
Now that 0.22 is released I want to ask You to think once again about my slighty changed suggestion in comment 30: Because of the huge changes in rendering with the patch, probably the most users of poppler will get unexpected results with the patch, and even if we think that the result is then more acrobat like, we are not acrobat. I know, that You're not a big friend of options, but if I change my patch in that way that the default behaviour is interpreting the PDF SA command which means that I draw solid lines if SA is on and and the requested line width, transformed into device space, is less than half a pixel and a shaped line else, and let the output as it is if SA is off, I think the rendering changes would be more acceptable. And if we then implement an option to override this default behaviour with either always draw solid 1 pixel lines or draw shaped 1 pixel lines it will keep all options for an user.
I'm not against options, it is just that more options makes it harder to regtest (basically I will end up regtesting just the default one) and also makes it harder to debug a bug because the user will fail to mention which of the options he is using. But if you think it is better to introduce an option, let's see it. Ideally it would not end up in globalparams, since i'm dreaming of killing globalparams one day.
Created attachment 72580 [details] [review] Introduce option SplashThinLineMode Here now a new option SplashThinLineMode in the SplashOutputDev constructor (and not in GlobalParams :-) ). I upload this patch with the default value splashThinLineDefault, which changes nothing in rendering, so also the PDF of comment 1 still looks awful, and in the patch there is no way to override the default value. To see differences, You just need to change the default value in the constructor in SplashOutputDev.h. Possible other values are: a) splashThinLineSolid: with this value the output looks like in cairo and like in acrobat reader with the default setting "enhance thin lines on" b) splashThinLineShape: with this value the output looks like in acrobat reader with the setting "enhance thin lines off" I haven't changed the rendering output for splashThinLineDefault, because I encountered that nearly no PDF uses the stroke adjust (SA) parameter. I guess, one reason for this is that acrobat reader ignores this PDF parameter. I think, that splashThinLineSolid would be the better default value, because it changes the rendering to the cairo or acrobat reader way, but as I already commented, this would change 39% of rendering output with splash. But now it's up to You to decide. If You let the default value on splashThinLineDefault, we should probably spend a commandline option to pdftoppm to override it, even You denied that in comment 16. I would do it and I also would change the man page then. In either case I plan to extend the qt extension to override the default value, so that programs like okular can introduce the possibility to set this option at least to either splashThinLineSolid or splashThinLineShape to simulate the acrobat reader possibilities, but the implementation also depends on Your decision which should be the default value.
Should the splash->setThinLineMode(splashThinLineDefault); in SplashOutputDev::type3D1 follow the value set in the constructor?
(In reply to comment #35) > Should the splash->setThinLineMode(splashThinLineDefault); in > SplashOutputDev::type3D1 follow the value set in the constructor? Hmmh, I've done that intentional: I don't want to change character drawing (if it isn't done with setting a clipping path and fill operation, we have there one example in our PDF suite). If we would do so, probably also antialiasing effects in characters would change, and I didn't find enough samples to decide if changing the drawing mode here looks prettier or not, therefore I let it here as it is.
Patch commited. Should we close here and continue the qt improvements in a different bug or you prefer to do it here?
(In reply to comment #37) > Patch commited. Should we close here and continue the qt improvements in a > different bug or you prefer to do it here? I think we should do it here, because the reporter complains about the display in okular in comment 2 and 3, and without the qt extension we have no solution for it and can't close this bug. BTW, since You commit it with splashThinLineDefault, should we spend a commandline option for pdftoppm, too? Wouldn't do that here, another matter altogether.
(In reply to comment #38) > (In reply to comment #37) > > Patch commited. Should we close here and continue the qt improvements in a > > different bug or you prefer to do it here? > > I think we should do it here, because the reporter complains about the > display in okular in comment 2 and 3, and without the qt extension we have > no solution for it and can't close this bug. Ok > > BTW, since You commit it with splashThinLineDefault, should we spend a > commandline option for pdftoppm, too? Wouldn't do that here, another matter > altogether. Ok
Created attachment 74941 [details] [review] enables the usage of thin line modes in the qt frontend
Created attachment 74942 [details] [review] shows a possibility to use thin line modes in okular This is a patch for the actual okular code. You can change the visibility of thin lines in Settings::Configure okular, menu Accessibility.
I think it'd be cleaner reusing the RenderHint enum instead of adding a setter that takes an int. Also a better explanation than splashThinLineDefault would be needed, i don't expect "users" of the Qt frontend to necessarily understand what splashThinLineDefault means
(In reply to comment #42) > I think it'd be cleaner reusing the RenderHint enum instead of adding a > setter that takes an int. I thought about that first, but hint is a bit mask with a boolean meaning (switch it on / off), but here we have a three state logic. Or would You prefer to omit the shape mode and then switch on the "enhance thin lines" in solid mode? > Also a better explanation than > splashThinLineDefault would be needed, i don't expect "users" of the Qt > frontend to necessarily understand what splashThinLineDefault means You think something about "No special handling for thin lines"? Or if we omit the "shape mode" we could just comment "enhance thin line on/off"!
(In reply to comment #43) > (In reply to comment #42) > > I think it'd be cleaner reusing the RenderHint enum instead of adding a > > setter that takes an int. > > I thought about that first, but hint is a bit mask with a boolean meaning > (switch it on / off), but here we have a three state logic. Or would You > prefer to omit the shape mode and then switch on the "enhance thin lines" in > solid mode? Well, the thing is that "thin lines enhacement" clearly fit into the "RenderHint" idea so I'd like to use that. We have two options one as you say only provide an enum for on/off, but you added 3 states so i guess it makes sense to export the 3. The option to export the three is add two enums, say ThinLineSolid and ThinLineShape and document that if both are present one "wins" over the other. I think this second solution might work, what do you think? > > > Also a better explanation than > > splashThinLineDefault would be needed, i don't expect "users" of the Qt > > frontend to necessarily understand what splashThinLineDefault means > > You think something about "No special handling for thin lines"? Or if we > omit the "shape mode" we could just comment "enhance thin line on/off"! To be honest i didn't really understand the difference between the 3 modes so i'm not really sure i can comment in what should be written, but splashThinLineDefault is what should not :D Nobody will understand that without having to read the code (and a lot of code)
Created attachment 75400 [details] [review] use two enums for thin line modes in qt I changed it now and use two enums instead of the three state int. I also correct a small bug in the thinLineShape mode in splash: because lineWidth isn't in user device coordinates, the calculated shape in this mode never changed. Now it is transfered first into user device coordinates to calculate the shape.
Created attachment 75401 [details] [review] patch for okular to test the qt enahncements Here You can also better see the the changes for thinLineShape mode!
(In reply to comment #44) > To be honest i didn't really understand the difference between the 3 modes > so i'm not really sure i can comment in what should be written, but > splashThinLineDefault is what should not :D Nobody will understand that > without having to read the code (and a lot of code) I'll write a short document to explain the thin line modes and will upload it to this patch, probably tomorrow. I think a few pictures makes it easier to understand.
Why the changes outside the qt4 folder?
(In reply to comment #48) > Why the changes outside the qt4 folder? Sorry, remove the SplashOutputDev.h changes. Because I encountered problems with the mode thinLinesShape I changed the default to test it with pdftopmm and forgot to revert it. The changes in Splash.cc were to fix that problem with thinLinesShape. The changes are harmless, because this mode is queried in the if statement, and I didn't want to open a new bug.
Commited, I've adapted the okular patch a bit to make it appear in the backend settings dialog instead of in the global one
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.