Build display lists from SkParagraph output using the ParagraphPainter interface #37087
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The test failures in SingleOpSizes are due to changing the size of a DlBlurMaskFilter and the corresponding change in how much space it takes up in the DL. You'll need to adjust the sizes (add 8 bytes - even though you only added a 4-byte field, record alignment pads that to 8) here: The other test failures are side effects of this and should go away when you fix the byte counts for SingleOpSizes. You can run the SingleOpSizes test in isolation until it passes and then the others should pass as well... |
jason-simmons
force-pushed
the
skpara_display_list_roundtrip
branch
from
c2c4046 to
f06ad20
Compare
|
Updated the tests - PTAL |
jason-simmons
force-pushed
the
skpara_display_list_roundtrip
branch
from
f06ad20 to
33b94b1
Compare
| skia.setForegroundColor(txt.foreground); | ||
| skia.setForegroundPaintID(CreatePaintID(txt.foreground_dl)); | ||
| } else { | ||
| flutter::DlPaint dl_paint; |
There was a problem hiding this comment.
This piece is new and I'm not sure why it's needed now...?
There was a problem hiding this comment.
The TextStyle API has color and foreground fields that are somewhat redundant. color sets the color of the text, and foreground sets all of the painting properties of the text including the color. If foreground is set, then it takes precedence over color.
Before this PR the style's color field would be forwarded to its SkPargraph counterpart. Now, if color is used then Flutter must create a DlPaint representing the color.
| if (skia.hasBackground()) { | ||
| txt.background = skia.getBackground(); | ||
| if (skia.hasBackground() && | ||
| std::holds_alternative<SkPaint>(skia.getBackgroundPaintOrID())) { |
There was a problem hiding this comment.
What happens in the PaintID case?
There was a problem hiding this comment.
Added support for mapping a PaintID back to the DlPaint here
| } | ||
| DlPaint paint; | ||
| paint.setColor(color); | ||
| DlBlurMaskFilter filter(SkBlurStyle::kNormal_SkBlurStyle, blurSigma, false); |
There was a problem hiding this comment.
Check blurSigma for non-zero first. (Skia uses != 0.0 but then their MakeBlur factory returns null if sigma isn't > 0, so I'd just use > 0.0 here).
There was a problem hiding this comment.
In this case we will always have a MaskFilter, but it may be a NOP. Didn't this cause any performance regressions?
There was a problem hiding this comment.
Added this check.
AFAIK we do not have a benchmark focused on text shadows. I did not notice any changes in general benchmarks due to this PR. But text shadows are rarely used and would not likely show up there.
| void drawFilledRect(const SkRect& rect, | ||
| const DecorationStyle& decorStyle) override { | ||
| DlPaint paint = toDlPaint(decorStyle); | ||
| paint.setDrawStyle(DlDrawStyle::kFill); |
There was a problem hiding this comment.
Perhaps have toDlPaint take an optional DlDrawStyle which defaults to kStroke?
jason-simmons
force-pushed
the
skpara_display_list_roundtrip
branch
from
33b94b1 to
a03ed24
Compare
…r interface SkPaint does not provide APIs for extracting the definitions of some attributes such as filters. The engine will instead use DlPaint to describe how text foregrounds and backgrounds will be painted. The DlPaint objects will be represented as PaintIDs in SkParagraph text styles. The ParagraphPainter will then map the PaintIDs back to the original DlPaint objects.
jason-simmons
force-pushed
the
skpara_display_list_roundtrip
branch
from
a03ed24 to
d0b04cf
Compare
jason-simmons
added
the
autosubmit
…raphPainter interface (flutter/engine#37087)
…raphPainter interface (flutter/engine#37087)
…raphPainter interface (flutter/engine#37087)
…raphPainter interface (flutter/engine#37087)
…raphPainter interface (flutter/engine#37087)
…raphPainter interface (flutter/engine#37087)
…raphPainter interface (flutter/engine#37087)
…115185) * 645d4e313 Roll Dart SDK from 1e37edb1f6c1 to 2b6909ba35e5 (1 revision) (flutter/engine#37516) * 4cbfe4238 Build display lists from SkParagraph output using the ParagraphPainter interface (flutter/engine#37087) * 05649893f Roll Skia from 77620568f467 to 1a7879e4e26a (6 revisions) (flutter/engine#37520) * f48be3491 Run pub get before building host.dart. (flutter/engine#37502) * e815a60a7 Roll Skia from 1a7879e4e26a to a457da8acd16 (5 revisions) (flutter/engine#37524) * 0f93194b7 Roll ICU from 20f8ac695af5 to da0744861976 (1 revision) (flutter/engine#37525) * 329900fc8 Roll Skia from a457da8acd16 to 29b9dbd7a8ed (1 revision) (flutter/engine#37528)
…r interface (flutter#37087) SkPaint does not provide APIs for extracting the definitions of some attributes such as filters. The engine will instead use DlPaint to describe how text foregrounds and backgrounds will be painted. The DlPaint objects will be represented as PaintIDs in SkParagraph text styles. The ParagraphPainter will then map the PaintIDs back to the original DlPaint objects.
…lutter#115185) * 645d4e313 Roll Dart SDK from 1e37edb1f6c1 to 2b6909ba35e5 (1 revision) (flutter/engine#37516) * 4cbfe4238 Build display lists from SkParagraph output using the ParagraphPainter interface (flutter/engine#37087) * 05649893f Roll Skia from 77620568f467 to 1a7879e4e26a (6 revisions) (flutter/engine#37520) * f48be3491 Run pub get before building host.dart. (flutter/engine#37502) * e815a60a7 Roll Skia from 1a7879e4e26a to a457da8acd16 (5 revisions) (flutter/engine#37524) * 0f93194b7 Roll ICU from 20f8ac695af5 to da0744861976 (1 revision) (flutter/engine#37525) * 329900fc8 Roll Skia from a457da8acd16 to 29b9dbd7a8ed (1 revision) (flutter/engine#37528)
…lutter#115185) * 645d4e313 Roll Dart SDK from 1e37edb1f6c1 to 2b6909ba35e5 (1 revision) (flutter/engine#37516) * 4cbfe4238 Build display lists from SkParagraph output using the ParagraphPainter interface (flutter/engine#37087) * 05649893f Roll Skia from 77620568f467 to 1a7879e4e26a (6 revisions) (flutter/engine#37520) * f48be3491 Run pub get before building host.dart. (flutter/engine#37502) * e815a60a7 Roll Skia from 1a7879e4e26a to a457da8acd16 (5 revisions) (flutter/engine#37524) * 0f93194b7 Roll ICU from 20f8ac695af5 to da0744861976 (1 revision) (flutter/engine#37525) * 329900fc8 Roll Skia from a457da8acd16 to 29b9dbd7a8ed (1 revision) (flutter/engine#37528)
SkPaint does not provide APIs for extracting the definitions of some attributes such as filters. The engine will instead use DlPaint to describe how text foregrounds and backgrounds will be painted. The DlPaint objects will be represented as PaintIDs in SkParagraph text styles. The ParagraphPainter will then map the PaintIDs back to the original DlPaint objects.