Enhance the handling of symbolic fonts combined with kerning and ligature option (#2143)

[speckyspooky]

No description provided.

[hvbtup]

@vk-github18 @speckyspooky I'm a bit worried regarding multi-threading.

BIRT is supposed to work in a multi-threading environment.

But OpenPDF uses a global LayoutProcessor (a class, with static objects, or a singleton, I don't remember ATM), with some flags.
And we switch these flags on and off possibly several times during processing a BIRT report.

Of course this works in simple test cases, but isn't this going to run into bugs due to race conditions in a MT environement under load? And these kind of bugs are very, very hard to reproduce and to analyze later.

Probably this cannot be solved in BIRT at all, but requires a change in OpenPDF?

[vk-github18]

Why would you change the flags? Can you provide a usecase and example?

[hvbtup]

@vk-github18 Regarding "Why would you change the flags?" Because that's what 2aef7c4 does.

[vk-github18]

LayoutProcessor is ment to be switched on before Processing and not to be switched on and off

[hvbtup]

Let me summarize the situation and please correct me if I'm wrong.

Since we updated to OpenPDF 2.0.3, several issues were reported regarding different non-latin fonts and ligatures with the Calibri font (#2143, #2133, #2119, #2058, #2043, #2033).

Thomas (with your help) tried his best to fix these issues, which all seem to be related to LayoutProcessor somehow (at least this is my impression).

Fixing the latest issue #2143 seems to require switching LayoutProcessor features on and off on a per-font basis before calling drawText (see the commit I mentioned above). This is a low-level internal routine which writes some text fragment in a given font, size and style at a given position to the page.

According to Volker, this is not really supported.

So what can we do now?

Note: Even if we had to fiddle with the LayoutProcessor settings only at report level instead of text fragment level, this would not remedy the situation. As long as OpenPDF uses global variables, which have to be set this way or that way for different fonts to achieve correct results, it is not really thread-safe.
The old iText library was thread-safe AFAIK, so shouldn't this be a design goal for OpenPDF as well?

[speckyspooky]

If the LayoutProcessor is enabled then the kerning cannot be Set because the processor is already online.
But the LayoutProcessor must be online because the full disabling of the processor is controlled via enabling flag.
So it was my target to do it in this way.

Other ways will not work.

[vk-github18]

The LayoutProcessor is only activated, if you call LayoutProcessor.enable*(). Otherwise it is not active and can not cause problems. Using LayoutProcessor can help for correct rendering of foreign scripts or of Latin letters with accents, see e.g. DIN 91379. You changed BIRT, included FOP in your classpath and activated FOPGlyphProcessor by this. LayoutProcessor is designed to be called at the start with some options and not changed afterwards. It works only with OpenType-Fonts. It should not be necessary to switch it off. There was a bug with legacy fonts, that is fixed in the trunk of OpenPdf and in https://github.com/openpdfsaucer/openpdfsaucer 2.0.9. With this fix legacy processing is always used for legacy fonts. So to be consistent, either A: Don't use LayoutProcessor B: Use LayoutProcessor for all glyph layout and use only OpenType fonts. Again, LayoutProcessor is about glyph layout, it replaces the legacy glyph layout using the HarfBuzz layout engine built into recent Java versions resulting in correct representation of many scripts. Kerning and ligatures are just minor options that can be switched on per font. If LayoutProcessor is disabled, these settings have no effect. See also #2107

[speckyspooky]

The communication is to long.
I invested a lot of time for testing and my solution is working.

And I tested a lot of cases.
I can retest the cases of kerning but it the LayoutProcessor is sometimes misteriouse.

The current way is working for all issues which was addressed.

[hvbtup]

The sad truth about multi-threading is that race conditions occur in production only.
You cannot really test them with reasonable effort.
I bet a box of beer that this will lead to "wrong output" issues in the next 3 years.
And these errors will be very, very hard to reproduce and analyze.

@vk-github18, can you point us to the bug which is fixed in OpenPDF and what change was made in OpenPDFSaucer?

So to be consistent, either
A: Don't use LayoutProcessor
B: Use LayoutProcessor for all glyph layout and use only OpenType fonts.

If I understand the issues I mentioned correctly, then for some reports we need to disable the LayoutProcessor, and for others we need to enable it. So we can choose neither A nor B.

I still think a clean approach would require that OpenPDF supports different LayoutProcessor objects instead of just a class with static attributes.
Basically, the low-level routines which depend on the settings (probably in the PDFContentByte class) could be overloaded with an optional layoutProcessor argument, which BIRT could supply.
This way each BIRT report generation task could use its personal LayoutProcessor settings which don't affect other threads.

[speckyspooky]

There is also e special case where the LayoutProcessor is to be enabled with usage flag "0".
Which means processor is active but without kerning and ligatures.

Because LayoutProcessor.disabled() is not like the same like LayoutProcessor.enable(0).
I don't know if this is a bug or a feature or a special constellation of handling.

[vk-github18]

@vk-github18, can you point us to the bug which is fixed in OpenPDF and what change was made in OpenPDFSaucer?
See https://github.com/openpdfsaucer/openpdfsaucer/pull/40
and LibrePDF/OpenPDF#1159

So to be consistent, either
A: Don't use LayoutProcessor
B: Use LayoutProcessor for all glyph layout and use only OpenType fonts.

If I understand the issues I mentioned correctly, then for some reports we need to disable the LayoutProcessor, and for others we need to enable it. So we can choose neither A nor B.

LayoutProcessor should be able to handle all cases. If you can provide an OpenPDF-only example I can have a look.

[vk-github18]

There is also e special case where the LayoutProcessor is to be enabled with usage flag "0". Which means processor is active but without kerning and ligatures.

Because LayoutProcessor.disabled() is not like the same like LayoutProcessor.enable(0). I don't know if this is a bug or a feature or a special constellation of handling.

You are calling enable(Font.LAYOUT_LEFT_TO_RIGHT), in most circumstances I would just call LayoutProcessor.enable();

And no -- calling disable is not the same as enable.
Again: LayoutProcessor is about glyph layout, substitution, reordering etc. Kerning and ligatures are just minor aspects.

[hvbtup]

@speckyspooky
I created an OpenPDF issue 1307 for this and in that issue Volker pointed to an OpenPDF example GlyphLayoutDocumentKernLigaPerFont.java which shows that the LayoutProcessor can be enabled once on startup and then kerning and ligature support can be specified on a per-font basis.

I think the code from commit 2aef7c4 and other places should be changed in such a way that is uses the same approach as the OpenPDF example, that is, enable or disable the LayoutProcessor only once at the beginning and then enable or disable kerning and/or ligatures using the LayoutProcessor methods LayoutProcessor.setLigatures(font) and LayoutProcessor.setKerning(font).

@vk-github18 Though I find the API not quite clear: Is it disabled by default? I mean, I would expect a boolean argument as well...

[vk-github18]

We should take our time to think about a good solution. The default for Kerning and Ligatures is Off if you call LayoutProcessor.enable(), and can be enabled with the set...() Methods. Switching back and forth is not possible. If you call Layoutprocessor.enableKernLiga() Kerning and Ligatures are on and can not be switched off.

[vk-github18]

Do you need the options Kerning and Ligatures? Or do you just need support for complex scripts? In this case just use LayoutProcessor.enable() Without options.

[vk-github18]

@hvbtup

enable or disable kerning and/or ligatures using the LayoutProcessor methods
LayoutProcessor.setLigatures(font) and LayoutProcessor.setKerning(font).

set means enable here.
These methods should also only called once at the start.

[hvbtup]

OK, so, if I understand everything correctly, the best practice would be:

• Call LayoutProcessor.enable() once at BIRT startup time.
• For kerning and ligatures, the default is false. Use LayoutProcessor.setLigatures(font) and/or LayoutProcessor.setKerning(font) (if the user wants to enable it for a specific font)exactly once for that, when the font is loaded for the first time (I recall vaguely that either OpenPDF or BIRT use a font cache).

For BIRT, this could be implemented as follows:

Have three global properties in fontsConfig.xml:

1. whether the LayoutProcessor shall be enabled at all (the default should be true)

Document that all other settings only work if this first property is true.

2. whether kerning shall be enabled by default (default: false)
3. whether ligatures shall be enabled by default (default: false)

These settings only specify the defaults and can be overridden per font.

For each font, optionally override the defaults from 2. and 3. :
4. Override kerning property from 2.
5. Override ligatures property from 3.

In the code, when a font is used for the first time (and only then):
Check if property 4 is specified for this font. If yes, use this value.
Otherwise, use the value from property 2.
If the value is yes, call LayoutProcessor.setKerning(font), otherwise do nothing.

For ligatures, in the same way, use the values from the properties 5 and 3 and LayoutProcessor.setLigaturesfont).

Never call LayoutProcess.enableKernLiga() or LayoutProcessor.disable().
Note that the properties cannot be set for individual reports, but I guess that's OK.

I'm undecided about whether we need a special handling in the code for symbolic fonts.
Using the configuration options I described above, they can be configured in fontsConfig.xml.
OTOH modifying this file is not always easy, depending on the situation.
So the default for symbol fonts should "just work". If not, then we still need a special handling for these fonts, wich should be switched on by default. For these fonts, instead of the values from properties 2 and 3, the logic should use defaults for kerning and ligatures that just work.

[speckyspooky]

Hi together,
the core problem was that the kerning and ligatures should be disabled to avoid automated handling at all.
So we cannot set by default the of LayoutProcessor to "true".
And there was also special handling of OpenPDF why LayoutProcessor.disable() was not working correctly therefore I used enabled(0).
And of course, the handling of symbolic fonts should be given by default. This was a side effect that I disabled all and the symbolic fonts are not displayed correctly. Which is now fixed.

I'm confused with the description of the comment:

Means this the method can only be called for this specific font 1 times or over all 1 times?

[hvbtup]

I don't mind if the default for the LayoutProcessor is enabled or disabled.

[vk-github18]

Again LayoutProcessor is about glyph layout, substitution etc for complex scripts, not about Ligatures and Kerning, these are just options that can be set if LayoutProcessor is enabled. Kerning and Ligatures can be enabled once per font. Disable works as expected and switches off LayoutProcessor, then you have the old Layout of OpenPdf. LayoutProcessor works with Opentype fonts and ignores other fonts. Possibly you had a problem with a legacy font because of a bug fixed only in the trunk but not in the latest release

[hvbtup]

@vk-github18
AFAIK we (BIRT) cannot use the trunk of OpenPDF, we must use a Maven release.
Do you know when the next release of OpenPDF is to be expected?

[vk-github18]

I am not a maintainer, you should ask asturio or andreasrosdal. There is also a fork openpdfsaucer maintained by andreasrosdal, where this bug is fixed

[merks]

FYI, the latest BIRT nightly build is using today's release of openpdf:

https://repo1.maven.org/maven2/com/github/librepdf/openpdf/2.0.4/

[vk-github18]

@hvbtup
I think, your proposal should work.
If you call LayoutProcessor.enable() as default, you are changing the processing of all reports,
this should work, but is a risk.
To be conservative, you can make the current processing the default, i.e.
call LayoutProcessor.enable() only if the option "complex-scripts" is set.
The options "kerning" and "ligatures" take only effect if Layoutprocessor is enabled and are otherwise ignored.

OpenPdf has a font cache, if you want to load a font more than once with different options, you can switch the cache off for this font.

In OpenType there are no symbolic fonts, these are legacy fonts, that should be ignored by LayoutProcessor.