fix #281601 and fix #284218 [workaround]: broken on-screen rendering … #5794

AntonioBL · 2020-03-08T17:51:44Z

…of synthetically emboldened fonts

Resolves: https://musescore.org/en/node/284218 and https://musescore.org/en/node/281601

See https://musescore.org/en/node/281601#comment-900261 for the explanation of the Qt bug involved.
This is a workaround.

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
[N/A] I created the test (mtest, vtest, script test) to verify the changes I made

AntonioBL · 2020-03-08T17:53:26Z

Important note: this PR must be thoroughly tested to check if it does not re-introduce kerning problems (especially under Windows).

libmscore/textbase.cpp

Jojo-Schmitz · 2020-03-08T17:55:16Z

libmscore/textbase.cpp

+            // only needed for certain artificially emboldened fonts
+            // see https://musescore.org/en/node/281601#comment-900261
+            // in Qt 5.12.x this workaround should be no more necessary if
+            // env variable QT_MAX_CACHED_GLYPH_SIZE is set to 1


Would it make sense then to use #if (QT_VERSION < QT_VERSION_CHECK(5, 12, 0))?

Maybe, but we should also check that it is possible to defined QT_MAX_CACHED_GLYPH_SIZE=1 inside MuseScore code (maybe before all the initializations) and that it works without slowing the program (in my pc it does not slow down the program, but better safe than sorry).
This comment is just a reminder that in the future (if and when we will use a newer version of Qt) the workaround could be no more needed.

libmscore/textbase.cpp

dmitrio95 · 2020-03-08T19:04:37Z

Thanks for the workaround and for the explanation you have linked! I have also noticed that if zoom factor in QPainter's world transformation is close to 1 then no issue occurs, but I didn't know the reason of that.

However I am not sure that it would be correct to use this workaround. The reason of my concern is that changing font size actually changes various font properties and metrics in the way which doesn't directly correspond to scaling glyphs of fixed size. One of important aspects of that is spacing between letters being dependent on font size so scaling font size by some factor doesn't actually scale size of a text block by the same factor.

In MuseScore this leads to this workaround leading to noticeable shifts in relative letters positions and occasional collisions of text with other elements when changing zoom level (which is especially noticeable when changing zoom level with Ctrl + mouse wheel). These occasional collisions result from layout assuming font size different to what is used by drawing procedure with this workaround. The implementation in this PR limits this effect to bold text only but independently of font face, so even text using fonts not suffering from the emboldening issues will show the behaviour which I described above.

The part with collisions should probably be resolved if layout would use the same font size as with this workaround, but this also may be not the correct way to handle text layout. It might be arguable and dependent on a particular context, but the primary purpose of layout in MuseScore editor (at least in page mode) seems to be to show the appearance which would be the same as what would be produced after printing the score on a paper. If we accept this purpose of layout as the main one in this context, then the correct way to handle text layout seems to be what is implemented in MuseScore right now: using font size suitable for paper printing and scaling text by pixel transformations rather than by changing font size.

As of Qt 5.12, my self-builds with Qt 5.12.5 (on Linux) still show this emboldening issue, but setting QT_MAX_CACHED_GLYPH_SIZE=1 indeed fixes the issue. I didn't check its effect on text painting performance though but could do that if needed.

dmitrio95 · 2020-03-08T19:06:29Z

By the way, this issue also seems to belong to the same set of font emboldening issues.

AntonioBL · 2020-03-08T19:47:10Z

@dmitrio95 : you are right: I can see this spacing shift when zooming in and out.
I expected that scaling the text dimension and the coordinate system by the same amount would lead to the same text appearance: why isn't it so?
I remember a problem of kerning under Windows being related to an integer truncation, but that was happening only under Windows (see https://musescore.org/en/node/117191 and #3298 ), and I basically was using a similar approach (I don't remember the fine details at the moment: it was a long work of trial and error to find a working solution). I strongly feared that this PR could reopen that bug, but you say it is misbehaving also under Linux. 😞

By the way, issue https://musescore.org/en/node/279766 is exactly this bug.

I don't know if it is possible to somehow identify the problematic fonts.

mirabilos · 2020-03-08T19:53:15Z

Dmitri Ovodok dixit:

As of Qt 5.12, my self-builds with Qt 5.12.5 (on Linux) still show this emboldening issue, but setting `QT_MAX_CACHED_GLYPH_SIZE=1` indeed fixes the issue. I didn't check its effect on text painting performance though but could do that if needed.

Has anyone contacted the Qt developers about this? This seems like an issue we’d not be the only ones to have, and which they might know a proper fix, now we’re aware of a (version-dependent) workaround.

AntonioBL · 2020-03-08T21:34:35Z

Well, I tried three times to ask for help in Qt forum these years, but they answered only once.
So I am not confident about having an answer from their side.
In case it is needed for further investigations, here is a minimal example, which can be compiled via QtCreator.
font_bold.zip

mirabilos · 2020-03-09T13:04:52Z

Howard Chang dixit:

+#if defined Q_OS_WIN || defined Q_OS_LINUX Yeah you're right. I thought this was unfamiliar because normally I see this: `#if (defined (_MSCVER) || defined (_MSC_VER))`

That’s also irregular.

So I think it's probably better to have parentheses.

Standard style is: #if defined(Q_OS_WIN) || defined(Q_OS_LINUX) But, as we said, we almost certainly want this instead: #ifndef Q_OS_MAC

AntonioBL · 2020-05-04T19:05:28Z

I think I managed to make the workaround work.
Tests are welcome, as well as suggestions to improve the PR. 😉

AntonioBL · 2020-05-04T19:30:25Z

Mmmh, the dimension is not yet the correct one, as seen in the vtests, but I think this could be the correct way of takling this bug (hopefully).

AntonioBL · 2020-05-05T20:26:48Z

Ok. I found that this workaround works for screen painting only, but not for png, pdf or svg export. In those cases a further rescaling involving screen dpi and painting device dpi is involved, but I could not find something which perfectly works in all cases (and by perfectly I mean 100% superposition).
By the way, text drawing is a real mess, especially when comparing output between different operating systems: text position is slightly different between the three operating systems for example when exporting a pdf.

libmscore/textbase.cpp

Jojo-Schmitz · 2020-05-06T07:26:06Z

The remaining small vtest differences might well be due to some floating point rounding going on?

mirabilos · 2020-05-06T09:02:16Z

libmscore/textbase.cpp

+            qreal dx = p->worldTransform().dx();
+            qreal dy = p->worldTransform().dy();
+            qreal factor = 1 / mm; // correction factor for bold text drawing
+            p->setMatrix(QMatrix(mm * factor, 0.0, 0.0, mm * factor, dx, dy));


Isn’t mm * factor = 1 ?

due to floating point arithmetic not exactly. But should be... so also might be the cause for the remaining small offsets. Guess p->setMatrix(QMatrix(1.0, 0.0, 0.0, 1.0, dx, dy)); would be more correct here?

Yes. It is a leftover because at the beginning I was manually adjusting the factor to see when the problem was disappearing.
I found that the problem is indeed the fact that the diagonal elements of the painter matrix are not 1. If you look at exported output in the current MuseScore version, you will see that for svg there is no problem (m11 is forced to 1); for pdf and png when the resolution is 360 or larger there is no problem (m11 is greater or equal to 1, since it is resolution / internal resolution and the internal resolution at the moment is 360).
Well, for pdf the synthetically emboldened fonts are not emboldened, so there is not actually a problem (if we can say so) for m11<1.
For png export, when the resolution is lower than 360, for some font dimensions we see the problem.

I was wondering if maybe use this workaround only when painting to the screen (I also don't know what happens when using two screens at different resolutions), and let the export function unchanged. What do you think?

MuseScore doesn't deal too well with 2 screens with different DPIs already (it always only takes the settings of the primary screen as far as I can tell), I don't expect this PR to change that

mirabilos · 2020-05-06T09:23:45Z

Joachim Schmitz dixit:

due to floating point arithmetic not exactly. But should be...

Yeah, but mathematically, so we could inline this, perhaps as: p->setMatrix(QMatrix(/* mm * factor */ 1, 0.0, 0.0, /* mm * factor */ 1, dx, dy));

Jojo-Schmitz · 2020-05-06T09:36:03Z

Yeah, but mathematically, so we could inline this, perhaps as: p->setMatrix(QMatrix(/* mm * factor / 1, 0.0, 0.0, / mm * factor */ 1, dx, dy));

Yes, better for documentation. I'd use 1.0 though

AntonioBL · 2020-05-06T09:37:50Z

By the way, have you tried the artifacts? Do they work ok for you?

Jojo-Schmitz · 2020-05-06T09:40:52Z

Well, your 2nd commit didn't generate artifacts

AntonioBL · 2020-05-06T10:00:13Z

Ouch, I forgot the collect_artifacts. 😅
The new build should create them.

AntonioBL · 2020-05-06T11:25:37Z

You see problem in exported images only when the resolution is (significantly) below 360.
You can try for example 200 or 100 dpi.

Jojo-Schmitz · 2020-05-06T11:26:36Z

before (at 96DPI, same as screen, to not compare apples to oranges ;-)):

after:

mirabilos · 2020-05-06T11:45:32Z

Antonio Lotti (ABL) dixit:

(Freetype), but it seems that Qt text framework is a little buggy when using Freetype (the one at hand is one example; another one

Indeed, exporting the same text from LibreOffice (using Freetype) works, the distance between the two copies of the glyph visible in the 2.3.2 export is filled black here.

Maybe a good candidate for GSoC 2021 could be implementing a new text drawing engine (for score texts, not for all the program texts) independent of Qt

OK, but please make that still use the (system) freetype library, ideally with fontconfig, so that the user settings for that (what kind of kerning (autohinter, bytecode, etc.) and which fonts to allow with which substitutions etc.) are still honoured. I currently use that to ensure only fonts with a licence suitable for embedding end up in the PDF (glyph substitution can choose an arbitrary one from the system otherwise). Too bad the Qt people appear to not be interested (you said some‐ thing about them not having responded to inquiries regarding this)…

mirabilos · 2020-05-06T11:45:32Z

Also http://www.mirbsd.org/music/free/resources/Gentium-1.03.zip and then “Gentium Italic”…

Jojo-Schmitz · 2020-05-06T11:48:50Z

Would a 170DPI screen count as high (enough) for testing? (it is for reproducing the issue)

AntonioBL · 2020-05-06T12:09:36Z

OK, but please make that still use the (system) freetype library, ideally with fontconfig, so that the user settings for that (what kind of kerning (autohinter, bytecode, etc.) and which fonts to allow with which substitutions etc.) are still honoured. I currently use that to ensure only fonts with a licence suitable for embedding end up in the PDF (glyph substitution can choose an arbitrary one from the system otherwise). Too bad the Qt people appear to not be interested (you said some‐ thing about them not having responded to inquiries regarding this)…

One additional problem of Qt is that even when they solve a bug, most of the time they do not backport it to older Qt versions (such as Qt 5.9.x that we are using for MuseScore).

I think MuseScore is already using Freetype to draw score symbols, but texts have additional difficulties (kerning, ligatures, etc...). Qt is using Harfbuzz for text shaping, but I don't know how complicate a text engine can become.

AntonioBL · 2020-05-06T12:11:30Z

Would a 170DPI screen count as high (enough) for testing? (it is for reproducing the issue)

Maybe, no idea. I think the most problematic cases could be those that require the "-x" ("--gui-scaling") option.

mirabilos · 2020-05-06T12:33:07Z

Antonio Lotti (ABL) dixit:

texts have additional difficulties (kerning, ligatures, etc...). Qt is

Not to mention combining characters, like I use ɔ plus ⃝ to fake, as ɔ⃝, the copyleft sign, and this ends… somewhat badly, http://www.mirbsd.org/music/free/Pachelbel%20--%20Canon%20in%20D.pdf merely has it not centred in the footer, but when editing that file without limiting MuseScore to a certain set of fonts, these two are rendered one after another, like: ɔ ⃝

Jojo-Schmitz · 2020-05-06T13:29:27Z

Would a 170DPI screen count as high (enough) for testing? (it is for reproducing the issue)

Maybe, no idea. I think the most problematic cases could be those that require the "-x" ("--gui-scaling") option.

Looks good there too, not using those options

…of synthetically emboldened fonts

AntonioBL · 2020-05-06T17:03:00Z

I limited the workaround only for the cases in which it could be really necessary. Unfortunately, I don't know how to limit it only to misbehaving fonts (so it is applied to all bold fonts when not exporting a vectorial format and when m11 is < 1).
I also changed the default pdf export dpi to the internal rendering dpi, so that m11 = 1 in that case (similar to svg export case). I don't think it will change much, since pdf should already be a vectorial file (and dpi for pdf export does not make much sense, except if rasterizing the output file).

AntonioBL · 2020-05-08T16:49:01Z

I found a better way to deal with font rescaling between the different dpi involved.
I was misled by Qt documentation https://doc.qt.io/qt-5/qtextlayout.html#QTextLayout-3 the function already exists for Qt < 5.13 (but the QPaintDevice is not const).

mirabilos · 2020-05-08T21:07:25Z

Aww yes: https://doc.qt.io/qt-5.12/qtextlayout.html#QTextLayout-2

This kind of documentation issue is… troubling.

And so much thanks for working on it!

mirabilos · 2020-05-08T21:10:35Z

mscore/preferences.cpp

@@ -130,7 +130,7 @@ void Preferences::init(bool storeInMemoryOnly)
            {PREF_EXPORT_MP3_BITRATE,                              new IntPreference(128, false)},
            {PREF_EXPORT_MUSICXML_EXPORTBREAKS,                    new EnumPreference(QVariant::fromValue(MusicxmlExportBreaks::ALL), false)},
            {PREF_EXPORT_MUSICXML_EXPORTLAYOUT,                    new BoolPreference(true, false)},
-            {PREF_EXPORT_PDF_DPI,                                  new IntPreference(300, false)},
+            {PREF_EXPORT_PDF_DPI,                                  new IntPreference(DPI, false)},


Doesn’t this change the default dpi for the PDF export from 300 to 360?

If so, at least https://github.com/musescore/MuseScore/blob/master/build/Linux%2BBSD/mscore.1.in#L209 needs adjusting, and maybe the QCommandLineArg thingy used for --help as well.

That line in the manual page is about PNG resolution and that has been changed to 360 a while ago already

I wonder whether changing PDF's DPI is the right way, IMHO printers usually default to 300 DPI?

Well, pdf is a vectorial format, so dpi does not really make sense for it (except if it is rasterizing things as images).
By using pdf dpi equal to internal DPI, we ensure that the matrix elements are 1 and therefore no problems with fonts should appear.

Ah, yes, the DPI is only used for embedded images I guess.

mirabilos · 2020-05-08T21:33:30Z

Thar line in the manual page is about png resolution

Oh right, sorry.

and that has been changed to 360 a while ago already

Free manpage fix for you ☻

Jojo-Schmitz · 2020-05-09T07:29:53Z

Free manpage fix for you ☻

See #6066

anatoly-os · 2020-06-02T22:10:36Z

@AntonioBL Do I understand correctly that the workaround is not needed with let's say Qt 5.12.8 and higher?

AntonioBL · 2020-06-03T07:59:17Z

@AntonioBL Do I understand correctly that the workaround is not needed with let's say Qt 5.12.8 and higher?

In principle, the workaround is not needed for Qt >=5.12 if we set QT_MAX_CACHED_GLYPH_SIZE=1 during the program initialization with qputenv (if we do not change the zoom logic).

Maybe the best thing to be done would be to change the zoom logic so that the diagonal elements of the painter are always 1.0 (the coordinate system is not scaled); I think this could also help for Windows font rendering and maybe, once this is implemented, we could return to native font engine (instead of FreeType engine; this would help for loading DRM-protected fonts). However, that would be a huge reworking (with possible additional side-effect bugs).

Jojo-Schmitz reviewed Mar 8, 2020

View reviewed changes

libmscore/textbase.cpp Outdated Show resolved Hide resolved

Jojo-Schmitz reviewed Mar 8, 2020

View reviewed changes

libmscore/textbase.cpp Outdated Show resolved Hide resolved

Jojo-Schmitz reviewed Mar 8, 2020

View reviewed changes

libmscore/textbase.cpp Outdated Show resolved Hide resolved

AntonioBL force-pushed the boldfont branch from 2fd9cad to 5f23944 Compare March 8, 2020 18:04

AntonioBL force-pushed the boldfont branch from 5f23944 to e76691a Compare March 8, 2020 20:45

AntonioBL marked this pull request as draft April 20, 2020 08:56

AntonioBL force-pushed the boldfont branch from e76691a to b56b0f1 Compare May 4, 2020 18:50

AntonioBL changed the base branch from master to 3.x May 4, 2020 18:57

AntonioBL marked this pull request as ready for review May 4, 2020 19:04

AntonioBL marked this pull request as draft May 5, 2020 20:45

AntonioBL force-pushed the boldfont branch from b56b0f1 to ed4cbc5 Compare May 5, 2020 20:48

Jojo-Schmitz reviewed May 6, 2020

View reviewed changes

libmscore/textbase.cpp Outdated Show resolved Hide resolved

mirabilos reviewed May 6, 2020

View reviewed changes

fix #281601 and fix #284218 [workaround]: broken on-screen rendering …

d5e7249

…of synthetically emboldened fonts

AntonioBL force-pushed the boldfont branch from 8d34176 to 17c0677 Compare May 6, 2020 16:55

AntonioBL marked this pull request as ready for review May 6, 2020 17:03

AntonioBL mentioned this pull request May 6, 2020

MuseScore 3.5 Beta. PRs to be merged #6027

Closed

12 tasks

change default pdf dpi export to rendering value + collect_artifacts

1cf3237

AntonioBL force-pushed the boldfont branch from 17c0677 to 1cf3237 Compare May 8, 2020 16:45

mirabilos reviewed May 8, 2020

View reviewed changes

anatoly-os merged commit eb33bfa into musescore:3.x Jun 6, 2020

AntonioBL deleted the boldfont branch June 9, 2020 07:33

AntonioBL mentioned this pull request Jun 23, 2020

MuseScore 3.5 Release Candidate. PRs to be merged #6195

Closed

1 task

Jojo-Schmitz mentioned this pull request Jul 22, 2020

[MU4 Issue] PRs from 3.x to be merged into / ported to master #6079

Closed

29 tasks

anatoly-os added a commit that referenced this pull request Aug 20, 2020

Manually merged #5794 to master

2116e06

AntonioBL mentioned this pull request Jan 10, 2021

fix #315363 : Workaround for bad on-screen rendering of emboldened fo… #7212

Merged

7 tasks

MarcSabatella mentioned this pull request Nov 23, 2022

[MU4 Issue] MuseJazz bold renders poorly in some situations #14435

Open

fix #281601 and fix #284218 [workaround]: broken on-screen rendering … #5794

fix #281601 and fix #284218 [workaround]: broken on-screen rendering … #5794

Conversation

AntonioBL commented Mar 8, 2020

AntonioBL commented Mar 8, 2020

Jojo-Schmitz Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrio95 commented Mar 8, 2020

dmitrio95 commented Mar 8, 2020

AntonioBL commented Mar 8, 2020

mirabilos commented Mar 8, 2020 via email

AntonioBL commented Mar 8, 2020

mirabilos commented Mar 9, 2020 via email

AntonioBL commented May 4, 2020

AntonioBL commented May 4, 2020

AntonioBL commented May 5, 2020

Jojo-Schmitz commented May 6, 2020

Choose a reason for hiding this comment

Jojo-Schmitz May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirabilos commented May 6, 2020 via email

Jojo-Schmitz commented May 6, 2020 • edited Loading

AntonioBL commented May 6, 2020

Jojo-Schmitz commented May 6, 2020

AntonioBL commented May 6, 2020

AntonioBL commented May 6, 2020

Jojo-Schmitz commented May 6, 2020 • edited Loading

mirabilos commented May 6, 2020 via email

mirabilos commented May 6, 2020 via email

Jojo-Schmitz commented May 6, 2020 • edited Loading

AntonioBL commented May 6, 2020

AntonioBL commented May 6, 2020

mirabilos commented May 6, 2020 via email

Jojo-Schmitz commented May 6, 2020

AntonioBL commented May 6, 2020

AntonioBL commented May 8, 2020

mirabilos commented May 8, 2020

Choose a reason for hiding this comment

Jojo-Schmitz May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirabilos commented May 8, 2020 via email

Jojo-Schmitz commented May 9, 2020 • edited Loading

anatoly-os commented Jun 2, 2020

AntonioBL commented Jun 3, 2020

Jojo-Schmitz Mar 8, 2020 •

edited

Loading

Jojo-Schmitz May 6, 2020 •

edited

Loading

Jojo-Schmitz commented May 6, 2020 •

edited

Loading

Jojo-Schmitz commented May 6, 2020 •

edited

Loading

Jojo-Schmitz commented May 6, 2020 •

edited

Loading

Jojo-Schmitz May 8, 2020 •

edited

Loading

Jojo-Schmitz commented May 9, 2020 •

edited

Loading