Enhance the PDF glyph handling to handle advanced kerning and ligatures (#2058) #2059

speckyspooky · 2025-02-23T21:51:35Z

No description provided.

(eclipse-birt#2058)

hvbtup

This may feel like nit-picking, but enabling kerning and ligatures involves a little overhead and of course it changes the width of text, thus possibly resulting in different line-breaking.

There is probably a reason why this is not enabled by default in OpenPDF.
So I think it should not be enabled by default in BIRT, instead it should be an option for the PDF emitter.

speckyspooky · 2025-02-24T08:24:40Z

Ok, make sense, so I will take a look into the emitter configuration.

hvbtup · 2025-02-24T08:36:27Z

Furthermore, enabling kerning and ligatures must be considered in the font width calculation. I don't know if OpenPDF takes this into account.

Besides, it is somewhat strange that OpenPDF has a LayoutProcessor class with static methods.
So using kerning and ligatures is all or nothing and cannot be decided per font instance!?

Decades ago I added kerning and ligature support for the ReportLab Python PDF creation library in my "wordaxe" repo https://deco-cow.sourceforge.net/. However, I only ever tested this for latin languages.

speckyspooky · 2025-02-25T05:02:53Z

I wouldn't see it to negative with the handling of openPDF.
Yes, there is may be a reason way the glyph handling is not enabled by default. I assume that it is more a performance topic instead of handling topic. Because the glyphs was handled before based of the different fonts but not correct add all.

The according article/reference-document of openPDF explain the topic and reference to different examples.
That a static way is used can be a reaction due to the complexity of openPDF and font-handling.

Please be aware the font handling of BIRT isn't easy too and I invested lot of time to figure out why the config-switch wasn't working.

speckyspooky · 2025-02-25T05:20:00Z

@hvbtup
The usage of user property or PDF-configuration isn't possible because the font handling ist complete based at layout-engine level. I mean it is a completely different processing stream instead of the output rendering.

So if we would need a kind of configuration switch we cannot use a user-property/PDF-renderer-config
we would need a kind of global configuration like the ECMA-script-switch-property based on JVM-level.

@wimjongman & @merks
What is your opinion to this siwtch option.

Switch: Yes/No
Global switch?

hvbtup · 2025-02-25T07:38:38Z

If we change things here, we should at least:

Test and document what happens for an extreme text like "AV AV AV AV AV AV AV AV AV AV AV AV AV" when this text is used in a right-aligned paragraph or a justified paragraph.
Test what happens when text is copied from the PDF into a text editor using Windows-1252 encoding when the text contains ligatures like ffl or ff.
Test the tagged output with PAC 2024 if we create PDF/UA (the idea is similar to the previous test, but the input is based on the tagged PDF. Hint: I saw that LayoutProcessor.java contains a method to output text with a boolean argument actualText (probably meant to create an ActualText attribute in the PDF).

I don't say that all of this must work, but we should at least know what works and what not.
And if some of these tests show issues, then IMHO the default should be "no kerning and ligatures".

speckyspooky · 2025-02-25T07:52:46Z

There are mixed some details, the copy of text to editor is only a hint of text display but isn't a test case.
PDF/UA will work due to embedded fonts, I don't see problems there.
The only Thing would be the case with AV and here till my current testing I have seen no effect.

hvbtup · 2025-02-25T08:10:15Z

I agree with Thomas insofar as it it impossible to change the setting per font.
This is because the LayoutProcessor class uses static properties to store state and BIRT is multi-threaded.
If BIRT is used in a multi-threaded fashion, then calling a LayoutProcessor static method in thread A influences the rendering in thread B.
Thus LayoutProcessor is not thread-safe.
A switch should be based on a system property to make it clear that it cannot be changed at runtime.

hvbtup · 2025-02-25T09:18:43Z

Temp.zip

A test report showing that enabling kerning breaks right-aligment and centered alignment, while justified text works.
This report is also a test case for #2057

wimjongman · 2025-02-25T09:58:48Z

@wimjongman & @merks What is your opinion to this siwtch option.

Guys, I am just baffled with your knowledge about this topic. 🙏 Reading you comments, I think the logical thing to do is to make it a global switch which is off by default.

Great work!! ⭐⭐⭐⭐⭐

speckyspooky · 2025-02-25T22:36:37Z

So, I figured out an alternative instead of a global switch with a system property.
I have added a new font-configuration-tag "font-kerning" to the fontsConfig*.xml.
With this configuration the kerning can be enabled or disabled (default: disabled/unused).

This switch can be configured at every font-xml because it is for all setups/configurations the same internal logic,
means all possible configuration will be checked at all font-config-files.

Let me know I can add in addition a system property but I think the config solution should be good enough.

wimjongman · 2025-02-26T08:53:29Z

How do people set this property? Does it have easy access?

hvbtup

Please remove your personal directories from the config file.
I think the wording should be improved.

Apart from that:
The overall code looks good to me and I like the idea of configuring this in fontsConfig.xml.

engine/org.eclipse.birt.report.engine.fonts/fontsConfig.xml

engine/org.eclipse.birt.report.engine.fonts/fontsConfig_pdf.xml

engine/org.eclipse.birt.report.engine.fonts/fontsConfig.xml

hvbtup · 2025-02-26T09:04:47Z

How do people set this property? Does it have easy access?

The fontsConfig.xml file is supposed to be editable, e.g. to add custom TTF font paths, so I would say: Yes, easy enough.

hvbtup · 2025-02-27T08:12:21Z

...irt.report.engine/src/org/eclipse/birt/report/engine/layout/pdf/font/FontMappingManager.java

@@ -54,10 +63,14 @@ public class FontMappingManager {
 			this.fontAliases.putAll(parent.getFontAliases());
 			this.fontEncodings.putAll(parent.getFontEncodings());
 			this.compositeFonts.putAll(parent.getCompositeFonts());
+			if (!this.fontKerningAdvancedUsage)


I think what you mean is: if it is not explicitly defined, get it from parent.
But then the variable should probably be a Boolean object and the test should be fontKerningAdvancedUsae == null.

The property is not the Object Boolaen it is the type boolean which is also located at parent side therefore this compare (and without null)

I think the point is that you can't distinguish the three states:

Explicitly set to false.

Explicitly set to true.

Not set at all.

with a boolean value that can only have two states...

ok, may be I'm really slowly today, my property definition is done with an initial value and this is "false".
and a boolean (type) must have a value so which moment should switch false to "null". (I try only to understand this point.)

Line 66:
My first thought that your intention was "the property should inherit from parent only if not set explicitly".
Then the case "this should not use advanced kerning, even though the parent does" cannot work.
But now I wonder if the condition is necessary at all?
In lines 62 .. 65 the other properties are copied from parent without a condition. So why do you use a condition in line 66?

hvbtup · 2025-02-27T08:12:33Z

...irt.report.engine/src/org/eclipse/birt/report/engine/layout/pdf/font/FontMappingManager.java

 		}
 		this.fontEncodings.putAll(config.fontEncodings);
 		this.searchSequences.putAll(config.searchSequences);
 		this.fontAliases.putAll(config.fontAliases);
+		if (!this.fontKerningAdvancedUsage)


...irt.report.engine/src/org/eclipse/birt/report/engine/layout/pdf/font/FontMappingManager.java

...e.birt.report.engine.emitter.pdf/src/org/eclipse/birt/report/engine/emitter/pdf/PDFPage.java

hvbtup

In PdfPage.java, if I understand it correctly, the LayoutProcessor.enableKernLiga() is called without checking the option in fontsConfig.xml.
This makes the option pointless!?
And since the LayoutProcessor is basically a singleton, I think it makes sense to enable it (or not, depending on the option) at best exactly once, and not inside a frequently called method like PdfPage.drawText.

ligatures (default: unused/off)

hvbtup

I still don't understand the two if statements in FontMappingManager.java, but if it works, so what...

Enhance the PDF glyph handling to handle advanced kerning and ligatures

66cc229

(eclipse-birt#2058)

speckyspooky added the Enhancement Small change to improve the current supported functionality label Feb 23, 2025

speckyspooky added this to the 4.19 milestone Feb 23, 2025

speckyspooky self-assigned this Feb 23, 2025

speckyspooky requested a review from hvbtup February 24, 2025 05:09

hvbtup requested changes Feb 24, 2025

View reviewed changes

wimjongman linked an issue Feb 25, 2025 that may be closed by this pull request

PDF, advanced glyp handling: Khmer Unicode (Hanuman Font) Not Rendering Correctly #2058

Closed

speckyspooky requested a review from hvbtup February 25, 2025 22:31

hvbtup requested changes Feb 26, 2025

View reviewed changes

engine/org.eclipse.birt.report.engine.fonts/fontsConfig.xml Outdated Show resolved Hide resolved

engine/org.eclipse.birt.report.engine.fonts/fontsConfig_pdf.xml Outdated Show resolved Hide resolved

engine/org.eclipse.birt.report.engine.fonts/fontsConfig.xml Outdated Show resolved Hide resolved

speckyspooky force-pushed the enhance_pdf_advanced_glyph_handling_2058 branch 2 times, most recently from 0f2bc1f to 401bfcd Compare February 26, 2025 19:46

speckyspooky requested a review from hvbtup February 26, 2025 20:06

hvbtup reviewed Feb 27, 2025

View reviewed changes

...irt.report.engine/src/org/eclipse/birt/report/engine/layout/pdf/font/FontMappingManager.java Outdated Show resolved Hide resolved

hvbtup reviewed Feb 27, 2025

View reviewed changes

...irt.report.engine/src/org/eclipse/birt/report/engine/layout/pdf/font/FontMappingManager.java Outdated Show resolved Hide resolved

hvbtup reviewed Feb 27, 2025

View reviewed changes

...e.birt.report.engine.emitter.pdf/src/org/eclipse/birt/report/engine/emitter/pdf/PDFPage.java Outdated Show resolved Hide resolved

hvbtup requested changes Feb 27, 2025

View reviewed changes

speckyspooky force-pushed the enhance_pdf_advanced_glyph_handling_2058 branch from 401bfcd to 57fb373 Compare February 27, 2025 19:21

Add the configuration of the advanced font usage for kerning and

05bdc9d

ligatures (default: unused/off)

speckyspooky force-pushed the enhance_pdf_advanced_glyph_handling_2058 branch from 57fb373 to 05bdc9d Compare February 27, 2025 19:50

speckyspooky requested a review from hvbtup February 28, 2025 09:39

hvbtup approved these changes Feb 28, 2025

View reviewed changes

speckyspooky merged commit 4bb9439 into eclipse-birt:master Feb 28, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance the PDF glyph handling to handle advanced kerning and ligatures (#2058) #2059

Enhance the PDF glyph handling to handle advanced kerning and ligatures (#2058) #2059

speckyspooky commented Feb 23, 2025

hvbtup left a comment

speckyspooky commented Feb 24, 2025

hvbtup commented Feb 24, 2025

speckyspooky commented Feb 25, 2025

speckyspooky commented Feb 25, 2025

hvbtup commented Feb 25, 2025

speckyspooky commented Feb 25, 2025

hvbtup commented Feb 25, 2025

hvbtup commented Feb 25, 2025

wimjongman commented Feb 25, 2025 •

edited

Loading

speckyspooky commented Feb 25, 2025

wimjongman commented Feb 26, 2025

hvbtup left a comment

hvbtup commented Feb 26, 2025

hvbtup Feb 27, 2025

speckyspooky Feb 27, 2025

merks Feb 27, 2025

speckyspooky Feb 27, 2025

hvbtup Feb 27, 2025

hvbtup Feb 27, 2025

hvbtup left a comment

hvbtup left a comment

Enhance the PDF glyph handling to handle advanced kerning and ligatures (#2058) #2059

Enhance the PDF glyph handling to handle advanced kerning and ligatures (#2058) #2059

Conversation

speckyspooky commented Feb 23, 2025

hvbtup left a comment

Choose a reason for hiding this comment

speckyspooky commented Feb 24, 2025

hvbtup commented Feb 24, 2025

speckyspooky commented Feb 25, 2025

speckyspooky commented Feb 25, 2025

hvbtup commented Feb 25, 2025

speckyspooky commented Feb 25, 2025

hvbtup commented Feb 25, 2025

hvbtup commented Feb 25, 2025

wimjongman commented Feb 25, 2025 • edited Loading

speckyspooky commented Feb 25, 2025

wimjongman commented Feb 26, 2025

hvbtup left a comment

Choose a reason for hiding this comment

hvbtup commented Feb 26, 2025

hvbtup Feb 27, 2025

Choose a reason for hiding this comment

speckyspooky Feb 27, 2025

Choose a reason for hiding this comment

merks Feb 27, 2025

Choose a reason for hiding this comment

speckyspooky Feb 27, 2025

Choose a reason for hiding this comment

hvbtup Feb 27, 2025

Choose a reason for hiding this comment

hvbtup Feb 27, 2025

Choose a reason for hiding this comment

hvbtup left a comment

Choose a reason for hiding this comment

hvbtup left a comment

Choose a reason for hiding this comment

wimjongman commented Feb 25, 2025 •

edited

Loading