Closed Bug 906521 Opened 6 years ago Closed 6 years ago

Update SVG-in-OpenType implementation to new spec draft

Categories

(Core :: Graphics: Text, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: roc, Assigned: roc)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(9 files, 7 obsolete files)

10.69 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
700 bytes, patch
jfkthame
: review+
Details | Diff | Splinter Review
937 bytes, patch
jfkthame
: review+
Details | Diff | Splinter Review
2.54 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
1.80 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
3.94 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
4.69 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
124.96 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
113.72 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
No description provided.
These patches can't land until someone updates the tests to the new format, and I haven't done that yet.
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 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.
Attachment #792153 - Flags: review?(jfkthame) → review-
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.
Attachment #792152 - Flags: review?(jfkthame) → review+
Attachment #792155 - Flags: review?(jfkthame) → review+
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.
Attachment #792156 - Flags: review?(jfkthame) → review+
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.
Attachment #792157 - Flags: review?(jfkthame) → review-
(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.
(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.
(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 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 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?
Attachment #792159 - Flags: review?(jfkthame) → review+
(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.
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)
Attached patch Part 5 v2 (obsolete) — Splinter Review
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.
Attachment #792153 - Attachment is obsolete: true
Attachment #792157 - Attachment is obsolete: true
Attachment #794474 - Flags: review?(jfkthame)
Attached patch Part 6 v2 (obsolete) — Splinter Review
Attachment #794493 - Flags: review?(jfkthame)
Attached patch Part 8: fix tests (obsolete) — Splinter Review
I deleted some tests that are only relevant to "glyphchar".
Attachment #792158 - Attachment is obsolete: true
Attachment #792158 - Flags: review?(jfkthame)
Attachment #794494 - Flags: review?(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.
Attachment #794455 - Flags: review?(jfkthame) → review+
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.
Attachment #794474 - Flags: review?(jfkthame) → review+
(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 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.
Attachment #794493 - Flags: review?(jfkthame) → review+
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. :(
Attachment #794494 - Flags: review?(jfkthame) → review+
Attachment #794495 - Flags: review?(jfkthame) → review+
(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
(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.
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.
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.)
(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 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.
Attachment #794493 - Flags: review+ → review-
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.)
Attachment #794474 - Flags: review+ → review-
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.
Attachment #794494 - Flags: review+ → review-
Attachment #794495 - Flags: review+ → review-
(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.
(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.
Attached patch Part 5 v3Splinter Review
Attachment #794474 - Attachment is obsolete: true
Attachment #795217 - Flags: review?(jfkthame)
Attached patch Part 6 v3Splinter Review
Attachment #794493 - Attachment is obsolete: true
Attachment #795218 - Flags: review?(jfkthame)
Attachment #795218 - Attachment description: Part 6 v2 → Part 6 v3
Attached patch Part 8 v2Splinter Review
I have run these tests locally.
Attachment #794494 - Attachment is obsolete: true
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/
Attachment #795217 - Flags: review?(jfkthame) → review+
Attachment #795218 - Flags: review?(jfkthame) → review+
Attachment #795219 - Flags: review?(jfkthame) → review+
Attachment #795220 - Flags: review?(jfkthame) → review+
(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 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).
Attachment #795220 - Flags: review+ → review?(jfkthame)
(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.
Well, eager as I am to check in, I suppose I should wait for you to test locally.
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!
Attachment #795220 - Flags: review?(jfkthame) → review+
Depends on: 916048
You need to log in before you can comment on or make changes to this bug.