[poppler] [PATCH] Fix Splash back-end FT_Load_Glyph flags

[Benoit Jacob]

Further remark.

If /really/ you want to keep the idea that you want to use the font's
native hinting when possible and only disable auto-hinting, I just
found that there is FT_LOAD_NO_AUTOHINT that does just that. So if you
want to do that, just edit my function getFTLoadFlags like this:

FT_Int32 SplashFTFont::getFTLoadFlags() const
{
 return aa ? FT_LOAD_NO_AUTOHINT | FT_LOAD_NO_BITMAP
           : FT_LOAD_DEFAULT;
}

I'm not saying that that's a good idea. I just mention it FYI.

However, before you do that, please check with the authors of the
Cairo back-end as there may be a good reason why they /don't/ do that.

I don't have a PDF file with fonts that do native hinting, so I can't test.

My subjective opinion is : don't bother at all, as hinting seems
always a bad idea to me in a PDF viewer; however I thought that I'd
mention that so you know how to implement that if that's what you
really want.

Benoit


2009/9/30 Benoit Jacob <jacob.benoit.1 at gmail.com>:
> Hi,
>
> Here on Opensuse 11.2 / KDE 4.3.1, Okular gave me very poor rendering
> of PDF files. Example:
> http://imagebin.ca/view/Z7jDxF.html
>
> The attached patch fixes it so it now looks like this:
> http://imagebin.ca/view/a_rIPp.html
>
> For comparison, here's how it looks in Evince.
> http://imagebin.ca/view/hoRjU3.html
>
> As you can see, my fix makes Okular render this document exactly like
> Evince does.
>
> *** What it does ***
>
> This patch changes the flags that are passed to FT_Load_Glyph.
>
> First, notice that the Cairo back-end has a very simple logic for
> that: it always passes
>
>    FT_LOAD_NO_HINTING | FT_LOAD_NO_BITMAP
>
> By contrast, the Splash back-end used to have a much more complex
> logic, that was buggy and self-inconsistent.
>
> It was buggy because it did this (in the file splash/SplashFTFont.cc):
>
> // if we have the FT2 bytecode interpreter, autohinting won't be used
> #ifdef TT_CONFIG_OPTION_BYTECODE_INTERPRETER
>
> the bug here is that this preprocessor define is irrelevant here. It
> only tells you that FreeType was built with support for the bytecode
> interpreter, but it doesn't tell that the bytecode interpreter is
> enabled in the user's configuration. Here on openSUSE 11.2, this
> define was defined but the bytecode interpreter was still disabled in
> the default user config.
>
> Notice however a very important fact here: it was actually the
> intention of whoever wrote this code to disable hinting UNLESS the
> bytecode interpreter is enabled. This means that my patch, which
> disables hinting, honors the spirit of this code except perhaps in the
> case where the bytecode interpreter is enabled; but that case never
> was appropriately handled.
>
> Replacing this #ifdef by just "#if 0" was enough to fix the rendering
> glitches that I observed.
>
> However, the code also was self-inconsistent, because the
> determination of the flags was done at 3 different places in the code,
> namely in SplashFTFont::makeGlyph(), in
> SplashFTFont::getGlyphAdvance() and in SplashFTFont::getGlyphPath(),
> and out of these 3 implementations, only the first two agreed, the 3rd
> one was inconsistent with them.
>
> To fix that, my patch introduces a method
> SplashFTFont::getFTLoadFlags() that centralizes the computation of
> these flags.
>
> Here's its code:
>
> FT_Int32 SplashFTFont::getFTLoadFlags() const
> {
>  return aa ? FT_LOAD_NO_HINTING | FT_LOAD_NO_BITMAP
>            : FT_LOAD_DEFAULT;
> }
>
> As you can see, in the anti-aliasing case, it does the exact same
> thing as the Cairo back-end.
>
> I preserved the existing logic for the non-anti-aliasing case, as I
> don't have an opinion on these matters. The logic of using
> FT_LOAD_DEFAULT in that case is what was previously being done in the
> Splash back-end and I just kept it without asking questions.
>
> Cheers,
> Benoit
>