Simplify file handling in ft2font. #15104
Conversation
| open_args->flags = FT_OPEN_STREAM; | ||
| open_args->stream = &self->stream; | ||
| } else { | ||
| if (PyObject_HasAttrString(py_file_arg, "read") && |
There was a problem hiding this comment.
... or alternatively we can just read the whole file in memory, as previously the case in this branch...
Does it have to be? Otherwise it might be reasonable to deprecate its use and clean up later. |
|
well, that can happen later :) |
| } | ||
|
|
||
| return 0; | ||
| memcpy(buffer, tmpbuf, n_read); |
There was a problem hiding this comment.
Do you have any insights how often libpng calls this callback?
There was a problem hiding this comment.
My guess is just once to load the file, but no guarantees here.
There was a problem hiding this comment.
This is FreeType, not libpng. It's called pretty repeatedly, maybe for every glyph? For example, in examples/misc/ftface_props.py, it's called 68 times, while in examples/showcase/anatomy.py, it's called 8064 times. There are not 8000 glyphs in that figure, so I guess it's even more than once per glyph. Maybe it's improved in newer versions, but over half (4574) of those are called with 0 count.
There was a problem hiding this comment.
Looks like calls with count=0 are simply used, well, to seek() the file, but given that FT only interacts with the file via this callback, and that read offsets are always given from the file start, we could even just skip pure seek()s (i.e. immediately return if count=0) as later read()s will anyways pass an absolute offset. Adding the early return doesn't seem to break anything, do you want that?
| } | ||
|
|
||
| return 0; | ||
| memcpy(buffer, tmpbuf, n_read); |
There was a problem hiding this comment.
This is FreeType, not libpng. It's called pretty repeatedly, maybe for every glyph? For example, in examples/misc/ftface_props.py, it's called 68 times, while in examples/showcase/anatomy.py, it's called 8064 times. There are not 8000 glyphs in that figure, so I guess it's even more than once per glyph. Maybe it's improved in newer versions, but over half (4574) of those are called with 0 count.
|
Even with the latest FT there's a gazillion reads. |
|
It's not clear to me that this PR is solving a problem or improving performance. Is there an expected benefit other than reduced lines of code? |
|
Closes #15410, based on a test supplied there. |
|
FWIW, running But note these are just simple plots and don't heavily tax the text rendering routines. |
|
By executive decision, @QuLogic can merge this when happy. |
Just call the Python-level seek(), read() and close() instead of trying to play with C-level FILE*. Note that unlike the png case, we can't just pass restrict ourselves to passing in file-like objects because FT2Font is public API.
|
I handled the non-performance related comments in a separate commit (please squash when merging).
|
|
... and inlined convert_open_args as it's now short enough that I think inlining it is actually clearer. |
|
It's not a bug fix, so no, that's unlikely. And 3.3 is soon, you can already install the release candidate. |
Just call the Python-level seek(), read() and close() instead of trying
to play with C-level FILE*.
Note that unlike the png case (#15095), we can't just pass restrict ourselves to
passing in file-like objects because FT2Font is public API.
PR Summary
PR Checklist