808288 - <canvas> text drawing does not handle missing glyphs properly

Status: REOPENED

(Core :: Graphics: Canvas2D, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase for rendering missing-glyphs in <canvas>

In HTML content, when a character occurs for which no font can be found, we put a missing-glyph record in the textrun, which is rendered as a "hexbox" placeholder.

However, the canvas drawing code does not handle this, with the result that it treats the *character* code of the missing-glyph record as though it were a *glyph id* in the current font. This results in an incorrect glyph being rendered (quite likely the font's .notdef, which may be blank or a box, but it may result in a visible but completely wrong character appearing).

There are several possible fixes, listed here in likely order of complexity. Any one of these would be an improvement over the current behavior:

(1) simply don't render a glyph for missing-glyph records, so they remain blank in canvas;

(2) render glyph ID 0, the standard ".notdef" glyph slot, which will most often be either blank or a simple placeholder "box", though this is font-dependent;

(3) render a hexbox, like we do in HTML text.

The attached testcase uses the characters U+0528..052F, which are unassigned in Unicode (as of v6.2), and so are not expected to be found in any fonts; they should result in hexboxes in HTML content, but when drawn to <canvas>, they appear as unrelated, "arbitrary" glyphs depending on the particular fonts involved.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: rendering of the testcase on OS X 10.7

Note how the unassigned characters are rendered as completely unrelated glyphs in <canvas>, with different results in each of the fonts used. (Results are likely to vary across systems/versions, depending on the exact glyph palette of the fonts.)

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - leave missing glyphs blank in <canvas> text, rather than rendering arbitrary (incorrect) glyphs

This implements the simplest option - just leave the missing glyphs blank, advancing the x-position but not rendering anything.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - render the font's .notdef glyph for missing glyphs in <canvas>

This extends the preceding patch to render "missing" glyphs as glyph ID zero, which is defined as the .notdef glyph in truetype/opentype fonts. It could, in principle, be the wrong thing in other font technologies, but in practice it's a pretty safe bet. (I think we should do at least this, and preferably go the whole way to the hexbox option.)

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - draw missing-glyph hexboxes for <canvas> text, similar to missing glyphs in HTML text

And this replaces the use of .notdef (glyph 0) with code to call the gfxFontMissingGlyphs code that renders bitmap hexbox representations of the character code. This would make canvas text behave more like HTML text, and is potentially the most helpful to an author trying to diagnose problems with their page.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 678024 [details] [diff] [review]
part 3 - draw missing-glyph hexboxes for <canvas> text, similar to missing glyphs in HTML text

Review of attachment 678024 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3040,5 @@
>            mTextRun->GetDetailedGlyphs(i);
>  
>          if (glyphs[i].IsMissing()) {
> +          float xpos;
> +          float advance = detailedGlyphs[0].mAdvance * devUnitsPerAppUnit;

In the previous patch we only took detailedGlyphs[0].mAdvance into account for LTR test, I'm guessing this change doesn't matter though.

@@ +3053,5 @@
> +          // we don't have to draw the hexbox for them
> +          if (advance > 0) {
> +            Matrix matrix = mCtx->mTarget->GetTransform();
> +            nsRefPtr<gfxContext> thebes;
> +            if (gfxPlatform::GetPlatform()->SupportsAzureContent()) {

This is a little tricky, potentially this could go wrong if we fell back to a Cairo context because of for example extremely large Canvas size. Cairo content is technically -not- supported, but SupportsAzureContent would return true as the browser uses D2D content.

I'm thinking Cairo content will be good enough to do DrawMissingGlyph though, so I'm not too worried, we should probably add a comment though, and keep an eye out.

@@ +3067,5 @@
> +            gfxFloat height = font->GetMetrics().maxAscent;
> +            gfxRect glyphRect(xpos, baselineOrigin.y - height,
> +                              advance, height);
> +            gfxFontMissingGlyphs::DrawMissingGlyph(thebes, glyphRect,
> +                                                   detailedGlyphs[0].mGlyphID);

I'd rather have seen us use Azure-code directly here, but I understand why that's a hard thing to do, this is pretty terrible though! But oh well.

Comment 6

[Jonathan Kew [:jfkthame]]

Yeah, I realize this isn't ideal, but I didn't want to completely reimplement gfxFontMissingGlyphs just for this. Eventually I expect we'll want to replace it and clean up the ugliness here.

Comment 7

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f356d9b2524c
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e143317082
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eec0a5b76ff

Comment 8

[Ed Morley [:emorley]]

Backed out for:
REFTEST TEST-UNEXPECTED-PASS | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/canvas/text-emoji.html | image comparison (!=), max difference: 255, number of differing pixels: 204
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5eec0a5b76ff

https://hg.mozilla.org/integration/mozilla-inbound/rev/a08d5817fe26

Comment 9

[Jonathan Kew [:jfkthame]]

Ah yes - it makes perfect sense that test will no longer fail. Relanded, with the annotation in the reftest manifest updated accordingly:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58b4bd7b5065

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/58b4bd7b5065

Comment 11

[Masatoshi Kimura [:emk]]

Partially backed out because of bug 837461.

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Jonathan Kew (:jfkthame) from bug 837461 comment #18)
> (It's not really a good solution, though, as users might not even realize
> that some of the text in their document isn't showing up.)

I agree. We should render something to inform the user that something went wrong.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Reland part 2 & 3, but use .notdef glyph instead of hexbox when HWA is disabled

It looks like this patch doesn't regress 837461. (Of course empty box is rendered. But we should fix the root cause of failing to load the font, instead of suppress the rendering.)

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: Reland part 2 & 3, but use .notdef glyph instead of hexbox when HWA is disabled

Forgot to add a bug number in the comment.

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 717443 [details] [diff] [review]
Reland part 2 & 3, but use .notdef glyph instead of hexbox when HWA is disabled

Review of attachment 717443 [details] [diff] [review]:
-----------------------------------------------------------------

Alternatively, how about if we just reland part 2 for now? That should be "safe" in that it'll only draw the .notdef glyph, not hexboxes.

The problem with part 3 is that because of how pdf.js deals with font scaling via transforms on the context, it ends up drawing the hexbox at a hugely inflated size. I think we should try to address that by correctly handling the transforms involved, then we can re-land the hexbox support for all platforms, rather than having behavior that differs depending on the platform.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2365,5 @@
> +          }
> +          advanceSum += advance;
> +          // Do not draw hexbox when HWA is disabled to avoid bad
> +          // rendering by pdf.js (see bug 837461)
> +          if (!gfxPlatform::GetPlatform()->SupportsAzureContent()) {

Wouldn't this disable the hexbox on all non-Windows platforms, too? Is that actually necessary? (The comment doesn't match the code, as HWA may be enabled yet Azure Content disabled, AFAIK.)

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 717443 [details] [diff] [review]
Reland part 2 & 3, but use .notdef glyph instead of hexbox when HWA is disabled

OK, let's reland part 2.

Comment 19

[Masatoshi Kimura [:emk]]

Comment on attachment 678023 [details] [diff] [review]
part 2 - render the font's .notdef glyph for missing glyphs in <canvas>

This patch still applies cleanly.

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 678023 [details] [diff] [review]
part 2 - render the font's .notdef glyph for missing glyphs in <canvas>

OK, I think it's fine for this to re-land.

It means that in cases where pdf.js fails to load a font and hence draws "missing" glyphs, it is likely to get a box rather than blank, which may make certain examples of problem PDFs look a bit worse (but it won't produce the huge ugly black blobs reported in bug 837461).

The real solution to that is for pdf.js to fix up the font resources so that they load successfully, not to hide the problem (and thus mislead the user into thinking they're seeing a complete document, when in fact parts are missing) by skipping those glyphs altogether.

Comment 21

[Masatoshi Kimura [:emk]]

Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a99a6d3c09

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/34a99a6d3c09