Failure in PDFToPWGRasterBrowserTest.TestSuccess after freetype roll |
||||||
Issue descriptionFirst failing build: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/30566 Suspecting https://chromium-review.googlesource.com/c/chromium/src/+/682176 after local revert fixed the test.
,
Sep 26 2017
Rather frustrating, given that this passed the linux_chromium_chromeos_rel_ng bot which also ran this test (PDFToPWGRasterBrowserTest.TestSuccess in browser_tests). The underlying file chrome/test/data/printing/pdf_to_pwg_raster_test.pdf doesn't seem to have changed recently either.
That being said, I have been able to reproduce the difference across this CL, have added a small patch to the test to kick out the generated pwg
diff --git a/chrome/browser/printing/pwg_raster_converter_browsertest.cc b/chrome/browser/printing/pwg_raster_converter_browsertest.cc
index 46d3253aeb69..d3f32b0c94a1 100644
--- a/chrome/browser/printing/pwg_raster_converter_browsertest.cc
+++ b/chrome/browser/printing/pwg_raster_converter_browsertest.cc
@@ -105,6 +105,12 @@ IN_PROC_BROWSER_TEST_F(PDFToPWGRasterBrowserTest, TestSuccess) {
ASSERT_TRUE(base::ReadFileToString(pwg_file, &pwg_expected_data_str));
std::string pwg_actual_data_str;
ASSERT_TRUE(base::ReadFileToString(temp_file, &pwg_actual_data_str));
+
+ base::FilePath pdf_output_file = test_data_dir.AppendASCII(
+ "chrome/test/data/printing/pdf_to_pwg_raster_test-output.pwg");
+ int written = base::WriteFile(pdf_output_file, pwg_actual_data_str.c_str(), pwg_actual_data_str.size());
+ (void)written;
+
ASSERT_EQ(pwg_expected_data_str, pwg_actual_data_str);
}
and compiled rasterview from http://michaelrsweet.github.io/rasterview/ to render them and then compared the output by lining them up in gimp. It turns out that with this change, some of the characters have their right and left sides trimmed a bit. It looks like the glyphs may have been rendered with subpixel antialiasing but then were downsampled to full pixel antialiased. The exact code path used will need to be investigated to create a reduced test case, as it is not immediately apparent what changed.
,
Sep 27 2017
,
Sep 27 2017
I've attached the original and after roll bitmaps of the output so that the issue can be seen by those who can't directly read raw escaped strings of pwg data. Note that PDFium isn't using Skia to draw anything here, it appears it's just talking to FreeType directly. I'll see if rolling PDFium's FreeType affects any PDFium tests directly.
,
Sep 27 2017
Firefox has implemented an additional patch to fix Skia cropping. Please do not disregard these: https://bugs.chromium.org/p/skia/issues/detail?id=6663#c9 https://bugs.chromium.org/p/skia/issues/detail?id=6663#c10 How many copies of FreeType you carry and interact between? You might need a sync and a massive rebuild each time you roll. For example, see http://lists.nongnu.org/archive/html/freetype-devel/2017-09/msg00140.html
,
Sep 27 2017
There is no Skia involved here, this is PDFium using FreeType directly. Chromium will be built either with one copy of FreeType built in, or it will use the system FreeType, depending on the use_system_freetype gn build variable. PDFium in these tests will use the FreeType used by Chromium. I also wrote the CL mentioned in the above freetype-devel link, this issue happens after that. And yes, every Chromium build is a sync and massive rebuild (though is_component=true splits things up nicely here and makes things much easier and faster). The odd thing about this is that this was just rolling in FreeType 02e80da60 (we had already rolled to just before it). We've already rolled everything in Chromium well past the other lcd changes. It also passed the Chromium test bots the first time around, even though this pdf appears to contain its own subsetted fonts.
,
Sep 27 2017
I am not sure what I am supposed to see in those images. Some pixel cropping? Anyhow, there will be more changes in the coming days. Perhaps it will fix by itself.
,
Sep 27 2017
One more interesting confounding observation. Extracting the fonts from this pdf and noting that 'F' is glyph 41 in the 'Arial' subset font (the other font appears to only be used for the null glyph?), then building in debug and running this test under gdb gdb --args out/debug/browser_tests --gtest_filter=PDFToPWGRasterBrowserTest.TestSuccess --single-process (gdb) break CFX_FaceCache::RenderGlyph (gdb) run Shows that PDFium's glyph cache is working and the 'F' glyph is only rasterized once into the cache. However, in the 'with_roll.png' above the first 'F' is clipped and the second is not. I can only assume the second time there is a cache hit, but this seems to render fine. When the cache goes to render it does call FT_Render_Glyph with FT_RENDER_MODE_LCD. It comes back with a width of 12 (4 pixels) which seems reasonable. Later it does try to shift this mask around by the subpixels. The first time it draws 'F' is at 2 subpixel shift. However, none of this blitting code depends on FreeType. I'll have to take another look when I have time.
,
Sep 27 2017
Assigning to bungeman@. Agree it's frustrating. Sometimes tests will pass on CrOS in the CQ (which doesn't run tests for debug builds) but fail on other builders that do build for debug.
,
Sep 28 2017
FT_RENDER_MODE_LCD? ClearType enabled? Regardless, the images are clearly not LCD but they should be. Are you caching the whole LCD image?
,
Sep 28 2017
The first concrete thing I can see different between the before and after runs is that before the FreeType change FreeType handed back a 18x5 bitmap and after it hands back a 12x5. Where they overlap they have the same values, but FreeType seems to have dropped some non-zero values from the glyph rendering. This FreeType is built with FT_CONFIG_OPTION_SUBPIXEL_RENDERING, so I wouldn't have expected anything to change here.
If I apply the FreeType CL and then apply the patch
diff --git a/src/smooth/ftsmooth.c b/src/smooth/ftsmooth.c
index 99a9883d..b7d55750 100644
--- a/src/smooth/ftsmooth.c
+++ b/src/smooth/ftsmooth.c
@@ -174,7 +174,7 @@
#else /* FT_CONFIG_OPTION_SUBPIXEL_RENDERING */
/* add minimal padding for LCD filter depending on specific weights */
- if ( lcd_filter_func == ft_lcd_filter_fir )
+ if ( lcd_filter_func /*== ft_lcd_filter_fir*/ )
{
if ( hmul )
{
then things magically work. As a result I suspect that there are multiple copies of ft_lcd_filter_fir floating about and as a result my assertion in Comment #6 that there is only one copy of FreeType in Chromium is false.
This is borne out by adding a code test in ftsmooth when lcd_filter_func is set but not equal to ft_lcd_filter_fir (it can't work as a breakpoint because we're actually testing the linker here).
diff --git a/src/smooth/ftsmooth.c b/src/smooth/ftsmooth.c
index 99a9883d..47ea5f7e 100644
--- a/src/smooth/ftsmooth.c
+++ b/src/smooth/ftsmooth.c
@@ -174,8 +174,12 @@
#else /* FT_CONFIG_OPTION_SUBPIXEL_RENDERING */
/* add minimal padding for LCD filter depending on specific weights */
- if ( lcd_filter_func == ft_lcd_filter_fir )
+ if ( lcd_filter_func /*== ft_lcd_filter_fir*/ )
{
+ if (lcd_filter_func != ft_lcd_filter_fir) {
+ int i = 0;
+ (void)i;
+ }
if ( hmul )
{
cbox.xMax += lcd_weights[4] ? 43
And setting a breakpoint on the 'int i' line. Sure enough, there are two copies.
0x00007fffe1ac83d3 <+403>: lea 0x85f06(%rip),%rax # 0x7fffe1b4e2e0 <ft_lcd_filter_fir>
(gdb) info symbol 0x7fffe1b4e2e0
ft_lcd_filter_fir in section .text of src/chromium/src/out/debug/./libfreetype.so.6
(gdb) print lcd_filter_func
$1 = (FT_Bitmap_LcdFilterFunc) 0x7ffff3770f20 <ft_lcd_filter_fir>
(gdb) info symbol 0x7ffff3770f20
ft_lcd_filter_fir in section .text of src/chromium/src/out/debug/./libgfx.so
This is probably only an issue in component builds, because these were compiled from the same source in the non-component build the linker will just pick one. Which probably explains why we are only seeing this on some builders. In the future, the gold linker will warn/error in this situation (see https://bugs.chromium.org/p/chromium/issues/detail?id=449754 ) but we're probably blocking that bug at the moment (as are many other things).
As a result, there is probably no real FreeType issue here, this probably just a Chromium build issue.
,
Sep 28 2017
Great analysis, thank you for digging into this! And... argh! Any idea where this structure mismatch is coming from? Did some builders miss a rebuild of FreeType after the roll, or what could this be?
,
Sep 28 2017
Hmmm. Why ft_lcd_filter_fir symbol is even exported? http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=771d3c8ae25d5042a3f6b76a329ad3dd782eca66
,
Sep 28 2017
So the state of FreeType in Chromium right now is that in a component build libfreetype.so.6 is built and makes sense. However, harfbuzz-ng is a group containing two static libraries, one of which depends on the 'base' FreeType static library. As a result any shared object which depends on harfbuzz-ng will statically link against the two harfbuzz static libraries and the base static FreeType library, hence having an additional copy of everything in bootstrap_freetype_for_harfbuzz. This leads to all the problems one would expect from having multiple copies of a library present, in this case it means there are multiple copies of some functions floating around. In the FreeType change in the roll which was reverted, it is assumed that there is only one copy of the function ft_lcd_filter_fir, but there are two. In a non-component build, the linker will just pick one of everything, so we don't see the issue. The builders all rebuilt FreeType as expected, it's just that the component ones will break but the non-component ones are 'fine' (but would run afoul of gold's --detect-odr-violations flag, if we can ever get it turned on). Note that the ft_lcd_filter_fir isn't ever exported, it's just that one copy of FreeType is using one copy, and a different copy of FreeType is using another. It's just an ODR violation across shared objects. The only solution I can really think of off the top of my head is to build all the various 'base' libraries as "component" so that they're built as shared objects in a component build to prevent the duplication of symbols.
,
Sep 28 2017
At least that comparison has just moved back to ftlcdfil.c, so it is "static" so to speak. Perhaps this will help.
,
Sep 28 2017
I just took a look at converting everything to component (shared libraries in a component build) and I don't think that's directly possible because the build system very much does not want an target (like an .so) to be overwritten. As a result we can't really use the trick that distros use of building FreeType without HarfBuzz, then building HarfBuzz with FreeType, then building FreeType with HarfBuzz and overwriting the original FreeType. As a result, I'm leaning toward Chromium's HarfBuzz and FreeType targets just being source sets and just creating a 'font' component (name subject to bike shedding) which pulls in both source sets. This makes no difference to the non-component build (except fixing the odr violation situation), and actually makes the component build a bit nicer since most changes were going to cause rebuilds and re-links anyway.
,
Sep 29 2017
I took a bit of a look at trying to have a separate libfreetype.so.6 and libharfbuzz.so in the component build, mostly so that there is a simple way to support using system libraries. Otherwise it's a bit messy to support the 'use internal harfbuzz but system freetype' (and a few other) situations. The only way to break the build cycle without being destructive is to do something like 1) compile all the sources. 2) link the base libraries. 3) link the complete libraries against the base libraries. 4) create the final libraries from the complete libraries using a tool like 'patchelf --replace-needed <other complete library path> <other final library path> <complete library>' that outputs to <final library path> instead of updating in place. In other words, create the libraries with the right code then create copies of those libraries with fixed up paths. Unfortunately, while patchelf exists, it doesn't quite do what we want and it doesn't work with all supported shared objects (we really want the build's linker tool to handle this sort of thing if we wanted it). So I think this is possible, but I don't think it's practical. Also, I find that https://chromium.googlesource.com/chromium/src/+/lkcr/docs/component_build.md#dependencies-between-targets is quite apropos here.
,
Oct 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7619f01f1fa10674df56b9921df2f94718478201 commit 7619f01f1fa10674df56b9921df2f94718478201 Author: Ben Wagner <bungeman@chromium.org> Date: Mon Oct 09 15:35:00 2017 Build FreeType and HarfBuzz together. The effect of this change is to hide the FreeType and HarfBuzz builds from the rest of the build except for a new component library named 'freetype_harfbuzz'. All existing users of the 'freetype' and 'harfbuzz' targets will instead depend on the 'freetype_harfbuzz' target. The 'freetype_harfbuzz' component will depend on the appropriate source sets or export the system packages for both FreeType and HarfBuzz. The reason for this change is that the full FreeType and HarfBuzz libraries we desire are circularly dependent. Before this change the build attempted to work around this by building HarfBuzz as a static library which also pulled in a small bootstrap FreeType static library. However, this causes ODR violations in component build, and potentially in the non-component build as well. Recent changes in FreeType cause FreeType to act incorrectly in these circumstances, so these ODR issues are preventing FreeType from rolling. A different approach would be to 1) Compile all the sources. 2) Link the base libraries. 3) Link the complete libraries against the base libraries. 4) Create the final libraries from the complete libraries by re-targeting the dependent library names directly. This last step can in theory be done for ELF using a tool like patchelf --replace-needed <other complete library path> <other final library path> <complete library> that outputs to <final library path> instead of updating in place. However, even if there were such a tool for dlls, this is something which would be needed on any platform. As a result, this only seems promising if all linkers supported this sort of re-writing. BUG= chromium:768938 Change-Id: I488dc8cde8f6a8f88903d25f13ecd61fd2d74a3f Reviewed-on: https://chromium-review.googlesource.com/696241 Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: Elliot Glaysher <erg@chromium.org> Reviewed-by: Alex Clarke <alexclarke@chromium.org> Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Ben Wagner <bungeman@chromium.org> Cr-Commit-Position: refs/heads/master@{#507381} [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/build/config/freetype/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/build/linux/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/chrome/browser/ui/libgtkui/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/content/shell/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/headless/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/remoting/host/it2me/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/third_party/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/third_party/WebKit/Source/platform/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/third_party/WebKit/Source/platform/fonts/WebFontDecoder.cpp [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/third_party/freetype/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/third_party/harfbuzz-ng/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/ui/gfx/BUILD.gn [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/ui/gfx/harfbuzz_font_skia.h [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/ui/gfx/render_text_harfbuzz.cc [modify] https://crrev.com/7619f01f1fa10674df56b9921df2f94718478201/ui/gfx/render_text_harfbuzz.h
,
Oct 9 2017
Checked locally that rolling FreeType past the commit in question no longer reproduces this issue.
,
Dec 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b5d4c386b9969cff0a8c5c33d51c9ac7112e1b08 commit b5d4c386b9969cff0a8c5c33d51c9ac7112e1b08 Author: Tom Anderson <thomasanderson@chromium.org> Date: Fri Dec 22 02:40:20 2017 Fix harfbuzz unbundle This CL changes the harfbuzz unbundle to set use_system_harfbuzz=true. Since this build flag exists, it's easier to toggle it than to rewrite third_party/harfbuzz-ng/BUILD.gn entirely. R=thestig@chromium.org BUG= 768938 Change-Id: I510994b1d78dc3fe4866ff341018b458ab24e78b Reviewed-on: https://chromium-review.googlesource.com/840621 Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Cr-Commit-Position: refs/heads/master@{#525909} [modify] https://crrev.com/b5d4c386b9969cff0a8c5c33d51c9ac7112e1b08/build/linux/unbundle/harfbuzz-ng.gn [modify] https://crrev.com/b5d4c386b9969cff0a8c5c33d51c9ac7112e1b08/build/linux/unbundle/replace_gn_files.py
,
Jan 22 2018
,
Jan 23 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Sep 26 2017