New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DPI #2463

Merged
merged 2 commits into from Nov 22, 2016

Conversation

Projects
None yet
7 participants
@pvandertak

pvandertak commented Jul 15, 2015

Triggered by a few notification e-mails I got referencing my previous pull requests ( wkhtmltopdf/qt#12 and #1863 ), I thought I'd look into it again. This pull request should fix several open issues related to DPI (e.g. #2290) and font size and kerning; at least those referenced by the aforementioned pull requests. The output should also be correctly sized regardless of the operating system and Windows text size settings because qt_defaultDpi() is no longer used (although I tested it only on Windows), so that the --zoom 0.78125 workaround mentioned in #2156 should no longer be necessary.

The version of WebKit used by Qt/wkhtmltopdf seems to assume 96 DPI, and round off values to multiples of 1/96 inch pixels. Therefore, setting the output resolution on the QPrinter was ineffective.

Before this commit (and the corresponding commit in the wkhtmltopdf qt repository), Qt scaled WebKit's output by a factor of qt_defaultDpi() to match the printer DPI. WebKit still rounded the results to whole pixels at 96 DPI, before the values were scaled to the larger DPI. Therefore, increasing the printer DPI did not actually increase the apparent DPI of the rendered content.

The WebKit zoom factor is applied before the values are rounded. Effectively, WebKit thus produces output with 96 * zoom_factor pixels per CSS inch. By setting the zoom_factor to desired_dpi / 96 the desired resolution (pixels per CSS inch) can be achieved. WebKit then renders with the desired DPI, but Qt scales the result by desired_dpi / qt_defaultDpi() again when painting to the PDF printer. If this scaling is removed, the output ends up with the right DPI, and the right scale; i.e. one CSS inch corresponds to one inch on the printed page.

I am not sure whether this pull request plays nicely due to the changes in the qt git submodule, but creating a pull request in the submodule as well seems redundant. If required, the corresponding commits are at https://github.com/pvandertak/qt/tree/dpi_fix.

Some other remarks related to this change:

  1. The version of WebKit in Qt 5.4 seems to have better support for devicePixelRatio/deviceScaleFactor that should likely be used instead of the zoom level. This version does not, and only works with 96 DPI. I don't see any downside to using the zoom level for achieving higher DPI, though. As far as I can see, it gives much better results for font kerning, correct positioning, tiny borders, etc.
  2. I cannot find a good reason why qt_defaultDpi() was used originally instead of 96. I think scaling by qt_defaultDpi() was incorrect, as it caused the rendered page to be scaled incorrectly when qt_defaultDpi() != 96. (See wkhtmltopdf/qt#12) The scaling was originally introduced in http://trac.webkit.org/changeset/32293 , but the commit message does not provide any info why qt_defaultDpi() was used instead of 96.
  3. I set --dpi's default value to 96 since that corresponds to the original behavior. Probably a higher value would be a better default, as it gives better quality without actually costing much (since PDF is a vector format, and raster image resolution is controlled by a different DPI setting anyway).
  4. I hardcoded the DPI in wkhtmltoimage to 96, because that corresponds to what it used to do. It would probably be better to make it configurable in wkhtmltoimage just like it is in wkhtmltopdf.
  5. I don't think the -l flag does anything other than setting the resolution to a preset value, and given the issues with the DPI it did not seem to work reliably. Now that the DPI setting works reliably, perhaps the -l setting should be removed in favor of setting the DPI explicitly.
@ashkulz

This comment has been minimized.

Member

ashkulz commented Jul 27, 2015

Sorry for not replying earlier, looks interesting but I'm not sure whether it's the correct thing to do. WebKit internally uses a DPI of 72 for screen output and 300 for printer output, with a CSS Pixels per Inch of 96. Also, (ab-)using the zoom factor this way makes me slightly uncomfortable; not sure if this approach will have to be carried for the 0.13 builds (and effectively makes it 100% unable to be upstreamed).

@ashkulz ashkulz added this to the future milestone Jul 27, 2015

@pvandertak

This comment has been minimized.

pvandertak commented Jul 27, 2015

Using the zoom factor to fix it is not ideal, but as far as I can see there just isn't native support for increasing the DPI in this version of WebKit. For future versions there seem to be better alternatives, but for this version I don't think there's another way. The zoom factor does exactly what is necessary though.

There are quite a few issues regarding the --dpi setting, font kerning, and proper output scale, and currently the --dpi setting does not work as documented (I see no effect except for page size rounding), so in my opinion this change fixes many issues. I agree that you probably need a better solution when switching to a WebKit version that has better support for device pixel scaling, but that is required regardless of whether this change is merged in the 0.13 builds.

The comment For screen output we assume 1 inch == 72 px, for printer we assume 300 dpi does not seem to correspond to the results I'm seeing. I tried to trace its history, but it seems that is no longer available (before the initial commit). At that time, there seemed to be hardcoded values of 72 that are now gone. Perhaps the comment is no longer accurate.

@ashkulz

This comment has been minimized.

Member

ashkulz commented Jul 27, 2015

See line 646, that is still there. Have you tested on Linux or OS X?

@pvandertak

This comment has been minimized.

pvandertak commented Jul 27, 2015

The 72 on line 646 is because 1 pt is defined as 1/72th of an inch. I was talking about the values at line 54 and 152. Line 54 is in the computeLength* functions, which is the function where the comment in the header file is.

I tested it on Windows 7 and Ubuntu.

@ashkulz

This comment has been minimized.

Member

ashkulz commented Jul 28, 2015

I'll try it with a few test-cases (e.g. #1563) and merge if all looks well.

@llCorvinSll

This comment has been minimized.

llCorvinSll commented Aug 18, 2015

For me this fix resolve issue with windows sacaling on different mashines, and also remove kerning issue on HiDPI Windows mashine

@llCorvinSll

This comment has been minimized.

llCorvinSll commented Nov 10, 2015

@ashkulz any progress in testing this pullrequest?

@ashkulz

This comment has been minimized.

Member

ashkulz commented Dec 15, 2015

not really, I've been MIA for the last few months and most probably till early next year.

@rlmumford

This comment has been minimized.

rlmumford commented Jan 20, 2016

@pvandertak I can't get this to compile as the fix-dpi branch on qt has now been deleted and so running git submodule update after merging the branch gives the following error

fatal: reference is not a tree 14bade74941a20444282acca6d6122564b93921e

@pvandertak

This comment has been minimized.

pvandertak commented Jan 20, 2016

It is probably trying to get the submodule from https://github.com/wkhtmltopdf/qt instead of mine. I think you need to fetch the commits in the qt submodule from https://github.com/pvandertak/qt manually - the dpi_fix branch there exists. I think something like the following should solve it:

cd qt
git fetch https://github.com/pvandertak/qt
cd ..
git submodule update
@tomcardoso

This comment has been minimized.

tomcardoso commented Feb 23, 2016

FWIW, just wanted to +1 this PR — hoping it'll fix a frustrating font sizing problem for me.

@gkinsman

This comment has been minimized.

gkinsman commented Feb 25, 2016

+1 also - we're struggling with varying DPI's on development machines versus production environments. I created a template on a high DPI machine, and was unaware of this issue and so now have to manually set the zoom level for that specific template.

@gkinsman

This comment has been minimized.

gkinsman commented Feb 25, 2016

Ok so it looks like I might be holding it wrong. I've gone back to 12.1.2 to see if it's solved, and I'm still getting different results at different DPI/Res combinations.

Is it even possible to use wkhtmltopdf in a res/dpi independant way? It seems that no matter what I do, switching between 3800x1800 (175% scaling) and 1920x1080 (100% scaling), the resulting PDF's are always different. I've attempted to make my template agnostic of this, but it's quite difficult and imo shouldn't be necessary.

Any ideas?

@pvandertak

This comment has been minimized.

pvandertak commented Mar 6, 2016

The DPI was not working correctly in 12.1.2 either (or in any other version afaik), so that it makes sense that it did not do what you expected when you tried it there. 12.1.2 just had a bug that made the --dpi switch do something. It was not actually changing the DPI accurately, but rather scaling the page with some illogical factor dependent on the system/monitor DPI. Someone mentioned the change as a regression because they apparently relied on the broken behavior. Most likely they were coincidentally using a value that worked for them, but did not notice that e.g. the CSS inch unit no longer was an inch on paper, or that the dots-per-CSS-inch was actually still the same.

It is very likely that this pull request fixes the issue you're seeing and makes the --dpi setting behave the way you expect it to behave.

@tomcardoso

This comment has been minimized.

tomcardoso commented Mar 22, 2016

While we wait for 0.13 to become the master release: can someone explain to me how to get proper measurements using wkhtmltopdf right now? Currently I'm running wkhtmltopdf both locally on a Mac and in a Linux environment, in both cases using the node-wkhtmltopdf package, which wraps wkhtmltopdf. No matter what combination of disable-smart-shrinking, zoom and dpi I try, I can't get my fonts to size properly (i.e. setting some text at 9pt in CSS doesn't actually come out as 9pt in my final file).

What's the current workaround / magical sequence of settings to get predictable output? @ashkulz? @pvandertak?

@gkinsman

This comment has been minimized.

gkinsman commented Mar 22, 2016

I've found that detecting the zoom factor of the OS and supplying that number directly as the zoom factor to the library makes PDF's render consistently regardless of the system DPI.

In Windows this requires a native interop call (as in http://stackoverflow.com/a/21450169/169713) but I'm not sure how to do it on Linux - hopefully it'd be much more straight forward.

Unfortunately unless you can get it from node, it would make it a non-platform-independent fix.

@pvandertak

This comment has been minimized.

pvandertak commented Mar 22, 2016

@tomcardoso I think that currently the only way to get a high-dpi result is by compiling your own version of wkhtmltopdf with the changes from this pull request applied, and the corresponding changes to the qt submodule from https://github.com/pvandertak/qt/tree/dpi_fix.

With the current version, you can get the scaling (but not the DPI) right using the --zoom argument, as @gkinsman wrote. For non-Windows, if memory serves me right, it uses a constant value of 72 or 75. So you would need a --zoom factor of 96/72 or 96/75. (Or was it 72/72=1 or 75/72? It's been a while...You'd have to experiment.)

Still, you will only get an effective DPI of around 96, nothing really high. The --dpi argument currently affects almost nothing. The only thing it really affects is the rounding of the page size.

One particularly unpleasant, intrusive workaround would be to scale your content by 2, e.g. use a font size of 18 pt instead of 9 pt. Then use a --zoom factor of 0.5 multiplied by the OS-specific correction factor above. That would give you an effective DPI of 96/zoom, which would be approximately 192 (depending on the OS). Of course, you can use any scaling factor. A factor of 10 may be easier to work with.

@tomcardoso

This comment has been minimized.

tomcardoso commented Oct 13, 2016

Just gonna pop in here to once again +1 this. It'd be awesome to get this fixed! Happy to help in any way I can (likely through testing once it's merged into a feature branch).

@moopmonster

This comment has been minimized.

moopmonster commented Nov 8, 2016

Hi @pvandertak is it possible to share this fix out (in compiled form) so that we can help to test it? I can't get Qt to compile on windows (and it seems like opening a whole can of worms to get it compiled). Just need to see if it fixes some font kerning issues that we're facing in Windows 10. Thank you.

@pvandertak

This comment has been minimized.

pvandertak commented Nov 8, 2016

Hi @moopmonster, see below for my compiled Windows and Ubuntu binaries. I no longer need HTML to PDF conversion for my current work, so the binaries I have are from last year.

wkhtmltopdf.zip

@ashkulz ashkulz merged commit ffb77c9 into wkhtmltopdf:master Nov 22, 2016

@ashkulz ashkulz added the Merged label Nov 22, 2016

@ashkulz ashkulz modified the milestones: 0.12.4, future Nov 22, 2016

@ashkulz

This comment has been minimized.

Member

ashkulz commented Nov 22, 2016

Thanks, @pvandertak!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment