-
Notifications
You must be signed in to change notification settings - Fork 103
Implement lazy font loading, enable embedded font extraction #146
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
Conversation
How the output directory looks like now:
|
74469ce
to
60f4fd3
Compare
Removal of --no-heap-copy is a rebase conflict, will force-push EDIT: yep, conflict was from rebasing this onto #145, now fixed |
60f4fd3
to
e1c85bb
Compare
e1c85bb
to
31f5e58
Compare
Additional comment for |
0c940b7
to
cc230cb
Compare
Commenting only on the embedded fonts part: The js side makes a copy of the three handles at initialisation, see here: JavascriptSubtitlesOctopus/src/pre-worker.js Lines 113 to 115 in 128be61
I would guess those need to be updated when reiniting the handles, but it isn't immediately obvious to me where this happens. Is the library handle, JSO is using internally, exposed to users somehow? There’s |
As far as I know it's only exposed in the worker, probably for debugging reasons. There are various spots where Most of the oct_add_font or other stuff, is only available to direct users of the worker.js (not jso module interface). I can update the code so these values get updated properly, or rid of them elsewhere? Even if desired, you can always inspect/grab these handles via |
My line of thought was that if JSO users can get the internal library the change to also clear memory fonts (and reinit all handles) needs to be documented. If no one has access to it, its an internal change and the existing high level description of "allows to change the track" or similar is sufficient for the affected API functions.
I might have missed something, but grepping through the code it seems like those values are actually only getting assigned to and never used? (If that’s the case: why do they even exist?)
Sorry, can you explain howsuch a direct use for a JSO user might look like, so that Both |
You haven't missed something :) The only way I see to use these APIs are by code ran from the worker side specific, and only relevant to forks, which could already add such API. To be able to use that you need to instantiate the There is a kind of direct way to execute code, and that is via Don't know what to tell you about the intent of those methods/functions/variables being exposed, but I don't see any way to use them at the moment without modifications. I believe this topic should probably be a discussion elsewhere, for now I'll update the code to refresh these variables. |
cc230cb
to
bc4136b
Compare
Thanks, if there's no way an user can modify the library and renderer states or add memory fonts, then there’s nothing more to worry about regarding embedded fonts. Eventhough, the actually diff ended up quite concise, the interaction with the rest of JSO and the effects aren't, so how about a more verbose commit message similar to this? (also avoiding overly long lines):
If you can update the commit message and move the embedded font commit before the lazy loading one, I'm going to merge the embedded font commit in ~24h if rcombs or TFSThiagoBR98 don't object. The question of how to use the interface and dead code warrants a dedicated thread indeed. Only if there would have been some way to add memory fonts there had been direct relevance to the change at hand. (I just noticed |
bc4136b
to
c9c8a01
Compare
I have reworded the embedded fonts commit, and moved it before lazy font loading. On the topic of dead code, I have cleaned up a few of those and refactored a few methods to de-duplicate some code. |
Merged embedded font commit. |
9d797b0
to
c9c8a01
Compare
35a9216
to
382baa0
Compare
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.
Just one header change; otherwise LGTM.
Extracts default.woff2 from binary, embeds fonts.conf, removes .data files on output. Use --memory-init-file=0 to remove .mem files on output. Specify fallbackFont to override default.woff2. If lazyFileLoading is set to true, use FS.createLazyFile(). This is off by default, as it depends on correct HTTP headers sent back.
382baa0
to
7988397
Compare
Contains one commit (7a69b7f) from #145.
These changes extract the embedded font, and add the option to enable lazy font loading (where font resources are mapped, but not downloaded until libass tries to use them).
Additionally,
fallbackFont
option can be used to override the default fallback font that gets used. This way, users can specify their preferred font, including CJK ones (for example Noto Sans CJK .ttc).Enabling lazy font loading is done via
lazyFontLoading
set totrue
, but it defaults tofalse
.This is necessary given some requirements from WASM to not fetch the whole file, specifically:
Access-Control-Expose-Headers: Accept-Ranges, Content-Length, Content-Encoding
(at least)If these conditions match, HEAD request will be issued and file will be accessed via Ranges as-needed. Otherwise content will be fetched as a whole file.
Additionally, this PR enables embedded font extraction support on libass side. This is split on a separate commit.