Fop
  1. Fop
  2. FOP-1953

[PATCH] Cleanup and tests to TTFFile (subsetting Glyf)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Resolution: Fixed
    • Affects Version/s: 1.0
    • Fix Version/s: None
    • Component/s: fonts
    • Labels:
      None
    • Environment:
      Operating System: Linux
      Platform: PC
    • External issue ID:
      51596

      Description

      This patch cleans up and adds some high level tests for the Glyf subsetting in TTFFile.

      1. glyftable.patch
        26 kB
        Mehdi Houshmand
      2. cleanup.patch
        25 kB
        Mehdi Houshmand

        Activity

        Hide
        Vincent Hennebert added a comment -

        So a test was possible after all, this is great news. I've applied the patch with minor modifications to make the test run consistently on (hopefully) all JVMs. (I replaced two HashMap with TreeMap.)

        Thanks!
        Vincent

        Show
        Vincent Hennebert added a comment - So a test was possible after all, this is great news. I've applied the patch with minor modifications to make the test run consistently on (hopefully) all JVMs. (I replaced two HashMap with TreeMap.) Thanks! Vincent
        Hide
        Simon Pepping added a comment -

        Thanks for your explanations. Note that I will not commit this patch. I will leave that to committers who are more knowledgable about fonts.

        Show
        Simon Pepping added a comment - Thanks for your explanations. Note that I will not commit this patch. I will leave that to committers who are more knowledgable about fonts.
        Hide
        Mehdi Houshmand added a comment -

        Attachment glyftable.patch has been added with description: Glyf Table

        Show
        Mehdi Houshmand added a comment - Attachment glyftable.patch has been added with description: Glyf Table
        Hide
        Mehdi Houshmand added a comment -

        Added javadocs to GlyfFlags and changed the method name to make it less ambiguous.

        Show
        Mehdi Houshmand added a comment - Added javadocs to GlyfFlags and changed the method name to make it less ambiguous.
        Hide
        Mehdi Houshmand added a comment -

        Hi Simon,
        I guess it's best to address each of these, individually:

        >The title suggests that the patch cleans up something; it does not; it
        >implements a redesign.

        Well, I guess so, the reason I call it clean up is because previously there was a lot of duplicated code, magic numbers, poorly named variables... all the big names. I started it with the intention of not changing the working code, but as I got into it, it seemed that there the old code was iterating over the whole of the subset glyph list several times... A job that could have been made simpler through recursion. I didn't redesign it, because if I were to do so, I would have changed FontFileReader by removing the write method, and remapping the glyphs when the glyph data is extracted from "glyf". (see the TODO)

        >Without knowing much detail about Glyf tables, I was a bit surprised about the
        >method names GlyfFlags.offsetToNextComposedGlyf(int flags)...

        Ok, for this you have to read the TTF spec. But, the flags represent the properties and parameters of the composed glyph and, depending on which flags are set, the number of bytes until the next glyph data.. That method returns the offset to the next composed by reading which flags are set, and which aren't. Since none of the glyph parameters are changed, we just want the offset to the next composed glyph.

        > GlyfFlags.moreComposites(int flags)

        This analyses the flags, checks whether the current data is the last glyph in the "glyf" data we are reading. It is simply used to control the do-while loop.

        Hope that helps, the glyf table spec can be found at: http://www.microsoft.com/typography/otspec/glyf.htm

        I think this change more is more consistent with the spec, since composite glyphs can be recursive, it seemed prudent to me to clean-up/redesign the code to match that.

        Show
        Mehdi Houshmand added a comment - Hi Simon, I guess it's best to address each of these, individually: >The title suggests that the patch cleans up something; it does not; it >implements a redesign. Well, I guess so, the reason I call it clean up is because previously there was a lot of duplicated code, magic numbers, poorly named variables... all the big names. I started it with the intention of not changing the working code, but as I got into it, it seemed that there the old code was iterating over the whole of the subset glyph list several times... A job that could have been made simpler through recursion. I didn't redesign it, because if I were to do so, I would have changed FontFileReader by removing the write method, and remapping the glyphs when the glyph data is extracted from "glyf". (see the TODO) >Without knowing much detail about Glyf tables, I was a bit surprised about the >method names GlyfFlags.offsetToNextComposedGlyf(int flags)... Ok, for this you have to read the TTF spec. But, the flags represent the properties and parameters of the composed glyph and, depending on which flags are set, the number of bytes until the next glyph data.. That method returns the offset to the next composed by reading which flags are set, and which aren't. Since none of the glyph parameters are changed, we just want the offset to the next composed glyph. > GlyfFlags.moreComposites(int flags) This analyses the flags, checks whether the current data is the last glyph in the "glyf" data we are reading. It is simply used to control the do-while loop. Hope that helps, the glyf table spec can be found at: http://www.microsoft.com/typography/otspec/glyf.htm I think this change more is more consistent with the spec, since composite glyphs can be recursive, it seemed prudent to me to clean-up/redesign the code to match that.
        Hide
        Simon Pepping added a comment -

        Thanks for your explanation.

        The title suggests that the patch cleans up something; it does not; it implements a redesign.

        Without knowing much detail about Glyf tables, I was a bit surprised about the method names GlyfFlags.offsetToNextComposedGlyf(int flags) and GlyfFlags.moreComposites(int flags). At first sight it suggests that there is some hidden iterator. After some more thought, I assume that flags contains pointers to data in the table, and that these allow one to calculate the extent of this glyf; if that is so, then offsetToNextComposedGlyf may be better called extentOfComposedGlyf.

        Otherwise the design seems fine, but I am not able to judge about the correctness of the code.

        Show
        Simon Pepping added a comment - Thanks for your explanation. The title suggests that the patch cleans up something; it does not; it implements a redesign. Without knowing much detail about Glyf tables, I was a bit surprised about the method names GlyfFlags.offsetToNextComposedGlyf(int flags) and GlyfFlags.moreComposites(int flags). At first sight it suggests that there is some hidden iterator. After some more thought, I assume that flags contains pointers to data in the table, and that these allow one to calculate the extent of this glyf; if that is so, then offsetToNextComposedGlyf may be better called extentOfComposedGlyf. Otherwise the design seems fine, but I am not able to judge about the correctness of the code.
        Hide
        Mehdi Houshmand added a comment -

        Hi Simon,
        Yeah, Glyf refers to the table within a TrueType font, not a glyph. I've discerned the difference between the two by spelling the table-name as it is represented in the font, as "glyf".

        Hope that makes sense

        Mehdi

        Show
        Mehdi Houshmand added a comment - Hi Simon, Yeah, Glyf refers to the table within a TrueType font, not a glyph. I've discerned the difference between the two by spelling the table-name as it is represented in the font, as "glyf". Hope that makes sense Mehdi
        Hide
        Simon Pepping added a comment -

        Thanks for this work. Glyph is written with 'ph', not with 'f'.

        Show
        Simon Pepping added a comment - Thanks for this work. Glyph is written with 'ph', not with 'f'.
        Hide
        Mehdi Houshmand added a comment -

        Attachment cleanup.patch has been added with description: Cleanup and tests

        Show
        Mehdi Houshmand added a comment - Attachment cleanup.patch has been added with description: Cleanup and tests

          People

          • Assignee:
            fop-dev
            Reporter:
            Mehdi Houshmand
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development