Bug 107584

Summary: Font disregarded in Freetext annotation with default appearance (DA) but without appearance stream (AP)
Product: poppler Reporter: oliver.sander
Component: generalAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED MOVED QA Contact:
Severity: normal    
Priority: medium CC: oliver.sander
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Test file with freetext annotation without appearance stream
How it should look like
Test file as rendered but current poppler 0.67
Patch
Test file as rendered with the patch
Updated patch
Patch

Description oliver.sander 2018-08-15 16:23:26 UTC
Created attachment 141117 [details]
Test file with freetext annotation without appearance stream

Attached is an example file with a freetext annotation.  This annotation has a default appearance (DA) but no appearance stream (AP).  Poppler disregards the font of the annotation.  While admittedly the file has been constructed by hand-editing (annotating the file in Adobe Reader and then manually removing the /AP part), it is nevertheless a standard-conforming file.

Fixing this issue is a necessary first step towards fixing https://bugs.freedesktop.org/show_bug.cgi?id=81748 .

I attach a preliminary patch which kind-of fixes the issue.  'Kind-of' only, because there are (at least) two issues with it:
- poppler issues

  "Syntax Warning: Unknown font type: '???'"

  from the call to 'GfxFont::makeFont' in Annot.cc:2933

- 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).

The fact that poppler renders the text in the wrong color (red instead of black) is a separate bug.


Test file and patch have been written by Tobias Deiminger, who deserves all the praise.
Comment 1 oliver.sander 2018-08-15 16:24:22 UTC
Created attachment 141118 [details]
How it should look like
Comment 2 oliver.sander 2018-08-15 16:25:21 UTC
Created attachment 141119 [details]
Test file as rendered but current poppler 0.67
Comment 3 oliver.sander 2018-08-15 16:26:35 UTC
Created attachment 141120 [details] [review]
Patch
Comment 4 oliver.sander 2018-08-15 16:27:03 UTC
Created attachment 141121 [details]
Test file as rendered with the patch
Comment 5 oliver.sander 2018-08-17 10:17:23 UTC
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.
Comment 6 Tobias Deiminger 2018-08-17 21:55:34 UTC
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?
Comment 7 oliver.sander 2018-08-18 07:39:32 UTC
> 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?
Comment 8 oliver.sander 2018-08-18 11:17:58 UTC
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?
Comment 9 oliver.sander 2018-08-18 20:57:01 UTC
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.
Comment 10 Tobias Deiminger 2018-08-19 09:08:11 UTC
(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.
Comment 11 Tobias Deiminger 2018-08-19 09:27:34 UTC
(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.
Comment 12 oliver.sander 2018-08-20 07:14:58 UTC
> 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.
Comment 13 GitLab Migration User 2018-08-20 21:33:17 UTC
-- 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.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.