Re: Add support for FT_Face cloning
Hello Tayyab, > I have been following for quite some time but David is not > responding. It feels to me like he is not interested in this > proposal. David, while still being the spiritus rector behind the scenes, is not a regular contributor to FreeType any more due to time constraints; if such things happen more gentle 'pings' is the right action. > It’s okay if FreeType team doesn’t consider my contribution to be a > worthy inclusion. But this should have been communicated long > before I started working on it. I have written production level > code with all the formatting according to FreeType’s style. Now > seeing my merge request going into waste is really hurting me. I'm sorry that you feel hurt, but I don't share your point of view. First of all, there were severe reservations *from the very beginning*, and you have been warned. Secondly, you have *not* written production-level code: as mentioned in the MR, you have implemented only a very special case that probably covers your needs, but we have to take care of more than a single font format. In other words, you have submitted proof-of-concept code, which was good stuff for discussion. BTW, that you are following FreeType's formatting style is a matter of course. It's *your* job to make the life of a project's maintainers as easy as possible. The more the code blends into a project, the easier it is to concentrate on the important details. Anyway: As far as I can see, the concerns are too big to integrate your cloning code, sorry. Besides fundamental issues, it seems to me that there isn't much interest from users for this functionality; at least nobody voiced demand for it except you. Since FreeType's development speed is rather slow it shouldn't be too hard for you to maintain your cloning code privately for your project; this means that your time working on the MR was certainly not wasted. If someone is going to work on API details for FreeType 3 (which would be thread-safe, among other things), we now have a greater sensibility to mutability and related issues thanks to the discussion; it was certainly helpful and important to see your code – thanks for that. Werner
Re: Add support for FT_Face cloning
> So basically, we need to organize the fields in such a way that they
> reflect their traits. It can be having a prefix describing the
> trait or organizing like the following:
>
> typedef struct ABC {
> struct {
> /* Mutable Properties. */
> } mutable;
>
> struct {
> /* Lazy Properties. */
> } lazy;
>
> /* Immutable Properties. */
> FT_UInt first;
> FT_UInt second;
> } ABC;
Alas, this is not possible for public structures, which can't be
changed in any way to retain backwards compatibility.
Werner
Re: Add support for FT_Face cloning
Yeah, but I guess there is only one public data structure involved, that is
`FT_Face`. We can write comments for it as its not gonna change but we can
reorganize the internal ones.On Wednesday, February 9, 2022, 10:48:02 PM
GMT+5, Werner LEMBERG wrote:
> So basically, we need to organize the fields in such a way that they
> reflect their traits. It can be having a prefix describing the
> trait or organizing like the following:
>
> typedef struct ABC {
> struct {
> /* Mutable Properties. */
> } mutable;
>
> struct {
> /* Lazy Properties. */
> } lazy;
>
> /* Immutable Properties. */
> FT_UInt first;
> FT_UInt second;
> } ABC;
Alas, this is not possible for public structures, which can't be
changed in any way to retain backwards compatibility.
Werner
Re: Add support for FT_Face cloning
> First, the proposed merge requests adds a ton of conditional code paths that
> are impossible to properly review now or even maintain over time. Since there
> is no proper distinction in the source code between what is supposed to be
> shareable+readonly and exclusive+mutable, There is no way by looking at the
> code to determine the correctness of the change, this will make improving
> library internals even more difficult in the future. It is very likely that
> this feature is gonna break in subtle ways that are very hard to detect
> beforehand. Given the scope of the library's API, I don't think it is
> possible to come up with an extensive regression test that covers every
> possibility. I am convinced this will not be maintainable over time. Sorry.
I get your point and have a few options in mind to overcome this. But it would
require some structural changes. That's why I was deliberately being quiet
about that. So basically, we need to organize the fields in such a way that
they reflect their traits. It can be having a prefix describing the trait or
organizing like the following:
typedef struct ABC { struct { /* Mutable Properties. */ } mutable;
struct { /* Lazy Properties. */ } lazy;
/* Immutable Properties. */ FT_UInt first; FT_UInt second;} ABC;
Let me know if this sounds interesting, then we can further refine it.
> The locking requirements do not only apply to the input stream but to any
> data that is loaded or created on demand. A lot of these have been removed to
> make the library "more thread-safe" (it really isn't). And since the FreeType
> API was designed to use mutable state very intentionally (this saves a lot of
> memory compared to the use of immutable data structures, which was important
> for embedded systems). There is no clear way for a client to determine
> whether a given API will mutate the state, so the only safe option for a
> client is to lock on every API call.
Again, I agree with this. But I have shared the problem statement and how
immutability can overcome that. Definitely the safe option would be to acquire
the lock on every API call but we can document the APIs that would need it. I
was only referring to associate the lock with the input stream, not necessarily
acquiring it in read operation.
> Finally, calling these instances "clones" or "copies" is misleading. A best
> they should be called "dependent faces" or "child faces", but the fact that
> they complicate the lifecycle management of many objects is a problem in
> itself.
We can also consider to call it a "derived" face and name the function
"FT_Derive_Face" instead. We can also keep the list of child faces in the root
face to manage their life cycle. A child face would retain the parent face and
remove itself from the list on deallocation.
> An FT_Clone_Face() API that takes an input FT_Face instance and returns a new
> instance that has its own lifecycle (except that they will be children of the
> same FT_Library, and use the same FT_Memory allocator).
I have shared an alternate above.
> For the input stream, a way to either clone it, or share it safely between
> instances is needed, and should be provided as an argument to the function in
> some way. We could change the stream implementation used internally by the
> library to make this easier, or we could require the client to use
> FT_Open_Face() with a user-provided shareable stream for FT_Clone_Face() to
> work.
It seems a little complicated to me. Besides, some OS don't allow many
instances of the same file.
> The initial implementation would simply re-open the face with the new stream,
> inefficient but completely safe. But this opens the door to identifying
> read-only data in the source face that can be copied directly to the clone,
> saving all the work required to load/validate/compute it.
I have already identified the traits of variables. They are written alongside
each copy operation in clone implementations.
> Note that I wrote "copy" above, because for efficient and safe read-only
> sharing, atomic reference counting is required, which is not part of C99,
> hence not portable. However, it can be introduced as a separate step by
> defining the right abstractions (types and macros to operate on them).
> Essentially, we need the equivalent of std::shared_ptr<>, or the intrusive
> versions of it where the ref-count is at the start of the shared object, but
> in C99 instead. For platforms that do not support threads, just do non-atomic
> refcounts.
I also had this in mind but I was trying my level best to keep things simple
and additive only. I was thinking more of requiring atomic integer
implementation from the client side because it's cheaper than a lock.If we
decide to go this way then this would need to be catered first, I guess.On
Wednesday, February 9, 2022, 05:37:56 AM GMT+5, David Turner
wrote:
I have specific worries about what is being proposed here:
Re: Add support for FT_Face cloning
I have specific worries about what is being proposed here: - First, the proposed merge requests adds a ton of conditional code paths that are impossible to properly review now or even maintain over time. Since there is no proper distinction in the source code between what is supposed to be shareable+readonly and exclusive+mutable, There is no way by looking at the code to determine the *correctness* of the change, this will make improving library internals even more difficult in the future. It is very likely that this feature is gonna break in subtle ways that are very hard to detect beforehand. Given the scope of the library's API, I don't think it is possible to come up with an extensive regression test that covers every possibility. I am convinced this will not be maintainable over time. Sorry. - The locking requirements do not only apply to the input stream but to any data that is loaded or created on demand. A lot of these have been removed to make the library "more thread-safe" (it really isn't). And since the FreeType API was designed to use mutable state very intentionally (this saves a lot of memory compared to the use of immutable data structures, which was important for embedded systems). There is no clear way for a client to determine whether a given API will mutate the state, so the only safe option for a client is to lock on every API call. - Finally, calling these instances "clones" or "copies" is misleading. A best they should be called "dependent faces" or "child faces", but the fact that they complicate the lifecycle management of many objects is a problem in itself. Just like for multi-thread usage, the only correct way to deal with a mutable API is to either use locking of a single instance before doing any mutable operation (be it loading something, or changing some settings), or to create several FT_Face instances instead. This keeps an already complicated API manageable without introducing new failure modes. However, it should be possible to implement a real "cloning" facility that could be used for safe multi-threaded and variable-fonts usage. As long as all read-only sharing is hidden from the client, it can be introduced progressively into the source tree. What I mean more precisely: - An FT_Clone_Face() API that takes an input FT_Face instance and returns a new instance that has its own lifecycle (except that they will be children of the same FT_Library, and use the same FT_Memory allocator). - For the input stream, a way to either clone it, or share it safely between instances is needed, and should be provided as an argument to the function in some way. We could change the stream implementation used internally by the library to make this easier, or we could require the client to use FT_Open_Face() with a user-provided shareable stream for FT_Clone_Face() to work. - The initial implementation would simply re-open the face with the new stream, inefficient but completely safe. But this opens the door to identifying read-only data in the source face that can be copied directly to the clone, saving all the work required to load/validate/compute it. - Note that I wrote "copy" above, because for efficient and safe read-only sharing, atomic reference counting is required, which is not part of C99, hence not portable. However, it can be introduced as a separate step by defining the right abstractions (types and macros to operate on them). Essentially, we need the equivalent of std::shared_ptr<>, or the intrusive versions of it where the ref-count is at the start of the shared object, but in C99 instead. For platforms that do not support threads, just do non-atomic refcounts. - The most important part is being able to progressively increase the efficiency of the cloning process in a way that adds read-only sharing in an explicit way that is easy to control at review time, or when changing the library's internals. Le ven. 4 févr. 2022 à 17:02, Werner LEMBERG a écrit : > > > This proposal aims to take the best of both approaches by > > introducing a new function, `FT_Clone_Face`. [...] > > Excellent summary, much appreciated, thanks! > > What we are mainly interested in is whether other users would going to > use the proposed API as advertised. If yes, please say so. Otherwise > it would be great if you could discuss problematic issues with > Tayyab's approach. > > > Werner > >
Re: Add support for FT_Face cloning
> This proposal aims to take the best of both approaches by > introducing a new function, `FT_Clone_Face`. [...] Excellent summary, much appreciated, thanks! What we are mainly interested in is whether other users would going to use the proposed API as advertised. If yes, please say so. Otherwise it would be great if you could discuss problematic issues with Tayyab's approach. Werner
Re: Add support for FT_Face cloning
Motivation== With the rise of variable fonts, it's possible to derive many variation instances of a typeface with different combination of coordinate values or colors. FreeType has already provided support for these fonts. Currently, two ways come into mind for adding support for these variations from a client's perspective. * Have a single `FT_Face` object per font and setup specified variation coordinates on it whenever there's a request to perform a variation specific operation.* Open a new `FT_Face` object per variation and reuse it when needed. The first approach has following cons.1. There's a potential cost of context switching with each variation-specific operation.2. It's difficult to cache the face with variation-specific settings. The second approach solves above drawbacks but it has following cons.1. Additional cost of face initialization and setup.2. Increased memory usage. Proposal This proposal aims to take the best of both approaches by introducing a new function, `FT_Clone_Face`. This function would make a copy of the specified face by ensuring that the copied face shares as much memory as possible with the parent face. The output will be an exact copy of the source face leaving only glyph slot and size objects which will always be new. The client would be free to treat the cloned instance as any new face. This would help in solving the drawbacks of 2nd approach effectively. As an addition, this would also help a client achieve immutability based on any combination of settings at `FT_Face` level. Some Concepts= Before jumping into implementation details, there's a need to revise some concepts that would help understand the proposed solution. A variable in a data structure can have the following traits. Memory--* Static: The memory of this variable is statically allocated along with the parent struct and it has no life cycle of its own.* Dynamic: The memory of this variable is dynamically allocated / freed at runtime and it has a proper life cycle. State-* Immutable: This variable belongs to a data structure whose all members are set during initialization and not changed afterwards.* Mutable: This variable belongs to a data structure whose members are expected to be changed after initialization. Assignment--* ReadWrite: The value of this variable can be set / changed anytime.* Readonly: The value of this variable is set during initialaztion and not changed afterwards.* Lazy: This variable is only initialized when accessing it for the first time. Implementation Guidelines= The cloning can be done with the following set of rules. Static & (Immutable | Mutable) & (ReadWrite | Readonly | Lazy)--Such variable can be copied in cloned instance with just simple assignment since it does not involve any kind of memory management. Dynamic & Immutable & Readonly--Such variable can be shared in cloned instance provided that its lifecycle is only managed by the parent. Dynamic & Immutable & Lazy--There are two ways to resolve such variable:* Reset it along with related variables to initial values so that they can be initialized again on demand. In this case the lifecycle should be managed by the cloned instance.* Copy its value with simple assignment in cloned instance. If it wasn't initialized during cloning, then on demand initializer function would be invoked when accessed for the first time. In this case, the cloned instance should check for the variable's existence in parent first. If it's already available in the parent, the values should be copied from there. If it's not available in the parent, then on demand initializer should be invoked on parent instance. After that, the values should be copied from the parent. Since the variable is only initialized in parent, its lifecycle should be managed by parent only. Any Other Case--A new variable of same type must be created in cloned instance and the memory should be copied into it from the source variable. Needless to say, the cloned instance should also manage the lifecycle of new variable. Implementation Details== As of now, the implementation of `FT_Clone_Face` is complete but currently it only covers the `truetype` format. It is available as part of merge request !134 (https://gitlab.freedesktop.org/freetype/freetype/-/merge_requests/134/diffs). The commit history will be revised where the first-ever commit will declare a new function, `FT_Face_CopyFunc` in `ftdrv.h`. It would take two parameters, a source face and a target face. The implementation would be responsible for copying format specific face properties into the target face. A new field `copy_face` would be declared in `FT_Driver_ClassRec`. Its data type would be `FT_Face_CopyFunc` and each format would set this to `NULL`
