I’ve received reports that PDFs generated by Mu͒seScore when
viewed in Atril (but not in Okular or mupdf!) have the top
of the treble clef cut off. I found that the same happens
for glyphs of the font I use for URLs and copyright notices
(attached), but only from Mu͒seScore, not from LibreOffice.

I first reported this to Atril both because I couldn’t find
anything wrong with the font metrics (or after fixing what
could have been, I tried multiple variants) upstream at
https://github.com/mate-desktop/atril/issues/610 where a
helpful soul found that there’s some clip path in the PDFs
which Atril honours that clips off the rendering but that
under that the glyphs are fully present.

Then I dug into the Mu͒seScore Studio source code, but could
not find anything suspicious, so I extracted an MWE from its
source code (under GPLv2 though I doubt the example below is
actually nōntrivial enough to be copyrightable) to generate
a PDF and, voilà, the top of the text is cut off in Atril but
not in mupdf.

I’m attaching the MWE (qex.pro + qex.cc) and the font file
(SIL OFL, currently the .otf is the source) so you can
reproduce this. It reproduces for me on stretch, bullseye and
sid, probably universally. (Make sure $DISPLAY is set. Put the
.otf into ~/.local/share/fonts/ to test.)

$ QT_SELECT=5 qmake qex.pro && make && ./qex
$ mupdf moo.pdf
$ atril moo.pdf

Please forward this bug upstream as suitable. Thanks in advance!
(I don’t have the means to test with upstream’s versions.)

Hi Thorsten!

Strangely, I cannot reproduce this on my Trixie system, neither with the PDF
generated by your test program, nor with the PDFs attached to your Atril bug
on GitHub.

I tried opening the PDFs in Evince (my default viewer), Atril and Inkscape
with Poppler import; in all three they were displayed good. I am attaching
screenshots of how your dd.pdf looks in Atril and how your moo.pdf looks in
Inkscape, with a list of objects seen on the right. I do not see anything
being cut.

What Poppler version do you have? I have libpoppler-glib8t64 22.12.0-2.2+b1,
maybe your version is older?

I am also attaching moo.pdf generated by your test program with Qt 6.6.2
from experimental. Can you check if it has the same problem on your system?
That would be useful information for filing an upstream bug.

Dixi quod…

Further debugging reveals the cause:

When Qt5 embeds a font, it scales it to 2048 ppem, no matter if
it was 1000 ppem (PS/CFF) or 1024 ppem (TTF) before. I think this
is because [QTBUG-586] it cannot embed CFF fonts, so it always
converts to TTF (apparently even if it was TTF already).

The conversion is done not incompetently, the new metrics are
correct… but it only changes the metrics in the head table, not
in the OS/2 or hhea tables (as can be seen when loading the font
from the PDF in FontForge). Furthermore, the /FontBBox in the PDF
is constructed from the values from the original font.

I can confirm that if I prescale the font to 2048 ppem, ceteris
paribus, the bug no longer appears.

QPdfEnginePrivate::embedFont calls font->toTruetype()
but later uses font->fontEngine->properties() for metrics.

QFontSubset::toTruetype does the conversion to 2048 ppem
and others, saying it does not need to add an OS/2 table,
only to later copy the one from the original font if present
without adjusting its values.

bye,
//mirabilos

Dixi quod…

And Atril uses the values from the hhea table (found by hexediting).

#define TO_TTF(x) qRound(x * 2048. / ppem)

This is used in things like…

    font.hhea.maxAdvanceWidth = TO_TTF(fontEngine->maxCharWidth());

… but not in…

    font.hhea.ascender = qRound(properties.ascent);
    font.hhea.descender = -qRound(properties.descent);
    font.hhea.lineGap = qRound(properties.leading);

… and of course not when the OS/2 table is copied:

    if (!noEmbed) {
        QTtfTable os2;
        os2.tag = MAKE_TAG('O', 'S', '/', '2');
        os2.data = fontEngine->getSfntTable(os2.tag);
        if (!os2.data.isEmpty())
            tables.append(os2);
    }

(all examples from stretch’s Qt, as the oldest I had at hand)

bye,
//mirabilos

Actually, this code dates back to “Initial import from the monolithic Qt”
commit from 2011. The only thing that changed is that MAKE_TAG macro was
replaced with QFont::Tag class.

I checked Qt 4 history [1] and there this code dates back to “Long live Qt!”
commit from 2009. So it’s unlikely that we can find the original developer
and ask why it is like that and whether we actually need the OS/2 table.

Now that you dug so deeply, maybe you can try to replace qRound in those
three lines that you mentioned with TO_TTF, and check if it fixes the bug
(and does not break anything else)?

[1]: https://github.com/qt/qt/blame/4.8/src/gui/text/qfontsubset.cpp

Hi Dmitry,

(you use Googlemail, which is problematic, I picked your reply
from the BTS; perhaps send to 1070406-submitter@b.d.o instead
which should arrive)

Thanks for checking.

Yeah. Some renderers might benefit from it in some cases probably.
It contains about 17 or so values that need to be scaled, and it
has multiple versions… I could probably write something…

Chances are extremely slim that fixing the metrics will break
anything ;-)

That *and* figure out somehow how to fix the PDF /FontBBox, at
least… (though I binary-patched the hhea ascender in the PDF and
it made Atril happy, so it “should”, despite the still-wrong OS/2
table values some of which are notably used in clipping by some
softwares…)

I think I can try that, though my weekend’s about up by now. I’d
try it with one of the versions (most likely bullseye’s) if that
is okay for you.

For the Mu͒seScore side, things get trickier though as they likely
won’t be able to obtain fixed Qt builds for all platforms. I wonder
whether it would be possible to subclass and just override the
faulty code (or do post-fixups somehow) and patch it to just do
that (if the bug is still present in the used Qt version)… I’m not
even a C++ programmer… *sigh*

bye,
//mirabilos

Dixi quod…

I’m trying this (attached). That does both (by letting toTruetype()
adjust the already-existing scaling factor in the caller) and
applies suitable rounding (normal for normal values, away from zero
for the bounding box so it’s guaranteed to encompass all possible
values). I’ll build it now so I don’t know if it even compiles yet…

bye,
//mirabilos

Dixi quod…

    font.hhea.ascender = TO_TTF(properties.ascent.toReal());
    font.hhea.descender = TO_TTF(-properties.descent.toReal());
    font.hhea.lineGap = TO_TTF(properties.leading.toReal());

Dixi quod…

Yes, that worked. I’m attaching the final patch in entirety again
for your convenience.

It still does not address the OS/2 table, but it does manage to
fix both the PDF-side and font-side hhea table metrics, which is
enough for Atril at least. (Not sure whether it’s enough for my
gf’s printer, I’ll have to test. Or extend the patch to fix the
OS/2 table as well, which I probably should anyway; I have to find
the time for that sometime within the next few days.)

bye,
//mirabilos

No, the only way I've seen them sold is for $40 with a free OpenBSD CD.
	-- Haroon Khalid and Steve Shockley in gmane.os.openbsd.misc

Hi Thorsten!

Sorry for that. Using -submitter this time.

Thank you for the patch! I left some inline comments below.

Will you be able to forward your patch upstream when you finalize it?
Upstream Qt requires a CLA so they do not usually like when someone submits
a patch for which they are not the author.

The process is described in Qt wiki [1][2] but basically you need to create
an account in Gerrit, add a commit hook to generate Change-Id header, add
gerrit remote, and git push gerrit HEAD:refs/for/dev.

[1]: https://wiki.qt.io/Setting_up_Gerrit
[2]: https://wiki.qt.io/Gerrit_Introduction

I see you are passing negated values to this function, e.g. roundForBbox(-x).
If you move the negation outside of the function call, maybe you can just
use ceil? E.g. ceil(x) and -ceil(x).

I see why you moved properties to the top, but is moving postscriptName
needed? Maybe leave it where it was to minimize the diff?

You are passing scalep pointer here only to multiply it by this value?

It looks like fontEngine is a public member of QFontSubset, so you can do it
in the calling code.

I can't find any references to something that look like a "OS/2" table in the
pdf spec.

Assuming the font subset code only deal with subsetting for pdf's, maybe it
can go completely?

Just to help me double check, how is is the OS/2 table described in the font
in the pdf ?

/Sune

Dmitry Shachnev dixit:

Sort of. I can do the CLA, Gerrit, etc. dance, but I probably cannot
respond well if they want me to test things with vanilla upstream
(instead of the packaging), or if they have requests I as a C (but
not C++) developer don’t understand.

(The other half of fixing things is even more challenging. I got a
confirmation that Mu͒seScore Evolution upstream cannot upgrade their
Qt versions so they’ll need a different fix subclassing and overriding
the library functions for some cases. If anyone sufficiently proficient
in C++ is interested in helping with that, once we got the fix for Qt
itself going, ping me. Alternatively, helping to figure out how to patch
and rebuild the exact versions they use for Win/Mac or upgrade their
versions without losing Win7 and qtwebengine, IIUC — AIUI their Mac OSX
builds are still at Qt 5.9 for that reason…)

Not only them. And roundForBbox is basically just the canonical
“round away from zero / towards ±infinity for integer results”.

The negation in the callers is to convert *some* Qt coordinates
to PS/PDF coordinates, but only for the Y axis, as the X axis is
the same for them.

It’s not. I moved the whole block to make it easier to read,
but good point.

Yes.

Yes, except it’s the callee that determines the scaling factor,
not the caller. By passing it back like this, nothing would have
to change in the caller should the callee ever decide to not scale.


Sune Stolborg Vuorela dixit:

That’s because we’re talking about OTF/TTF-format tables here.

The problem exists at two different layers.

One is that, when embedding and subsetting a font, Qt generates
a PDF font descriptor whose bounding box is wrong.

The other is that, when embedding and subsetting a font, Qt
converts it to quadratic-spline TTF and scales it to 2048 ppem,
then embedding the resulting TTF in the data object corresponding
to the above-mentioned font descriptor (the data object itself,
when extracted, is a fully functional .ttf font file). The OTF/TTF
file format has description tables, and Qt does not correctly adjust
all values in all tables it includes when scaling the font itself.

The references I’ve been using are:

PDF: https://opensource.adobe.com/dc-acrobat-sdk-docs/pdfstandards/pdfreference1.0.pdf
TTF: https://learn.microsoft.com/en-us/typography/opentype/spec/

The OS/2 table specifically is documented at:
https://learn.microsoft.com/en-us/typography/opentype/spec/os2

bye,
//mirabilos

But with Qt 6.6 from experimental there should not be much difference:
we don't have any patches for this part of code, and we are lagging only
a few versions behind upstream.

And your test example compiles without changes with Qt 6, you just need
to call qmake6 instead of qmake.

Okay, makes sense.

Thank you.

Valid point. Let's see if Qt developers like this approach with passing a
pointer. If they don't, maybe the same thing can be achieved differently,
e.g. by returning QPair<QByteArray, qreal>.

But with Qt 6.6 from experimental there should not be much difference:
we don't have any patches for this part of code, and we are lagging only
a few versions behind upstream.

And your test example compiles without changes with Qt 6, you just need
to call qmake6 instead of qmake.

Okay, makes sense.

Thank you.

Valid point. Let's see if Qt developers like this approach with passing a
pointer. If they don't, maybe the same thing can be achieved differently,
e.g. by returning QPair<QByteArray, qreal>.

Dmitry Shachnev dixit:

Ah, good to know.

Thanks! That helps.

C++ can return multiple values like that? Wow, TIL.
That’s certainly useful, I’ll try that instead, then.

… it will have to wait until the weekend probably, though.

bye,
//mirabilos