Created attachment 141118 [details]
How it should look like

Created attachment 141119 [details]
Test file as rendered but current poppler 0.67

Created attachment 141120 [details] [review]
Patch

Created attachment 141121 [details]
Test file as rendered with the patch

Created attachment 141166 [details] [review]
Updated patch

Updated patch: Fixes the 'unknown font type' problem, and the case that the 'Font' subdirectory of the 'Resources' directory is a reference.  The rendering output appears unchanged.

Moving/resizing the FreeText annotation from the test document in Okular causes a crash for me with this patch. Can you reproduce it?

(In reply to oliver.sander from comment #0)
> - The text is moved a little bit towards the top of the page (notice how the
>   top part of "This is a test" is cut off).
As far as I can tell by now, we mustn't relate getDescent() to fontSize when calculating the baseline offset from top of the bounding box (see Tm + Td translation). It's inaccurate. It was also wrong for the formerly hardcoded Helvetica font, you just didn't notice it that much. While font size relates to the em square, ascent+descent is more or less out of the em square. How much out depends on the font. I don't know which metrics to use instead, have to learn more. What would you expect to fit vertically exact in the first line (and so determines the baseline offset from top)?
- capital glyphs from font (/CapHeight)?
- the glyph with highest ascender in the whole font (/Ascent), potentially much higher then capital glyphs?
- the highest glyph that takes actually part in the first line?
- something else?
- huh?

> Moving/resizing the FreeText annotation from the test document in Okular
> causes a crash for me with this patch. Can you reproduce it?

Yes.  I will have a look.

> > - The text is moved a little bit towards the top of the page (notice how the
> [snip]
> translation). It's inaccurate. It was also wrong for the formerly hardcoded
> Helvetica font, you just didn't notice it that much.

If it was wrong before, can we simply disregard the problem here and treat it in a separate bug report / patch?

Here is some initial explanation of why moving the annotation in Okular causes a crash:  After the move, Okular calls the method TextAnnotation::setTextFont with the argument

  QFont( "Noto Sans,12,-1,0,50,0,0,0,0,0,Regular" )

Then, the method TextAnnotationPrivate::toAppearanceString is called to get an appearance string from that.  However, the method is only a stub:

GooString * TextAnnotationPrivate::toAppearanceString(const QFont &font)
{
    GooString * s = GooString::format("/Invalid_font {0:d} Tf", font.pointSize());
    // TODO: Font family, style (bold, italic, ...) and pointSize as float
    return s;
}

The actual crash then results because the font tag 'Invalid_font' cannot be found in the font resource dictionary.

I'll modify the patch to not crash when the font tag cannot be found.  However, there are still a few things I don't understand, namely a) why is setTextFont only called after the annotation has been moved, but not before, b) How do we get the /Impact tag from that particular QFont object?

Created attachment 141188 [details] [review]
Patch

Update the patch: moving the annotation in Okular now does not make the program crash anymore.  Not surprising though, moving the annotation makes the font switch to the default font again.  I consider this as a separate bug, though, and propose to treat it in a separate bug report.

(In reply to oliver.sander from comment #8)
> a) why is setTextFont only called after the annotation has been moved,
> but not before
Document load and page render are read only operations, no annotation setters are required there. But when you modify the slightest bit of an annotation, the whole okular annotation model is synced to poppler using the various setters. Thereby we discard the original appearance. That's long existing okular/poppler behavior (and has some related bugs).

> b) How do we get the /Impact tag from that particular QFont object?
That'll get a long story, hopefully out of scope of this patch. Initially there won't be a QFont for Impact at all, because available QFonts are those found by Qt on your system. E.g., IMPACT.ttf is available on Windows, but typically not on Linux. We can only render it because IMPACT.ttf got embedded into the PDF by Adobe Reader. Maybe we can register a new QFont for the embedded font with QFontDatabase::addApplicationFontFromData [0]. Next problem is, that we can't get PostScript name (QFont::family != PostScript name), and we can't get filename from a QFont.

http://doc.qt.io/qt-5/qfontdatabase.html#addApplicationFontFromData

(In reply to oliver.sander from comment #9)
> Not surprising though, moving the annotation makes
> the font switch to the default font again.
IMO that's as good as it gets for now. Preserving the original font upon modification via Qt frontend can be targeted in a future patch.

(In reply to oliver.sander from comment #7)
> > > - The text is moved a little bit towards the top of the page (notice how the
> > [snip]
> > translation). It's inaccurate. It was also wrong for the formerly hardcoded
> > Helvetica font, you just didn't notice it that much.
> 
> If it was wrong before, can we simply disregard the problem here and treat
> it in a separate bug report / patch?

I'd vote for doing it in another patch, to keep the scope here clear. The only problem is, that we can't really file it as bug against poppler master, because as long as only Helvetica is used, the bug never comes to light.

@Albert: Are you reading here? Any suggestion from your side?

I've looked at Helvetica metrics: Ascent=718, Descent=207 => 1000-207=793. So with Helvetica there's always enough space for ascenders and capitals, even if the assumption Ascent+Descent=1000 is buggy here, too.

The new patch should probably
1) use Ascent to calculate baseline offset from top margin
2) double check if the fixed scaling of ascent/descent by 0.001 in GfxFont::readFontDescriptor is really correct. FontForge tells me that IMPACT.ttf has em size of 2048 (while most fonts have 1000). Seems we should scale with 1/2048 instead of 1/1000. That would explain the overly high Ascent value I observe for /Impact.

> I'd vote for doing it in another patch, to keep the scope here clear.

Agreed.  @Albert, are there objections to getting this patch merged now?

> The only problem is, that we can't really file it as bug against poppler master, because as long as only Helvetica is used, the bug never comes to light.

We wait for this patch to be merged, and file the bug afterwards.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/6.