1888084 - Small accents in PDF are not rendered in high zoom levels on macOS

Status: ASSIGNED

(Firefox :: PDF Viewer, defect, P3)

Description

[mhaug]

Attachment: missing-accent.pdf

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.4 Safari/605.1.15

Steps to reproduce:

Open Firefox on macOS Sonoma 14.4. Using its integrated pdf.js viewer, open a PDF file containing an accented character like the attached file and zoom in, for example to 300%.

The accent must not be part of the character's glyph and be drawn with its own text drawing operation in the content stream. In the examples we discovered the malfunction in, the font was CFF-based.

This bug does not reproduce on Windows or Fedora Linux.

Actual results:

Starting at some zoom level, the accent will vanish and stay vanish, if zooming farther in. There will be a zoom level which marks the border between the accent being visible and invisible.

Expected results:

Like pdf.js on Windows and Linux, and all other tested PDF viewers, the accent should be visible at all zoom levels.

Comment 1

[mhaug]

As an addition on platforms: I was able to reproduce this on ARM and x64_86 Mac computers.

Comment 2

[Calixte Denizet (:calixte)]

I can reproduce the bug on Mac OS 14.3.1 but not on Windows 11.
If I open the pdf in: https://mozilla.github.io/pdf.js/web/viewer.html
then it works correctly in Chrome but not in Firefox.
:jfkthame, would you have any ideas ?

Comment 3

[Calixte Denizet (:calixte)]

Attachment: The font file extracted from the pdf.

Comment 4

[Calixte Denizet (:calixte)]

Attachment: The generated font used in the canvas

Comment 5

[Jonathan Kew [:jfkthame]]

Strange.... I'm not sure what's happening here. For me, the accent (dot) disappears between 100% and 110% zoom on my high-dpi (retina) screen, but if I move the window to a low-dpi display then it disappears between 210% and 240%. So the threshold seems to be related to the device-pixel size involved.

It feels like maybe this has something to do with the accent being a zero-width glyph that is painted in isolation (since pdf.js draws each glyph with a separate fillText call), and at some level in the rendering stack we're clipping our painting area to the advance width instead of allowing for the actual bounds of the glyph outline. But I'm not sure where something like that would be happening, or why only at larger sizes.

Comment 6

[Calixte Denizet (:calixte)]

On mac, if I set gfx.canvas.accelerated to false then the dot is correctly displayed.
On Windows, the dot is correctly drawn whatever the above pref is and whatever the zoom level is.
:lsalzman, could you have a look please ?

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Jonathan Kew [:jfkthame] from comment #5)

Strange.... I'm not sure what's happening here. For me, the accent (dot) disappears between 100% and 110% zoom on my high-dpi (retina) screen, but if I move the window to a low-dpi display then it disappears between 210% and 240%. So the threshold seems to be related to the device-pixel size involved.

It feels like maybe this has something to do with the accent being a zero-width glyph that is painted in isolation (since pdf.js draws each glyph with a separate fillText call), and at some level in the rendering stack we're clipping our painting area to the advance width instead of allowing for the actual bounds of the glyph outline. But I'm not sure where something like that would be happening, or why only at larger sizes.

We just use SkFont::getBounds (via DrawTargetSkia::GetGlyphLocalBounds) to do this, and we always round out with the result. So I can't really conceive of a situation where we inadvertently create zero bounds from nothing. Do you have a way of seeing if Skia is doing the wrong thing here in https://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#1362 which is being called from https://searchfox.org/mozilla-central/source/dom/canvas/DrawTargetWebgl.cpp#4339 ?

I don't really have a working mac to test with at the moment, and so far my attempts to reproduce this at all on Linux have been entirely futile.

Comment 8

[Daniel Bodea [:danibodea]]

Reproduced in MacOS 11 in all the latest versions of the four channels. I confirm this report.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Lee Salzman [:lsalzman] from comment #7)

Do you have a way of seeing if Skia is doing the wrong thing here in https://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#1362 which is being called from https://searchfox.org/mozilla-central/source/dom/canvas/DrawTargetWebgl.cpp#4339 ?

I checked the bounds that are being reported here, and they look sensible to me. Some numbers from the SharedContextWebgl::DrawGlyphsAccel code, with the pdf displayed at 100% (on a high-dpi screen, so effectively 200%), when the accent renders correctly:

This is the accent glyph:

bounds -7.000000 -12.000000 x 5.000000 4.000000
xformBounds 789.903381 186.835083 x 9.166626 7.333328
clipBounds 789 186 x 11 9

and this is the letter 'x':

bounds -1.000000 -9.000000 x 11.000000 11.000000
xformBounds 783.479370 192.100433 x 20.166626 20.166672
clipBounds 783 192 x 21 21

After zooming to 110% (i.e. 220% after accounting for the retina screen), when the accent no longer renders:

Accent:

bounds -7.000000 -12.000000 x 5.000000 4.000000
xformBounds 868.893677 205.518875 x 10.083313 8.066666
clipBounds 868 205 x 11 9

Letter 'x':

bounds -1.000000 -9.000000 x 11.000000 11.000000
xformBounds 861.827271 211.310745 x 22.183350 22.183334
clipBounds 861 211 x 24 23

So it's not here that things are going wrong.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:calixte, could you have a look please?

For more information, please visit BugBot documentation.

Comment 11

[mhaug]

Would it be a good idea to transfer the bug to Core / Graphics to assign a triage owner closer to the root of the problem?

Comment 12

[Calixte Denizet (:calixte)]

:mhaug, yep you're right, I'm moving this bug since it's unlikely a pdf.js issue but a canvas one.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:lsalzman, could you have a look please?

For more information, please visit BugBot documentation.

Comment 14

[Lee Salzman [:lsalzman]]

Jonathan, do we know if maybe the accent is actually rendering outside the inflated bounds for some reason, with the offset maybe getting worse as the size gets bigger? That would be my best guess here.

Could you try altering line https://searchfox.org/mozilla-central/source/dom/canvas/DrawTargetWebgl.cpp#4376 to have a bigger inflated margin and see if that makes this magically work again?

Comment 15

[Jonathan Kew [:jfkthame]]

Huh, that's an interesting thought. I experimented, and confirmed that changing Inflate(2) here to Inflate(5), for example, makes the dropout happen at a larger zoom level; Inflate(10) even more so, and with Inflate(20) the accent remains visible all the way to 1000% zoom.

Obviously that's a hack rather than a proper fix, but it's a good clue. I wonder if what we're getting as a starting point for the bounds might simply be the glyph's typographic bounds rather than its ink bounds? Then we inflate a bit, and at small sizes that inflation is enough to "catch" the ink, but at larger sizes it is increasingly offset.

Comment 16

[Jonathan Kew [:jfkthame]]

It's not about typographic vs ink bounds, at least not here: the typographic bounds of the accent would be zero-width (before being inflated), but the bounds we're getting are (correctly) non-zero-width.

After a bit more poking at this, I don't believe this is the level where things are failing. To visualize the glyph bounds we're working with there in DrawTargetWebgl, I applied this debugging hack:

diff --git a/dom/canvas/DrawTargetWebgl.cpp b/dom/canvas/DrawTargetWebgl.cpp
--- a/dom/canvas/DrawTargetWebgl.cpp
+++ b/dom/canvas/DrawTargetWebgl.cpp
@@ -4373,7 +4373,8 @@ bool SharedContextWebgl::DrawGlyphsAccel
       IntPoint(newOffset.x / quantizeScale.x, newOffset.y / quantizeScale.y);
   // Ensure there is a clear border around the text. This must be applied only
   // after clipping so that we always have some border texels for filtering.
-  intBounds.Inflate(2);
+  const int kBoundsInflation = 5;
+  intBounds.Inflate(kBoundsInflation);
 
   RefPtr<TextureHandle> handle = entry->GetHandle();
   if (handle && handle->IsValid()) {
@@ -4405,6 +4406,21 @@ bool SharedContextWebgl::DrawGlyphsAccel
         textDT->FillRect(Rect(IntRect(IntPoint(), intBounds.Size())),
                          ColorPattern(DeviceColor(1, 1, 1, 1)),
                          DrawOptions(1.0f, CompositionOp::OP_OVER));
+        // Show the bounds of the target we're using.
+        textDT->StrokeRect(Rect(IntRect(IntPoint(1, 1),
+                                        IntSize(intBounds.width - 2,
+                                                intBounds.height - 2))),
+                         ColorPattern(DeviceColor(0, 0, 1, .5)),
+                         StrokeOptions(),
+                         DrawOptions(1.0f, CompositionOp::OP_OVER));
+        // Also stroke the uninflated bounds, to visualize what we think the glyph
+        // bounds should have been.
+        textDT->StrokeRect(Rect(IntRect(IntPoint(kBoundsInflation, kBoundsInflation),
+                                        IntSize(intBounds.width - 2 * kBoundsInflation,
+                                                intBounds.height - 2 * kBoundsInflation))),
+                         ColorPattern(DeviceColor(1, 0, 0, .5)),
+                         StrokeOptions(),
+                         DrawOptions(1.0f, CompositionOp::OP_OVER));
       }
       textDT->SetTransform(currentTransform *
                            Matrix::Translation(-intBounds.TopLeft()));

to draw visible rects both for the overall size of the temporary drawtarget where we're painting the glyph, and the original bounds that we think it should have. Increasing the kBoundsInflation value makes the accent continue to render at larger zoom levels, and we can see that the drawtarget is getting more and more padding around the actual glyph. But we can also see that the original (uninflated) bounds were correct all along: the painted accent is consistently within the inner rectangle.

Comment 17

[Jonathan Kew [:jfkthame]]

I think this is a bug inside Skia, triggered because the actual outline of the accent glyph is positioned entirely to the left of the glyph origin. My guess is that Skia is looking at the area of the canvas to be painted, and at the nominal position of the glyph (whose origin is beyond the right-hand edge of the canvas), and concluding that it can skip painting it. But that's wrong, because although the glyph's origin is beyond the edge of the canvas, its ink (which is significantly to the left of the origin) should still be visible.

If I comment out the call to internalQuickReject(bounds, paint) here, the accent renders correctly at all sizes, showing that the problem comes from Skia's overenthusiastic rejection of the painting operation, because it thinks the glyph is outside the canvas.

Comment 18

[Calixte Denizet (:calixte)]

:jfkthame, aren't we using Skia on windows ? because I can't reproduce the bug on Windows 11 whatever the pref gfx.canvas.accelerated is.

Comment 19

[Jonathan Kew [:jfkthame]]

Yeah, it doesn't reproduce for me on Windows either, nor on Linux. Maybe there are platform differences in Skia regarding how it handles glyph bounds? Or maybe there's some other reason we aren't hitting the same code path.

Comment 20

[Jonathan Kew [:jfkthame]]

It seems that the generated font used in the canvas (comment 4) has incorrect bounding box data in it, and that's what eventually leads to the accent glyph getting clipped at larger sizes (not at smaller sizes because Skia adds a bit of padding, and at small sizes that's enough to save us).

Taking the font file and dumping with TTX, the <head> table includes:

    <xMin value="0"/>
    <yMin value="-3060"/>
    <xMax value="4095"/>
    <yMax value="3560"/>

The first suspicious thing is the xMin value being zero, because we know that the dot-accent glyph is actually placed entirely to the left of the origin. So the font must have a significantly negative xMin.

Using TTX to recompile the dump into a new OpenType font, and then re-dumping this, we get a very different result:

    <xMin value="-318"/>
    <yMin value="-11"/>
    <xMax value="527"/>
    <yMax value="677"/>

A few values elsewhere in the font have also changed (been re-computed by TTX based on the actual glyphs); the advanceWidthMax, minRightSideBearing, and xMaxExtent fields in <hhea> are modified, as is the FontBBox in the <CFF> data, but I believe it is the <head> table bounding box that's leading to the problem here.

It's not clear to me yet whether the original font data in the PDF is wrong (and pdf.js isn't checking/fixing it in the process of extracting and converting the font, like TTX does when I decompile/recompile), or if the error is happening during the pdf.js conversion itself. Calixte, can you look into how those <head> table bounding box values are derived and see if there's anything suspicious there?

Comment 21

[Calixte Denizet (:calixte)]

We're writing the header here:
https://github.com/mozilla/pdf.js/blob/master/src/core/fonts.js#L3222-L3224
and astonishingly, for whatever font we're setting xMin to 0 and xMax to 4095 ! (and this code is there for a while: https://github.com/mozilla/pdf.js/pull/143/files#diff-3f7caf410c66f6298c5c704369ddbf06aa1ef33ebf35bb058f98fb6032c0c22bR917-R919)
The ascent and the descent are taken from the font bbox present in the cff header (/FontBBox [-1044 -3060 4088 3560]). We should take the xMin and the xMax from the bbox we've either in the font or in the pdf, but sure we shouldn't use those default values.
I just tested on my mac and replacing 0 by safeString16(-1044) and 4095 by safeString16(4088) fixes the issue.
:lsalzman, :jfkthame, thank you for your help.

Comment 22

[Jonathan Kew [:jfkthame]]

Hah, that would explain it! Yeah, using the FontBBox should make things much better.

(FWIW, the FontBBox in this example is actually a lot bigger than it needs to be; the true FontBBox would be [-318 -11 527 677]. I suspect the value we see here is the original bbox from before the font was subset to leave just the dot and x glyphs. But that's harmless; at least it won't result in glyphs being lost. And it's not worth iterating over all the glyphs to recompute an accurate bbox just for this purpose.)