-
Notifications
You must be signed in to change notification settings - Fork 116
Refactor Web platform font rendering to support FreeType coexistence and optimize glyph rasterization. #1272
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
…e in web multi-threaded mode.
…to feature/thunderllei_testtest
… in uploadToTexture.
… implementations into platform/web directory.
Hparty
left a comment
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.
Suggestion: Unify upload logic by pushing platform variance into CellDecodeTask
Currently WebAtlasUploadTask maintains a separate directUploadCells list alongside the base class tasks list, and upload() has to execute two independent loops. The asyncSupport() check in addCell is also semantically mismatched — it means "supports multithreading", not "supports direct upload".
Suggested approach
-
Add a virtual
upload(shared_ptr<Texture>, CommandQueue*)method toCellDecodeTask. The base implementation doesqueue->writeTexture(texture, atlasRect, pixels, rowBytes)as before. -
Create
DirectUploadCellTask(insrc/platform/web/) that holds ashared_ptr<WebImageBuffer>and overridesupload()to callwebBuffer->uploadToTexture(texture, offsetX, offsetY). -
WebAtlasUploadTaskonly overridesaddCell: for non-async codecs, callcodec->makeBuffer(), wrap the result in aDirectUploadCellTask, and push it into the base classtaskslist. Otherwise fall through to the baseaddCell. -
AtlasUploadTask::uploadbecomes a single unified loop with no override needed:
for (auto& task : tasks) {
task->wait();
if (!hardwarePixels) {
task->upload(texture, queue); // polymorphic dispatch
}
}- Delete
DirectUploadCellstruct,directUploadCellsmember, andWebAtlasUploadTask::uploadoverride.
This eliminates the dual-list / dual-loop pattern, keeps platform-specific logic isolated in WebAtlasUploadTask::addCell + DirectUploadCellTask::upload, and the static_pointer_cast<WebImageBuffer> is contained within src/platform/web/ (consistent with the static_pointer_cast<GLTexture> pattern used elsewhere).
Hparty
left a comment
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.
Additional Review Comments
1. uploadToTextureRegion uses gl.RED while uploadToTexture uses gl.ALPHA for alphaOnly
In web/src/tgfx.ts, the two upload functions handle alphaOnly textures differently:
uploadToTexture(line 97):gl.ALPHAuploadToTextureRegion(line 119):gl.RED
gl.RED is WebGL2-only. If WebGL1 compatibility is needed, this will fail. Either way, the inconsistency between the two functions for the same scenario looks unintentional.
2. Typo in GlyphRasterizer.h
Line 27: A Rasterizer that rasterizes a give Glyph. → should be a given Glyph.
3. getGlyphCanvas holds canvas objects until upload completes
Unlike readPixels which renders and immediately extracts pixels then calls releaseCanvas2D, getGlyphCanvas returns a canvas held by WebImageBuffer::MakeAdopted until upload finishes. If many new glyphs are added in a single frame, this could exhaust the canvas pool and cause frequent DOM element creation.
4. WebScalerContext::getGlyphCanvas comment says "Returns null"
The comment says "Returns null if the glyph cannot be rendered", but the actual return is emscripten::val::null(). In C++ context, "null" typically means nullptr. Consider clarifying to Returns val::null() if the glyph cannot be rendered.
Hparty
left a comment
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.
ScalerContext::asyncSupport() can be replaced with #ifdef
The newly added ScalerContext::asyncSupport() virtual method is only overridden by WebScalerContext (returns false), and its only call site is GlyphRasterizer::Make to decide between GlyphRasterizer and WebGlyphRasterizer. This is effectively a compile-time platform check disguised as a runtime virtual call.
Since WebScalerContext only exists on Web, this can be a simple #ifdef TGFX_BUILD_FOR_WEB branch in GlyphRasterizer::Make, consistent with how AtlasUploadTask::Make already handles the WebAtlasUploadTask split. This avoids adding an asyncSupport() method to ScalerContext whose semantics are vague and only serve one platform.
Hparty
left a comment
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.
Additional Suggestions
1. GlyphRasterizer members exposed as protected — consider using getters
All members (scalerContext, glyphID, fauxBold, stroke, glyphOffset) were changed from private to protected for WebGlyphRasterizer access. However glyphOffset is not used by WebGlyphRasterizer at all. Consider providing protected getter methods instead of exposing raw member variables, which gives better control over access.
2. uploadToTexture and uploadToTextureRegion are nearly identical
In web/src/tgfx.ts, both functions share the same logic (bind texture → set premultiply → choose format by alphaOnly → texSubImage2D → reset premultiply). The only difference is the offset parameters. uploadToTexture could simply call uploadToTextureRegion(GL, source, textureID, 0, 0, alphaOnly) to eliminate duplication.
3. Stroke handling in WebGlyphRasterizer::onMakeBuffer
GlyphRasterizer stores stroke as a Stroke object (via std::optional or direct member), while WebScalerContext::getGlyphCanvas takes const Stroke*. Please verify that the null/no-stroke case is handled correctly when passing the member to getGlyphCanvas — i.e., that a default-constructed Stroke doesn't accidentally produce stroke rendering when fill was intended.
异步解码 codec → 写入 dstPixels DirectUploadCellTask 的 onExecute 什么都不做(不需要解码)
扩大 CellDecodeTask 的可见性(原本是实现细节) |
在 Web 多线程构建中(TGFX_BUILD_FOR_WEB + TGFX_USE_FREETYPE 同时定义): FTTypeface → FTScalerContext → asyncSupport() 返回 true → 走标准 GlyphRasterizer(异步解码) |
1、这个不一致不是 bug,而是两个函数服务于不同场景——一个上传到新建的 RGBA 纹理,另一个上传到已有的 Atlas 纹理(可能是 R8 格式)。 |
1、把 glyphOffset 留在 private,其余四个保持 protected 即可。不需要加 getter。 |
src/gpu/tasks/AtlasUploadTask.h
Outdated
| virtual void upload(std::shared_ptr<Texture> texture, CommandQueue* queue) = 0; | ||
|
|
||
| virtual void cancel() { | ||
| } |
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.
CellUploadTask 不需要 cancel() 接口。cancel() 的唯一目的是在 AtlasUploadTask 析构时阻止线程池对已释放的 dstPixels 执行写入,这是 AsyncCellUploadTask 自身的资源管理问题,应该由它自己的析构函数负责:
~AsyncCellUploadTask() override {
Task::cancel();
}这样 AtlasUploadTask::~AtlasUploadTask 里的 cancel 循环也可以删掉,让 cellTasks 自然析构即可。DirectCellUploadTask 没有异步操作,析构时自然释放资源,不需要任何额外处理。
CellUploadTask 就只保留一个纯粹的 upload() 接口,职责更干净。
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.
done
src/platform/web/WebImageBuffer.cpp
Outdated
| } | ||
|
|
||
| std::shared_ptr<TextureView> WebImageBuffer::onMakeTexture(Context* context, bool) const { | ||
| auto textureView = TextureView::MakeRGBA(context, width(), height(), nullptr, 0); |
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.
onMakeTexture 始终创建 RGBA 纹理,但现在 _alphaOnly 可以为 true(通过 MakeAdopted(canvas, isAlphaOnly()))。后续 uploadToTexture 会按 _alphaOnly 选择 gl.RED 格式上传,向 RGBA 纹理写入 gl.RED 数据会格式不匹配。
当前 glyph 直传走 atlas 路径不经过 onMakeTexture,暂时不会触发。但建议按 _alphaOnly 选择纹理格式,避免未来踩坑:
std::shared_ptr<TextureView> textureView = nullptr;
if (_alphaOnly) {
textureView = TextureView::MakeAlpha(context, width(), height(), nullptr, 0);
} else {
textureView = TextureView::MakeRGBA(context, width(), height(), nullptr, 0);
}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.
done
Hparty
left a comment
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.
LGTM
重构 Web 平台字体渲染机制,将 WebTypeface 相关代码移至 platform/web 目录,实现与 FreeType 的共存支持。优化字形光栅化流程,引入 WebGlyphRasterizer 和 WebAtlasUploadTask 子类,提升 Canvas 直接上传纹理的性能。