fix left-shifted/misaligned bounding-box + wrong kerning in imagettfbbox/imageftbbox #1845
Conversation
…box/imageftbbox - load glyph with FT_LOAD_IGNORE_TRANSFORM for bbox as final bbox is rotated at once later (fixes "double-rotation" per glyph for calculating bbox) - reload the rotated glyph for painting after that (only if angle != 0) - rotate the original bbox at 0,0 and do not throw away xMin/yMin (drawing-rotation is also based at "origin" point - including the bearingX, see http://www.freetype.org/freetype2/docs/glyphs/glyphs-3.html#section-3) - this fixes the "left-shift"-problem also when angle = 0 - removed "xb/yb" and use "x/y" directly for offsetting brect (no need for adding "x1/y1" and substracting "yd" later) - removed therefore unused "yd" helper var which seems tried to fix parts of the original problems - initialize x/y with 0 instead of -1 in php_imagettftext_common() to make image*text() and image*bbox() results identical (there was a -1px shift in image*bbox() before) - fixed gdroundupdown() for negative numbers (-256 / 64 gives -5 instead of -4 before) - rotate kerning-delta by given angle (fixes completely wrong kerning and therefore wrong bounding box if angle != 0) - changed 3 tests and added a new one to test for the new (better) coordinates
|
@remicollet maybe you can have a look at this, or link someone adequate :-) |
|
Hi Awesome work. Thanks! (Thx Kalle, this PR went off my radar) I have to check as well with upstream GDorothea to sync and valid the changes but as far as I can see this is the definitive way to fix these issues. Well done. |
|
Thanks for tackling this annoying issue, Mark! I've just built the PR, but the new test (bug53504.phpt) fails on my system. 53504.diff: Also bug48801_1.phpt is failing. The diff: Not sure what's wrong there (maybe a different freetype version; I'm having libfreetype6 (= 2.5.2-1ubuntu2.5)). Unfortunately, the Travis checks don't execute these tests at all, so it seems it would be good to have additional testers, and maybe we should try to add some margin of error, anyway.
If noone beats me to it, I'll prepare a PR for libgd. Would like to have the tests running first, though. :) |
|
Strange ... I tested this with libfreetype6 2.5.2-3+deb8u1 ... but I agree, the tests could include a +/-1 tolerance (I just adopted the same test-technique as before). Though the two bugs like the 1px-shift and the rounding bug will not be covered by the test then :-) But the 3-pixel difference in the last line seems to be a little too much. Did you have a look at the PNG output file of this test? Is it still plausible? Does it still look nice? Could you attach it here? |
|
Yes, the image looks plausible:
|
|
okay yes ... seems that this is a litte rounding issue when adding the glyph specific offset ("Big" is shorter, "H-Shift" is longer in your PNG ...). But this will be another issue, I think. The bounding box is correct in both cases. Just difficult for the automated tests ... But it would be interesting, what makes the differrence here if it's not the freetype version. I currently have no idea. |
|
The patch looks good to me. And yes freetype versions and compilation settings may create slightly different results. In any case thanks a lot for your work, I have no problem to apply this patch to 5.6+ (and gd upstream but will need some tweaks I think, for 2.2 and master). |
|
Ok, cool. Do you have an idea, how to avoid failing tests here for different Freetype versions, without making things too complicated, and still have meaningful tests? |
|
There is no magic here. Testing the version in the test helps a bit On Fri, Jun 24, 2016 at 1:36 PM, Mark Plomer notifications@github.com
Pierre |
|
It would be helpful to analyze related test failures on http://qa.php.net/reports/run_tests.php. Unfortunately, the page is broken (already reported as https://bugs.php.net/72485). Perhaps its best to merge the PR as is, and do the fine tuning of the test(s) later. |
| ?> | ||
| --CLEAN-- | ||
| <?php @unlink(dirname(__FILE__) . '/bug53504.png'); ?> | ||
| --EXPECTF-- |
There was a problem hiding this comment.
Minor issue: this doesn't need to be EXPECTF; EXPECT would be sufficient.
|
By the way, this is still problematic in 7.0... |
|
@olivierberten Which problem do you mean? |
|
7.0.6 with bundled gd has the same problem. I just meant your patch also needs to be applied on the 7.0 branch. |
|
This is what 5.6+ means.
|
|
A small issue: I currently realized, that at 90°-rotated "Multi Line Test" and "AV Teg" (Test bug53504.phpt) the bottom right is 1px below bottom left. Maybe there is still another issue with the rounding-logic at the end of gdImageStringFTEx() ... but this is really more cosmetic :-) |
Just tried to, and noticed that gdft.c has grossly diverged between libgd and PHP's bundled version. According to the logs the latest sync of the bundled GD was with 6e9c4b3 (i.e more than 12 years ago). Bringing these files in sync again appears to require a major effort. :-( |
|
Is there a chance to add this PR at least to PHP for now, and do the sync with libgd in another task, if this is a bigger thing? |
|
The big problem is that we still have issues with FreeType; apperently, different versions/configurations are causing slightly different behavior, see, for instance, the recently reported libgd/libgd#302. This PR might generally improve things, or at least doesn't appear to make them worse, so I'm not against merging this PR. @pierrejoye What do you think? |
|
We cannot avoid freetype differences based on the configuration options. However the issue fixed by this PR is valid, no matter the config. Until we have the visual difference image comparisons in place, we can try to skip the tests, adding the missing constants to identify the ft options if necessary. I would really like to merge it. For upstream master the other problem is we have yet another change I am not to keen about, the LTR library support ... |
Fine. I'm going to merge within the next days, and also have a closer look at the tests.
I agree that we need a generally accepted solution there. :-) |
Fixes several (hopefully all :-)) concurring bugs/problems in calculating the bounding box in imagettfbbox/imageftbbox. Also fixed kerning for angle != 0.
This addresses several bug reports (I used https://bugs.php.net/bug.php?id=53504 as the "main" one):
Detailled changes:
This pull-request is for PHP 5.6 as the minimal version to fix. I also cherry-picked the fix from "Paul Tarjan" from 7.0 in this pull-request, to be consistent, so the main commit can be merged to 7.0 and master without bigger problems.
As an example for the fixes, here the changes visualized in the new test bug53504.phpt:
Before:
After:
And the visualized changes in bug43073_1.phpt:
Before:
After:
