WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
234171
[Cocoa] OT-SVG glyphs don't draw into canvases (because of the GPU process)
https://bugs.webkit.org/show_bug.cgi?id=234171
Summary
[Cocoa] OT-SVG glyphs don't draw into canvases (because of the GPU process)
Myles C. Maxfield
Reported
2021-12-10 13:23:43 PST
[Cocoa] OT-SVG glyphs don't draw into canvases
Attachments
Patch
(18.17 KB, patch)
2021-12-10 13:30 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(18.00 KB, patch)
2021-12-10 13:31 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(18.79 KB, patch)
2021-12-10 14:13 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(19.20 KB, patch)
2021-12-10 14:17 PST
,
Myles C. Maxfield
hi
: review+
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2021-12-10 13:30:28 PST
Created
attachment 446801
[details]
Patch
Myles C. Maxfield
Comment 2
2021-12-10 13:30:31 PST
<
rdar://problem/70166552
>
Myles C. Maxfield
Comment 3
2021-12-10 13:31:16 PST
Created
attachment 446802
[details]
Patch
Myles C. Maxfield
Comment 4
2021-12-10 14:13:31 PST
Created
attachment 446809
[details]
Patch
Myles C. Maxfield
Comment 5
2021-12-10 14:17:55 PST
Created
attachment 446811
[details]
Patch
Devin Rousso
Comment 6
2021-12-10 16:01:55 PST
Comment on
attachment 446811
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=446811&action=review
r=me, nice work! I won't claim to fully understand the motivation, but the logic looks sound :)
> Source/WebCore/platform/graphics/ImageBuffer.cpp:101 > +auto ImageBuffer::createCompatibleBuffer(const FloatRect& rect, const GraphicsContext& context) -> std::optional<CompatibleBufferDescription>
Aside: Why use `auto` instead of `std::optional<CompatibleBufferDescription>`?
> Source/WebCore/platform/graphics/ImageBuffer.cpp:195 > +auto ImageBuffer::compatibleBufferInfo(const FloatRect& rect, const GraphicsContext& context) -> CompatibleBufferInfo
ditto (:101)
> Source/WebCore/platform/graphics/ImageBuffer.cpp:207 > + // In practice, this should almost never happen - individual glyphs inside text pretty much > + // never end up this big.
NIT: this is assuming that this function is only ever called when dealing with glyphs, but there's nothing in the header that indicates that
> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:379 > + if (auto imageBufferInfo = ImageBuffer::createCompatibleBuffer(bounds, m_owner)) {
NIT: Should this be `imageBufferDescription` since `ImageBuffer::createCompatibleBuffer` returns a `std::optional<CompatibleBufferDescription>`, not a `CompatibleBufferInfo`?
> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:380 > + FontCascade::drawGlyphs(imageBufferInfo->imageBuffer->context(), font, glyphs + i, advances + i, 1, FloatPoint(), smoothingMode);
Does this also need a `prepareInternalContext(font, smoothingMode);` and `concludeInternalContext();` somewhere? Style: Why not `glyphs[i]` and `advances[i]`?
> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:398 > auto otsvgGlyphs = font.findOTSVGGlyphs(glyphs, numGlyphs);
Aside: as a (future) optimization it would be nice to avoid this and instead only have to do one iteration of `glyphs` (i.e. add a `Font::isOTSVGGlyph(GlyphBufferGlyph)` that you call in the `for` below)
> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:411 > + bool state = false;
NIT: How about `isOTSVGRun`?
> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:417 > + drawOTSVGRun(font, glyphs + i - glyphCountInRun, advances + i - glyphCountInRun, glyphCountInRun, runOrigin, smoothingMode);
ditto (:380)
> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:419 > + drawNonOTSVGRun(font, glyphs + i - glyphCountInRun, advances + i - glyphCountInRun, glyphCountInRun, runOrigin, smoothingMode);
ditto (:380)
Myles C. Maxfield
Comment 7
2021-12-10 16:12:03 PST
Comment on
attachment 446811
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=446811&action=review
>> Source/WebCore/platform/graphics/ImageBuffer.cpp:101 >> +auto ImageBuffer::createCompatibleBuffer(const FloatRect& rect, const GraphicsContext& context) -> std::optional<CompatibleBufferDescription> > > Aside: Why use `auto` instead of `std::optional<CompatibleBufferDescription>`?
CompatibleBufferDescription is defined inside ImageBuffer, and C++ says the scope starts at the word "ImageBuffer::" so because this is on the left of that it would have had to be std::optional<ImageBuffer::CompatibleBufferDescription>. Moving it to the right side means you can drop the ImageBuffer::
>> Source/WebCore/platform/graphics/ImageBuffer.cpp:207 >> + // never end up this big. > > NIT: this is assuming that this function is only ever called when dealing with glyphs, but there's nothing in the header that indicates that
Good point! Yep, I'll fix it.
>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:379 >> + if (auto imageBufferInfo = ImageBuffer::createCompatibleBuffer(bounds, m_owner)) { > > NIT: Should this be `imageBufferDescription` since `ImageBuffer::createCompatibleBuffer` returns a `std::optional<CompatibleBufferDescription>`, not a `CompatibleBufferInfo`?
👍
>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:380 >> + FontCascade::drawGlyphs(imageBufferInfo->imageBuffer->context(), font, glyphs + i, advances + i, 1, FloatPoint(), smoothingMode); > > Does this also need a `prepareInternalContext(font, smoothingMode);` and `concludeInternalContext();` somewhere? > > Style: Why not `glyphs[i]` and `advances[i]`?
It doesn't, because it doesn't use m_internalContext. This draws into the ImageBuffer's context instead. These parameters are pointer type. Do you think &(glyphs[i]) is more readable than glyphs + i?
>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:398 >> auto otsvgGlyphs = font.findOTSVGGlyphs(glyphs, numGlyphs); > > Aside: as a (future) optimization it would be nice to avoid this and instead only have to do one iteration of `glyphs` (i.e. add a `Font::isOTSVGGlyph(GlyphBufferGlyph)` that you call in the `for` below)
Let's chat offline about this. I'm curious more more details.
>> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:411 >> + bool state = false; > > NIT: How about `isOTSVGRun`?
🆒
Myles C. Maxfield
Comment 8
2021-12-10 16:53:48 PST
Committed
r286889
(
245117@trunk
): <
https://commits.webkit.org/245117@trunk
>
Myles C. Maxfield
Comment 9
2021-12-11 00:01:30 PST
Committed
r286906
(?): <
https://commits.webkit.org/r286906
>
Said Abou-Hallawa
Comment 10
2022-01-12 12:09:58 PST
Comment on
attachment 446811
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=446811&action=review
> Source/WebCore/platform/graphics/ImageBuffer.cpp:225 > +}
Maybe I misunderstand something here. But my understanding is this function returns 1. The size of the ImageBuffer in backend coordinates after clamping it if needed 2. The enclosing rectangle of the ImageBuffer in logical coordinates 3. The scaling factor which is the context scaling factor + the clamping scaling factor if applied. If my understanding is correct, I think this function can be written like this: static CompatibleBufferInfo compatibleBufferInfo(const FloatRect& rect, const GraphicsContext& context) { auto scaleFactor = context.scaleFactor(); ImageBuffer::sizeNeedsClamping(rect.size(), scaleFactor); auto backendSize = rect.size() * scaleFactor; return { expandedIntSize(backendSize), enclosingIntRect(rect), scaleFactor }; } In fact I think the function and the struct CompatibleBufferInfo are not needed. These few lines can be moved to the only caller createCompatibleBuffer().
> Source/WebCore/platform/graphics/ImageBuffer.h:76 > + struct CompatibleBufferInfo { > + IntSize physicalSizeInDeviceCoordinates; > + FloatRect inflatedRectInUserCoordinates; > + FloatSize scale; > + }; > + static CompatibleBufferInfo compatibleBufferInfo(const FloatRect&, const GraphicsContext&);
This function and this struct can be moved to ImageBuffer.cpp since they are only used internally by ImageBuffer.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug