906521 - Update SVG-in-OpenType implementation to new spec draft

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Remove support for 'glyphchar' since it's no longer in the spec

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Change from using the 'glyphid' attribute to using id='glyphNNN'

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: OTS should recognize version 0 of the SVG table

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: gfxSVGGlyphs should also check the version because it also does table parsing

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: Make OTS recognize the new table format

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: Make gfxSVGGlyphs understand the new table format

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These patches can't land until someone updates the tests to the new format, and I haven't done that yet.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 7: Handle cases where an SVG glyph has no frame or a non-SVG frame without crashing in debug builds

This could be in a separate bug, but I'm lazy.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've created a simple Web app for live editing of the SVG table in an OpenType font, using the new format:
https://github.com/rocallahan/svg-opentype-workshop
Just clone it and open index.html (or put it on a Web server).

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 792153 [details] [diff] [review]
Part 2: Change from using the 'glyphid' attribute to using id='glyphNNN'

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +365,2 @@
>      nsresult rv;
>      uint32_t glyphId = glyphIdStr.ToInteger(&rv);

If I'm reading the code correctly (ok, I didn't test!), then I think ToInteger will be too lenient here - I believe it will ignore whitespace and leading zeroes, for example, so that it would allow id="glyph 123" or even id="glyph 0000099", which should NOT be recognized according to the spec.

I assume it would also silently ignore trailing garbage after the number, which is also more lenient than we should be. The spec calls for

  id “glyph<glyphID>”, where <glyphID> is the glyph ID expressed as a non–zero-padded decimal value

and we should accept that and nothing else.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 792152 [details] [diff] [review]
Part 1: Remove support for 'glyphchar' since it's no longer in the spec

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

Seems a pity - I found it quite convenient for authoring. But if the general consensus is to drop it, I don't care enough to push back on it.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 792156 [details] [diff] [review]
Part 4: gfxSVGGlyphs should also check the version because it also does table parsing

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

As long as these fonts are only being deployed via @font-face, it wouldn't be an issue, but yes - we should check this, especially in case we have locally-installed fonts some day.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 792157 [details] [diff] [review]
Part 5: Make OTS recognize the new table format

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

Some of the issues here may be things that should be tightened up in the spec rather than relaxed in the code, but one way or another they need to be resolved so that the spec and the code are in closer agreement.

::: gfx/ots/src/svg.cc
@@ +37,5 @@
> +    NONFATAL_FAILURE("Couldn't read doc index offset from SVG table header");
> +  }
> +  if (doc_index_offset != 10) {
> +    NONFATAL_FAILURE("Invalid doc index offset");
> +  }

This assumes the doc index immediately follows the header, but the spec doesn't appear to require that; a tool could put the color palettes first, and then the document index. There's no reason to enforce a more rigid order here, AFAICS.

@@ +45,5 @@
> +    NONFATAL_FAILURE("Couldn't read color palettes offset from SVG table header");
> +  }
> +  if (color_palettes_offset != 0) {
> +    NONFATAL_FAILURE("Color palettes not supported");
> +  }

I don't think this belongs at the OTS level. If we don't yet support part of the SVG table spec, it's the responsibility of the gfxSVGGlyphs code to deal with that issue (whether by falling back to rendering without that feature, if feasible, or ignoring the table altogether). But OTS should only drop the table if it's actually ill-formed.

@@ +57,5 @@
> +  table.set_offset(doc_index_offset);
> +  uint16_t index_length;
> +  if (!table.ReadU16(&index_length)) {
> +    NONFATAL_FAILURE("Couldn't read SVG documents index");
> +  }

The spec requires that index_length > 0, so we should check that.

@@ +100,1 @@
>      }

Hmm, the spec as currently written (Sairus's draft of 2013/07/24) does not appear to require this - I don't see any text there that would prohibit a tool deciding, for example, to pad each document to a 4-byte boundary. Either the spec should explicitly require packing the documents with no gaps, or we should relax this check.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Seems a pity - I found it quite convenient for authoring. But if the general
> consensus is to drop it, I don't care enough to push back on it.

I'd like svg-opentype-workshop to grow a feature where you can enter a string of characters in a text box and then when you press "Create Template" we'll look up the cmap and build a template with glyph entries for all the characters in your string. For bonus points the template for each glyph would be its actual outline :-). I think that would make authoring just fine.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> ::: gfx/ots/src/svg.cc
> @@ +37,5 @@
> > +    NONFATAL_FAILURE("Couldn't read doc index offset from SVG table header");
> > +  }
> > +  if (doc_index_offset != 10) {
> > +    NONFATAL_FAILURE("Invalid doc index offset");
> > +  }
> 
> This assumes the doc index immediately follows the header, but the spec
> doesn't appear to require that; a tool could put the color palettes first,
> and then the document index. There's no reason to enforce a more rigid order
> here, AFAICS.

Ah, I misunderstood the existing code --- I thought we already required exact packing, but we don't.

I don't want to pass through color palettes right now because I don't know what the format is going to be and I didn't want to try to write code for them.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> (In reply to Jonathan Kew (:jfkthame) from comment #13)
> > ::: gfx/ots/src/svg.cc
> > @@ +37,5 @@
> > > +    NONFATAL_FAILURE("Couldn't read doc index offset from SVG table header");
> > > +  }
> > > +  if (doc_index_offset != 10) {
> > > +    NONFATAL_FAILURE("Invalid doc index offset");
> > > +  }
> > 
> > This assumes the doc index immediately follows the header, but the spec
> > doesn't appear to require that; a tool could put the color palettes first,
> > and then the document index. There's no reason to enforce a more rigid order
> > here, AFAICS.
> 
> Ah, I misunderstood the existing code --- I thought we already required
> exact packing, but we don't.

We may well have assumed that, but if it's not specified in the current merged spec, then OTS shouldn't insist on it.

> 
> I don't want to pass through color palettes right now because I don't know
> what the format is going to be and I didn't want to try to write code for
> them.

Can't we handle that by simply ignoring them (for now) in the gfxSVGGlyphs code?

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 792158 [details] [diff] [review]
Part 6: Make gfxSVGGlyphs understand the new table format

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

::: gfx/thebes/gfxSVGGlyphs.h
@@ +132,5 @@
>      } *mHeader;
>  
> +    const struct DocIndex {
> +        mozilla::AutoSwap_PRUint16 mNumEntries;
> +    } *mDocIndex;

I found it a bit confusing to have both mDocIndex and mIndex fields in the updated code. Given the spec, it redundant to have both of these, as they have a clearly-defined fixed relationship. So I'd prefer to describe the structure using something like

  typedef struct {
    mozilla::AutoSwap_PRUint16 mStartGlyph;
    mozilla::AutoSwap_PRUint16 mEndGlyph;
    mozilla::AutoSwap_PRUint32 mDocOffset;
    mozilla::AutoSwap_PRUint32 mDocLength;
  } IndexEntry;

  const struct DocIndex {
    mozilla::AutoSwap_PRUint16 mNumEntries;
    IndexEntry mEntries[1]; /* actual length = mNumEntries */
  } *mDocIndex;

(and then we'd simply pass &mDocIndex->mEntries[0] as the base address to bsearch in FindOrCreateGlyphsDocument).

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 792159 [details] [diff] [review]
Part 7: Handle cases where an SVG glyph has no frame or a non-SVG frame without crashing in debug builds

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

Looks fine; out of curiosity, do we understand what circumstances lead to this?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> > I don't want to pass through color palettes right now because I don't know
> > what the format is going to be and I didn't want to try to write code for
> > them.
> 
> Can't we handle that by simply ignoring them (for now) in the gfxSVGGlyphs
> code?

I suppose we can if we don't require tight packing. I'll do that.

(In reply to Jonathan Kew (:jfkthame) from comment #17)
>   const struct DocIndex {
>     mozilla::AutoSwap_PRUint16 mNumEntries;
>     IndexEntry mEntries[1]; /* actual length = mNumEntries */
>   } *mDocIndex;

Good idea.

(In reply to Jonathan Kew (:jfkthame) from comment #18)
> Looks fine; out of curiosity, do we understand what circumstances lead to
> this?

Not 100% sure how I hit this ... I was in the middle of editing an SVG document (changing a <rect> to a <circle> :-) ) and we crashed. But it can certainly happen; for example a glyph element could be an HTML element, or it could be display:none.

Comment 20

[Bill Gianopoulos [:WG9s]]

I am now including the patches on this bug in my nightly experimental builds available at http://www.wg9s.com/mozilla/firefox/  They are Windows Linux and Android only (sorry Roc no Mac build capability)

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 v2

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5 v2

I've removed the doc_location stuff because just as there is no language requiring documents to be contiguous, there's none requiring them to not overlap.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6 v2

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 8: fix tests

I deleted some tests that are only relevant to "glyphchar".

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9: Add tests for invalid "id" attributes

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 794455 [details] [diff] [review]
Part 2 v2

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +374,5 @@
> +      }
> +      if (ch == '0' && i == glyphPrefixLength) {
> +        return;
> +      }
> +      id = id*10 + (ch - '0');

Eww... I don't really like the manual conversion loop here; that sort of thing should be handled by a library function. But I suppose converting the string to 8-bit chars and using strtol() (and then checking that it consumed the whole string) would end up being just as verbose.

So, OK I guess.

Spaces around the * operator, please.

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 794474 [details] [diff] [review]
Part 5 v2

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

::: gfx/ots/src/svg.cc
@@ +31,5 @@
>  
> +  uint32_t doc_index_offset;
> +  if (!table.ReadU32(&doc_index_offset)) {
> +    NONFATAL_FAILURE("Couldn't read doc index offset from SVG table header");
> +  }

How about checking that doc_index_offset (and color_palettes_offset below) doesn't exceed length?

That would avoid the risk of an arithmetic overflow in the ReadNNN() functions after set_offset(); I don't think length itself can be anywhere close to overflowing as OTS rejects crazy-large tables early on, IIRC.

Comment 28

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Created attachment 794474 [details] [diff] [review]
> Part 5 v2
> 
> I've removed the doc_location stuff because just as there is no language
> requiring documents to be contiguous, there's none requiring them to not
> overlap.

OK for now, I guess, although I think we probably want to add that back to the spec (it used to be in our version); there's no legitimate use case (AFAICS) for overlapping documents, and it would seem to imply a mangled table.

I'm hoping to have time to look over a few spec issues with Sairus, after which we may want to revisit this.

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 794493 [details] [diff] [review]
Part 6 v2

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +56,5 @@
> +        const DocIndex* docIndex = reinterpret_cast<const DocIndex*>
> +            (svgData + mHeader->mDocIndexOffset);
> +        // Limit the number of documents to avoid overflow
> +        if (uint64_t(mHeader->mDocIndexOffset) + 2 +
> +                uint16_t(mDocIndex->mNumEntries)*sizeof(IndexEntry) <= length) {

Nit, spaces around * operator.

Comment 30

[Jonathan Kew [:jfkthame]]

Comment on attachment 794494 [details] [diff] [review]
Part 8: fix tests

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

Looks fine, though I didn't actually test these - I assume you did! Note that because bug 871961 got partially backed out, we're not currently running the text-svgglyphs directory on TBPL. :(

Comment 31

[Bill Gianopoulos [:WG9s]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> Created attachment 794493 [details] [diff] [review]
> Part 6 v2

This does not compile with GCC 4.5.2 gets this error:

In file included from /home/wag/mozilla/mozilla2/gfx/thebes/gfxFont.cpp:42:0:/home/wag/mozilla/mozilla2/gfx/thebes/gfxSVGGlyphs.h:139:5: error: qualifiers can only be specified for objects and functions

Comment 32

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #31)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> > Created attachment 794493 [details] [diff] [review]
> > Part 6 v2
> 
> This does not compile with GCC 4.5.2 gets this error:
> 
> In file included from
> /home/wag/mozilla/mozilla2/gfx/thebes/gfxFont.cpp:42:0:/home/wag/mozilla/
> mozilla2/gfx/thebes/gfxSVGGlyphs.h:139:5: error: qualifiers can only be
> specified for objects and functions

Oops that should have said GCC 4.5.1.

Comment 33

[Bill Gianopoulos [:WG9s]]

It seems to be this change:

     const struct IndexEntry {
         mozilla::AutoSwap_PRUint16 mStartGlyph;
         mozilla::AutoSwap_PRUint16 mEndGlyph;
         mozilla::AutoSwap_PRUint32 mDocOffset;
         mozilla::AutoSwap_PRUint32 mDocLength;
-    } *mIndex;
+    };

That causes the GCC 4.5.1 issue.

Comment 34

[Jonathan Kew [:jfkthame]]

Yes - we need to remove the 'const' from there.

Roc, could you tweak that in Part 6, please? Sorry I overlooked it in review.

(I'd also be inclined to express it as a typedef, as per comment 17, but I guess that doesn't really matter.)

Comment 35

[Bill Gianopoulos [:WG9s]]

(In reply to Jonathan Kew (:jfkthame) from comment #34)
> Yes - we need to remove the 'const' from there.
> 
> Roc, could you tweak that in Part 6, please? Sorry I overlooked it in review.

No need to apologize.

> 
> (I'd also be inclined to express it as a typedef, as per comment 17, but I
> guess that doesn't really matter.)

THat seems to fix the issue for me, thanks!

Comment 36

[Jonathan Kew [:jfkthame]]

Comment on attachment 794493 [details] [diff] [review]
Part 6 v2

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

Withdrawing r+ on this patch, as I just noticed a discrepancy between the code here and the spec for the document index.

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +119,5 @@
> +        unsigned int length;
> +        const uint8_t *data = (const uint8_t*)hb_blob_get_data(mSVGData, &length);
> +        if (entry->mDocOffset + entry->mDocLength <= length) {
> +            result = new gfxSVGGlyphsDocument(data + entry->mDocOffset,
> +                                              entry->mDocLength);

This is incorrect. The current SVG-in-OT draft says that mDocOffset in the document index is

"Offset from the beginning of the SVG Document Index to an SVG document. Must be non-zero."

so the total offset from the start of the table data should be mHeader->mDocIndexOffset + entry->mDocOffset.

Comment 37

[Jonathan Kew [:jfkthame]]

Comment on attachment 794474 [details] [diff] [review]
Part 5 v2

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

Cancelling r+ here as well, to draw attention to the incorrect document/length check.

::: gfx/ots/src/svg.cc
@@ +75,4 @@
>        NONFATAL_FAILURE("Bad SVG document length");
>      }
>  
> +    if (uint64_t(doc_offset) + doc_length > length) {

This needs to check doc_index_offset + doc_offset + doc_length, because doc_offset is measured from the start of the document index, not the start of the table as a whole.

Also, the spec requires doc_offset to be non-zero, so we should check that here too. (If it -were- zero, there's no way the SVG document it points to could be valid, so it'd get rejected at parse time. But given that the spec explicitly prohibits this, we should just reject the whole table as ill-formed.)

Comment 38

[Jonathan Kew [:jfkthame]]

Comment on attachment 794494 [details] [diff] [review]
Part 8: fix tests

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

And the font files here will also need updating to account for the document-offset bug.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #26)
> Eww... I don't really like the manual conversion loop here; that sort of
> thing should be handled by a library function. But I suppose converting the
> string to 8-bit chars and using strtol() (and then checking that it consumed
> the whole string) would end up being just as verbose.

And we'd have to manually check that the first character is 1-9.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #27)
> How about checking that doc_index_offset (and color_palettes_offset below)
> doesn't exceed length?

Done.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5 v3

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6 v3

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 8 v2

I have run these tests locally.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9 v2

Comment 45

[Jonathan Kew [:jfkthame]]

Comment on attachment 795217 [details] [diff] [review]
Part 5 v3

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

LGTM, modulo the copy/paste oopsie!

::: gfx/ots/src/svg.cc
@@ +41,5 @@
> +  if (!table.ReadU32(&color_palettes_offset)) {
> +    NONFATAL_FAILURE("Couldn't read color palettes offset from SVG table header");
> +  }
> +  if (color_palettes_offset >= length) {
> +    NONFATAL_FAILURE("Invalid doc index offset");

s/doc index/color palettes/

Comment 46

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> Created attachment 795219 [details] [diff] [review]
> Part 8 v2
> 
> I have run these tests locally.

Good :) -- though I wish we had them running on tbpl as well. We need to take another look at bug 871961. You're frequently on Windows, aren't you - any chance you can reproduce the crash we ran into there (bug 875878)?

Comment 47

[Jonathan Kew [:jfkthame]]

Comment on attachment 795220 [details] [diff] [review]
Part 9 v2

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

Hmm, does this test pass for you? I just built this patch set and got a failure on it - the testcase renders a red square. If you confirm it works for you, I'll try to investigate further here (but it won't be till tomorrow).

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes it does.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #47)
> Hmm, does this test pass for you? I just built this patch set and got a
> failure on it - the testcase renders a red square. If you confirm it works
> for you, I'll try to investigate further here (but it won't be till
> tomorrow).

I reapplied to inbound and reran the tests locally and the glyph IDs test still passes for me. So I think I should just check in.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, eager as I am to check in, I suppose I should wait for you to test locally.

Comment 51

[Jonathan Kew [:jfkthame]]

Comment on attachment 795220 [details] [diff] [review]
Part 9 v2

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

Sorry, my mistake - it does indeed work as expected. Let's get this landed!

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9766dc682c06
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cad8a76e971
https://hg.mozilla.org/integration/mozilla-inbound/rev/cff6ff652c4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb8fcd98fc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/862f41ab28b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/641b974b1a8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9932e507b43
https://hg.mozilla.org/integration/mozilla-inbound/rev/10affdb2651c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c616d0390af

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9766dc682c06
https://hg.mozilla.org/mozilla-central/rev/7cad8a76e971
https://hg.mozilla.org/mozilla-central/rev/cff6ff652c4c
https://hg.mozilla.org/mozilla-central/rev/3fb8fcd98fc9
https://hg.mozilla.org/mozilla-central/rev/862f41ab28b2
https://hg.mozilla.org/mozilla-central/rev/641b974b1a8f
https://hg.mozilla.org/mozilla-central/rev/a9932e507b43
https://hg.mozilla.org/mozilla-central/rev/10affdb2651c
https://hg.mozilla.org/mozilla-central/rev/7c616d0390af