Issue 566080043: Clean up and fix glyph contour generation
|
Created:
4 years, 10 months ago by hanwenn Modified:
3 years, 7 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
Description Clean up and fix glyph contour generation
Use FT_Outline_Decompose to do the real work.
This fixes at least two bugs in the existing code:
* Glyphs with multiple contours (e.g. "O" or "i") should walk the
contours separately
* Contours can wrap around to the start. This leads to OOB reads
and/or incorrect outlines in some circumstances.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use FT_Outline_Decompose #
Total comments: 8
Patch Set 3 : jonas #Patch Set 4 : const& #MessagesTotal messages: 30
Werner, can you confirm that this looks correct? I'm especially not certain about the wrapping.
Sign in to reply to this message.
I don't fully understand this code. Please add more comments. A quite well documented routine to walk over a contour is function `FT_Outline_Decompose` in file `src/base/ftoutln.c` of the FreeType source code. https://codereview.appspot.com/566080043/diff/560030043/lily/freetype.cc File lily/freetype.cc (right): https://codereview.appspot.com/566080043/diff/560030043/lily/freetype.cc#newc... lily/freetype.cc:124: else if (ctags[j] & 1) s/1/FT_CURVE_TAG_ON/ https://codereview.appspot.com/566080043/diff/560030043/lily/freetype.cc#newc... lily/freetype.cc:132: else if (ctags[j] & 2) s/2/FT_CURVE_TAG_CUBIC/
Sign in to reply to this message.
Why is this separate from https://codereview.appspot.com/569700043 ?
Sign in to reply to this message.
it's separate because it causes formatting changes. I like to keep the bugfixes and performance fixes separate. (Performance fixes should not cause regtest differences)
Sign in to reply to this message.
On 2020/05/09 14:23:04, lemzwerg wrote: > I don't fully understand this code. that is an ominous comment to come from the Freetype author. I had assumed you reviewed the existing code as well. > Please add more comments. > > A quite well documented routine to walk over a contour is function > `FT_Outline_Decompose` in file `src/base/ftoutln.c` of the FreeType source code. > I'll have a look.
Sign in to reply to this message.
On 2020/05/09 20:38:12, hanwenn wrote: > it's separate because it causes formatting changes. I like to keep the bugfixes > and performance fixes separate. (Performance fixes should not cause regtest > differences) 👍
Sign in to reply to this message.
> > I don't fully understand this code. > > that is an ominous comment to come from the Freetype > author. I had assumed you reviewed the existing code > as well. I have two difficulties: I'm not good at C++ and Guile stuff, and it's not completely clear to me what the code wants to achieve. It's not your recent changes that I'm struggling with but the overall design. What's definitely missing is handling the case where the first point of an outline is a (second-order) off-curve point, making it necessary to construct an on-curve point if the point before (i.e., the last point of the current contour) is a (second order) off-curve point, too. On the other hand, we talked about approximating the outlines by using the hull polygon that is spanned by the control points: If you do this the construction of the on-curve point wouldn't be necessary, right? Does your code change in commit 79ec62e6 do that already? The comment only says 'Substitute a line segment instead' but doesn't explain whether this is a legitimate approximation. > > A quite well documented routine to walk over a contour is function > > `FT_Outline_Decompose` in file `src/base/ftoutln.c` of the FreeType source > code. > > I'll have a look. Thanks. Note that the first point of a contour can never be a third-order off-curve point, which means that you don't need to handle wrapping in this case.
Sign in to reply to this message.
Use FT_Outline_Decompose
Sign in to reply to this message.
On 2020/05/09 20:38:12, hanwenn wrote: > it's separate because it causes formatting changes. I like to keep the bugfixes > and performance fixes separate. (Performance fixes should not cause regtest > differences) Sounds fair, but I don't understand the relation of the two changes: - Both touch the very same function, and with Patch Set 1 both shared half of the patch. - With the recent Patch Set, this looks more advanced than https://codereview.appspot.com/569700043. So does it replace the former? - If not, how are the two patches supposed to be ordered? This one first, or the other, or does one contain the changes of the other? Until this is clear, there is no point in reviewing either IMHO.
Sign in to reply to this message.
On Sun, May 10, 2020 at 10:38 AM <jonas.hahnfeld@gmail.com> wrote: > > On 2020/05/09 20:38:12, hanwenn wrote: > > it's separate because it causes formatting changes. I like to keep the > bugfixes > > and performance fixes separate. (Performance fixes should not cause > regtest > > differences) > > Sounds fair, but I don't understand the relation of the two changes: > - Both touch the very same function, and with Patch Set 1 both shared > half of the patch. > - With the recent Patch Set, this looks more advanced than > https://codereview.appspot.com/569700043. So does it replace the former? > - If not, how are the two patches supposed to be ordered? This one > first, or the other, or does one contain the changes of the other? > > Until this is clear, there is no point in reviewing either IMHO. The bugfix comes first. -- Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc File lily/freetype.cc (right): https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... lily/freetype.cc:143: }; Not sure if FT developers plan to change this interface at some point. In other projects, I have seen something akin to FT_Outline_Funcs funcs; memset(&funcs, 0, sizeof(funcs)); for external structs from dependencies to make sure no field goes uninitialized. https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... lily/freetype.cc:149: return ((Path_interpreter *) user)->moveto (*to); Do you want to create copies of the arguments? (here and for the other functions) https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... lily/freetype.cc:172: printf (" glyph %ld ", signed_idx); debug? https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... lily/freetype.cc:188: interpreter.transform_ = transform; create constructor for Path_interpreter?
Sign in to reply to this message.
jonas
Sign in to reply to this message.
https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc File lily/freetype.cc (right): https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... lily/freetype.cc:143: }; On 2020/05/10 09:16:58, hahnjo wrote: > Not sure if FT developers plan to change this interface at some point. In other > projects, I have seen something akin to > FT_Outline_Funcs funcs; > memset(&funcs, 0, sizeof(funcs)); > for external structs from dependencies to make sure no field goes uninitialized. the C99 standard actually says that omitted fields are zero-initialized https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html I added the field because GCC warns about it (I don't understand why) https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... lily/freetype.cc:149: return ((Path_interpreter *) user)->moveto (*to); On 2020/05/10 09:16:59, hahnjo wrote: > Do you want to create copies of the arguments? (here and for the other > functions) yes. I find it easier to reason about and read. It's all inlined anyway. https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... lily/freetype.cc:172: printf (" glyph %ld ", signed_idx); On 2020/05/10 09:16:58, hahnjo wrote: > debug? Done. https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... lily/freetype.cc:188: interpreter.transform_ = transform; On 2020/05/10 09:16:58, hahnjo wrote: > create constructor for Path_interpreter? Done.
Sign in to reply to this message.
On 2020/05/10 09:29:48, hanwenn wrote: > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc > File lily/freetype.cc (right): > > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... > lily/freetype.cc:143: }; > On 2020/05/10 09:16:58, hahnjo wrote: > > Not sure if FT developers plan to change this interface at some point. In > other > > projects, I have seen something akin to > > FT_Outline_Funcs funcs; > > memset(&funcs, 0, sizeof(funcs)); > > for external structs from dependencies to make sure no field goes > uninitialized. > > the C99 standard actually says that omitted fields are zero-initialized > > https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html > > I added the field because GCC warns about it (I don't understand why) It could be that this is because of C++ officially inheriting from C89 IIRC? https://en.cppreference.com/w/cpp/language/aggregate_initialization says: "If the number of initializer clauses is less than the number of members [...] the remaining members [...] are [...] copy-initialized from empty lists, in accordance with the usual list-initialization rules (which performs value-initialization for non-class types [...])" which I would read as "uninitialized". > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... > lily/freetype.cc:149: return ((Path_interpreter *) user)->moveto (*to); > On 2020/05/10 09:16:59, hahnjo wrote: > > Do you want to create copies of the arguments? (here and for the other > > functions) > > yes. I find it easier to reason about and read. It's all inlined anyway. const refs should work the same AFAICS
Sign in to reply to this message.
On 2020/05/10 10:00:54, hahnjo wrote: > On 2020/05/10 09:29:48, hanwenn wrote: > > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc > > File lily/freetype.cc (right): > > > > > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... > > lily/freetype.cc:143: }; > > On 2020/05/10 09:16:58, hahnjo wrote: > > > Not sure if FT developers plan to change this interface at some point. In > > other > > > projects, I have seen something akin to > > > FT_Outline_Funcs funcs; > > > memset(&funcs, 0, sizeof(funcs)); > > > for external structs from dependencies to make sure no field goes > > uninitialized. > > > > the C99 standard actually says that omitted fields are zero-initialized > > > > https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html > > > > I added the field because GCC warns about it (I don't understand why) > > It could be that this is because of C++ officially inheriting from C89 IIRC? > https://en.cppreference.com/w/cpp/language/aggregate_initialization says: > "If the number of initializer clauses is less than the number of members [...] > the remaining members [...] are [...] copy-initialized from empty lists, in > accordance with the usual list-initialization rules (which performs > value-initialization for non-class types [...])" > which I would read as "uninitialized". > > > > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... > > lily/freetype.cc:149: return ((Path_interpreter *) user)->moveto (*to); > > On 2020/05/10 09:16:59, hahnjo wrote: > > > Do you want to create copies of the arguments? (here and for the other > > > functions) > > > > yes. I find it easier to reason about and read. It's all inlined anyway. > > const refs should work the same AFAICS Yes, I know. Do you want me to change this?
Sign in to reply to this message.
On 2020/05/10 10:02:27, hanwenn wrote: > On 2020/05/10 10:00:54, hahnjo wrote: > > On 2020/05/10 09:29:48, hanwenn wrote: > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... > > > lily/freetype.cc:149: return ((Path_interpreter *) user)->moveto (*to); > > > On 2020/05/10 09:16:59, hahnjo wrote: > > > > Do you want to create copies of the arguments? (here and for the other > > > > functions) > > > > > > yes. I find it easier to reason about and read. It's all inlined anyway. > > > > const refs should work the same AFAICS > > Yes, I know. Do you want me to change this? Yes, const refs avoid relying on compiler optimizations to remove the copies.
Sign in to reply to this message.
On 2020/05/10 10:00:54, hahnjo wrote: > On 2020/05/10 09:29:48, hanwenn wrote: > > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc > > File lily/freetype.cc (right): > > > > > https://codereview.appspot.com/566080043/diff/560020046/lily/freetype.cc#newc... > > lily/freetype.cc:143: }; > > On 2020/05/10 09:16:58, hahnjo wrote: > > > Not sure if FT developers plan to change this interface at some point. In > > other > > > projects, I have seen something akin to > > > FT_Outline_Funcs funcs; > > > memset(&funcs, 0, sizeof(funcs)); > > > for external structs from dependencies to make sure no field goes > > uninitialized. > > > > the C99 standard actually says that omitted fields are zero-initialized > > > > https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html > > > > I added the field because GCC warns about it (I don't understand why) > > It could be that this is because of C++ officially inheriting from C89 IIRC? > https://en.cppreference.com/w/cpp/language/aggregate_initialization says: > "If the number of initializer clauses is less than the number of members [...] > the remaining members [...] are [...] copy-initialized from empty lists, in > accordance with the usual list-initialization rules (which performs > value-initialization for non-class types [...])" > which I would read as "uninitialized". strange, b/c I think list-initialization is { 1, 2 }, and doing struct x = { 0 } also initializes the rest to 0, even in C89, IIRC
Sign in to reply to this message.
const&
Sign in to reply to this message.
On 2020/05/10 10:03:57, hahnjo wrote: > > > > yes. I find it easier to reason about and read. It's all inlined anyway. > > > > > > const refs should work the same AFAICS > > > > Yes, I know. Do you want me to change this? > > Yes, const refs avoid relying on compiler optimizations to remove the copies. Done.
Sign in to reply to this message.
*Much* better to read and understand, thanks! LGTM now.
Sign in to reply to this message.
On 2020/05/10 11:12:37, lemzwerg wrote: > *Much* better to read and understand, thanks! LGTM now.
Sign in to reply to this message.
On 2020/05/10 11:12:37, lemzwerg wrote: > *Much* better to read and understand, thanks! LGTM now.
Sign in to reply to this message.
Sign in to reply to this message.
|
