-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a document explaining how the glyph keyed segment generation process works. #97
base: main
Are you sure you want to change the base?
Conversation
In my opinion it's much too early to merge this. |
This is not intended to be a final take on the subject, but rather just capture how the current prototype code works so I can share and gather feedback. With that in mind I added a status section that explains this is all still in early stages of development. |
cea8821
to
adcb236
Compare
It's premature to be gathering input on this outside of the working group, and the working group can access this document in other ways. This draft goes into great detail about one particular approach to forming glyph-keyed patches while entirely omitting other relevant subjects. It's far too early to merge this. |
adcb236
to
c672b5c
Compare
This proposal is not related to the specification. It's describing one possible implementation of part of spec compliant encoder. The encoder implementation is not part of the working groups deliverables, so I don't see any reason to keep it within the confines of working group discussions. In this case I'm specifically looking to gather feedback from outside the working group.
That's right this document is meant to describe how the particular approach I'm currently investigating in glyph_segmentation.cc functions. It's not the intention to present this as the only possible or a complete solution. The status section I added describes this, mentions some of the missing pieces, and also talks about possible other approaches. I added some additional tweaks to the wording to make this more clear. |
I don't think something like this is necessary for the upcoming review and in its present form I worry it could raise more questions than it answers.
My understanding of the spec development process is that we share and gather feedback with the working group first, and then when that process has been worked through we share with the wider W3C audience. @vlevantovsky is that your understanding as well? |
Just to reiterate, this document is not part of the specification and is not proposing any changes to the specification. This repo and the proposal here is for an IFT encoder implementation. An encoder implementation is not a deliverable of the web fonts working group (see section 3 in the charter here: https://www.w3.org/2023/02/webfonts-2023.html), so I don't believe any of this is in the scope of the working group. |
Oh, sorry, I thought this was somehow related to the upcoming release of the specification. In that case, can you give an idea of who the intended audience is? |
(This is still a W3C repository closely related to the spec, and I don't want there to be confusion on the part of spec reviewers about this approach.) |
Two main groups:
Yeah, agreed. The current setup somewhat just an artifact of the history of this repo. It was originally closely tied to the analysis suite which was very much relevant to the working group, but its since been renamed a few times now and the scope has been significantly narrowed to be just an encoder implementation. It's probably worth a discussion if it makes sense to move this repo outside of the w3c org, for example it might make sense to move it under the fontations group of projects since the client already lives over there. |
It sounds like Google considers this code to be their own project now. That hasn't been the basis on which Adobe was participating, but I suppose that was my confusion about its status given where the repository is stored. So, yes, if this is a Google project I think it would be best to move it outside of the W3C umbrella and then Google can add whatever documents to it that they would like. |
I wasn't implying this is a google owned project. I've also been operating under the assumption that it is a collaborative effort. All I was implying above is that we may wan't to consider discussing if this is still the best place for the repo to live. If a decision is made to move it that should be made as a group. Likewise if we all agree this is still the best place for it, then I don't see any issues leaving it here either. Back to the original comment, I'm not worried about spec reviewers seeing this and getting confused. This repo and document aren't linked anywhere from the specification. Additionally, this repo and this document are both clearly labelled as being an implementation of the specification. |
As what group? You said yourself that this project isn't a deliverable of the working group. And all of this is clearly in the context of whether my views about this document are relevant to merging it. |
At present that's pretty much just me and you who are actively involved in the encoder implementation so that's likely a conversation between us. If you're happy with the repo living here, then that works for me too. With the clarification that this document isn't tied to the specification, do you still have objections to merging this? |
Yes, because the repo being under w3c creates ambiguity about the status of such documents. |
I don't understand. It is documentation of what this particular implementation does, not a constraint on what all implementations must do (which would belong in the specification). |
@svgeesus If this repo contains a Google project, then I don't disagree, but I think it would be better to move it out from under the w3c hierarchy. If this is a project related to the working group, or a subset of the working group, then Garret and I have already discussed various issues with this approach and at the time he agreed what we actually do wouldn't take this form, because of those issues. Now he wants "feedback" on this approach, with some vague language about it not being "final", that doesn't take our concerns into account. This is part of a trend of this project moving in the direction of "this is a shared project when that works for Google and a Google project when it doesn't". Until there is more clarity about that I would prefer this document isn't merged under the w3c heading. |
I have trouble reconciling the algorithm described in the document with the explanation of its role. The introduction says:
This seems like an exaggeration at best relative to the first goal:
Surely almost all of the work concerning "how to split glyphs across a set of patches" is in the "desired segmentation", given how little the algorithm appears to change that grouping in light of what it finds. From the intro again:
This makes it sound as if this algorithm could eventually be extended to "produce" performant segmentations, even though it seems to have little to do with that more general problem. If the idea is that we want to gather suggestions on how to make this algorithm produce performance segmentations, I would think it should contain some guidance about that, given that how it operates currently is so independent of that. If, on the other hand, the goal of this algorithm is transform a "rough draft" of a segmentation (whether performant or not) into a "final draft" that meets the closure requirement, the document should characterize it that way. (There are of course many possible intermediate points between an algorithm that solves the whole problem and an algorithm that does a "tune up" on the output of some earlier algorithm. The IFTB prototype encoder took the equivalent of unicode "streams" as input -- groups of codepoints, some ordered and some not, that the encoder tried to keep more or less adjacent but would move around as needed to better optimize for dependencies. Maybe something like this algorithm could be extended that way but it's far from obvious (to me) how it would be extended that way given the inputs it takes and what it does.) |
I'm not sure how this case is addressed:
From what I can tell:
If this is indeed a gap then the algorithm doesn't meet the stated goal of ensuring the segmentation meets the closure requirement. |
docs/glyph_segmentation.md
Outdated
Given: | ||
|
||
* An input font, $F$. | ||
* A list of segments $s_0, …, s_n$. Where: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In "Goals" above it says the inputs are desired segmentations of Unicode codepoints. I don't see why a desired segmentation would put the same codepoint in two segments, so I don't see the purpose of
Or, if the idea is that the previous stage has done some optimization work, such that it is addressing certain desired duplication cases, why are the input segments in terms of codepoints rather than glyphs (or both)? What if the the earlier stage already has a good idea of where to put unmapped glyphs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s_0 is a special segment. It's the set of codepoints and associated glyphs placed the initial font. The s_i != s_0 is just saying this procedure doesn't apply to the init segment (s_0) since it's considered to be already available and doesn't need patches or conditions generated for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I understand that but it raises additional questions:
Is s_0 a set of codepoints (as the text seems to indicate) or a set of codepoints and associated glyphs? If the latter is it a requirement that the earlier stage determine the dependencies for s_0 and include them? It doesn't seem that there is any s_0-specific analysis in the algorithm and therefore it's unclear how it's ensured that s_0 contains its dependencies.
Or is the idea to treat any such dependencies as exceptions in the algorithm?
@skef thanks for refining your position from "do not merge" to specific, actionable questions and criticisms. That is helpful. @garretrieger could you address these points before merging? Either by correcting the algorithm or by adding clarifying prose about the capabilities and the specific problem being solved? I also suggest making it clearer where this sample code represents one implementation approach, not the only one. |
This document and the accompanying code are purely exploratory. I'm just looking to try out this approach and see if it's workable in practice, and at the same time gather some feedback from experts in the font space on it. The document doesn't do a good enough job communicating that, I'll fix that. It's not in anyway meant to be a complete, final, or only solution. |
Yeah, this is what the intent of this specific algorithm is. The idea is that something before it produces a high quality unicode based segmentation (ideally taking into account interactions between codepoints) and this procedure turns that into the actual list of patches and activation conditions for those patches such that the closure requirement is satisfied. I've rewritten parts of the introduction to make it more clear what the scope of this procedure is. |
This case is handled by the "additional conditions" check. Basically once we construct a disjunctive group, it's possible that there may be remaining conditions which are not yet discovered involving two or more segments simultaneously. Here's the relevant text: "Otherwise if the set is disjunctive, then the glyph is needed when at least one segment in the set is present or possibly due to some additional undiscovered conditions. These potential undiscovered conditions are important, and later on the process will attempt to rule them out." "Compute the Segment Closure Analysis for a new composite segment In this particular case, {x} will be found in I - D after testing s_4 U s_6 according to that procedure. If multi segment analysis is not performed then glyph x will be placed in the fallback patch whose load is triggered on the presence of any input segment (so s_1 OR .. OR s_n in this case). If multi segment analysis is performed then the correct condition will be discovered once the combination (s_4 U s_6) is tested. This is the sort of case where a good quality input unicode segmentation can really help out the creation of the glyph segments by placing x, y, z in a single segment together. |
I see better how that case is handled now. |
I'm stopping my review of this tonight before the merging section. My general impressions of the segmentation analysis:
|
are commonly used together. This document and the implementation make no attempt to solve this | ||
problem yet. | ||
|
||
* Patch merging: a very basic patch merging process has been included in the implementation but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bullet sort of comes out of the blue and needs more explanation. What is patch merging? Why might it be useful? You get into more detail below but at this point it's not clear what the topic is.
docs/glyph_segmentation.md
Outdated
Given: | ||
|
||
* An input font, $F$. | ||
* A list of segments $s_0, …, s_n$. Where: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I understand that but it raises additional questions:
Is s_0 a set of codepoints (as the text seems to indicate) or a set of codepoints and associated glyphs? If the latter is it a requirement that the earlier stage determine the dependencies for s_0 and include them? It doesn't seem that there is any s_0-specific analysis in the algorithm and therefore it's unclear how it's ensured that s_0 contains its dependencies.
Or is the idea to treat any such dependencies as exceptions in the algorithm?
group that are found in $I - D$. These glyphs may appear as a result of additional conditions and so | ||
need to be considered unmapped. If the group has no remaining glyphs don’t make a patch for it. | ||
|
||
4. Lastly, collect the set of glyphs that are not part of any patch formed in steps 1 through 3. These form a fallback patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
This is one of the things that doesn't seem to follow if there is no analysis for
$s_0$ . Couldn't there also be a dependency from a codepoint in$s_0$ ? -
What is the advantage of having a patch that is loaded if any other patch is loaded versus just putting these glyphs in
$s_0$ ? Seems like a false economy at best.
|
||
1. If the associated exclusive patch does not exist or it is larger than the minimum size skip this segment. | ||
|
||
2. Locate one or more segments to merge: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title of this section is "Merging Exclusive Patches" but the language in b seems to indicate that it's more like "Merging Into Exclusive Patches*. Otherwise it's confusing why we're looking at patches that mention multiple segments at all.
If we are looking at those, what is the "segment frequency" of a conditional patch? Is it maybe equivalent to the lowest frequency individual segment for conjunctive patches but the highest frequency individual segment for disjunctive patches? Is this outlined anywhere?
In any case I'm confused by these heuristics. Why sort first on the number of segments? Why are we apparently disregarding whether a patch is conjunctive or disjunctive, isn't that quite relevant to merging?
No description provided.