Cull rect problem?

I think it's a bug in boundingBoxForCompositing.

PaintLayer::boundingBoxForCompositingInternal produces a crazy initial rect from:

2529│   LayoutRect result = clipper(PaintLayer::DoNotUseGeometryMapper)
2530│                           .localClipRect(compositedLayer);

as:

$12 = {m_location = LayoutPoint(-16777215px, -16777215px), m_size = LayoutSize(33554431px, 33554431px)}

which is then intersected with the logical bounding box of the div, which results in only the latter:

$13 = {m_location = LayoutPoint(0px, 0px), m_size = LayoutSize(784px, 7px)}

This height is insufficient to cover the full 'g'. Investigating PaintLayerClipper::localClipRect seems prudent as a next step.

$12 is infiniteIntRect. Isn't the problem that the logical bounding box
is wrong? Also, where does the transform come into play?

What is the call stack leading to boundingBoxForCompositingInternal?

This issue does not happen on Mac/Win. It may be specific to Linux (I've not tested Android). I've seen similar on Linux where descenders are clipped only when text is selected.

Re: c#4:

Not yet sure where transform is actually incorporated for drawing text, looking into it. Some notes from InlineTextBoxPainter instrumentation:

a) where we emplace the DrawingRecorder:

logicalVisualOverflow="0,0 4x7", paintRect="0,0 4x7"

b) where we subsequently instantiate the TextPainter:

boxRect="0,0 3.5x7"

For reference in dev tools the div box height is 35 px.

Re: c#5:

#2 0x7ffbb64ec434 blink::PaintLayer::boundingBoxForCompositingInternal()
#3 0x7ffbb64eca83 blink::PaintLayer::boundingBoxForCompositing()
#4 0x7ffbb6273b28 blink::CompositedLayerMapping::updateCompositedBounds()
#5 0x7ffbb6274424 blink::CompositedLayerMapping::updateGraphicsLayerConfiguration()
#6 0x7ffbb629319a blink::GraphicsLayerUpdater::updateRecursive()
#7 0x7ffbb62932b1 blink::GraphicsLayerUpdater::updateRecursive()
#8 0x7ffbb62932b1 blink::GraphicsLayerUpdater::updateRecursive()
#9 0x7ffbb6292f66 blink::GraphicsLayerUpdater::update()
#10 0x7ffbb629583b blink::PaintLayerCompositor::updateIfNeeded()
#11 0x7ffbb6294f42 blink::PaintLayerCompositor::updateIfNeededRecursiveInternal()
#12 0x7ffbb6294bb6 blink::PaintLayerCompositor::updateIfNeededRecursive()
#13 0x7ffbb5b73842 blink::FrameView::updateLifecyclePhasesInternal()
#14 0x7ffbb5b73272 blink::FrameView::updateAllLifecyclePhases()
#15 0x7ffbb64483bb blink::PageAnimator::updateAllLifecyclePhases()
#16 0x7ffbb83c5435 blink::PageWidgetDelegate::updateAllLifecyclePhases()
#17 0x7ffbb84d31d4 blink::WebViewImpl::updateAllLifecyclePhases()
#18 0x7ffbb84c9761 blink::WebViewFrameWidget::updateAllLifecyclePhases()
#19 0x7ffbc313c72b content::RenderWidget::UpdateVisualState()
#20 0x7ffbc2f7030a content::RenderWidgetCompositor::UpdateLayerTreeHost()
#21 0x7ffbbbb458ed cc::LayerTreeHost::RequestMainFrameUpdate()
#22 0x7ffbbbc14bac cc::ProxyMain::BeginMainFrame()

Presuming the transform is handled via just placing it on the GraphicsLayer somehow, and paint value computation is all done with pre-transform values, then having a bounding box height of 7px is actually correct given box height ends up being ~35px and scale in the transform is 5.

So I think my previous commentary in c#3 was not really relevant, and probably the bug is something around text metrics bounding box bug/discrepancy on Linux.

Well, if we remove the transform and change font-size to 25pt it looks visually the same size wise but is not clipped. So perhaps we are losing some text metrics fractional values that end up being impactful for bounds/clip purposes when we scale the 5pt font size up via transform.

Research notes on LayoutText::visualOverflowRect metrics. We must go deeper.

Linux:
5pt and scaled
  logicalTop = 0
  logicalBottom = 7
  rect = 0,0 4x7 [note 7 * 5 (scale factor) = 35 and 35 < 37 (see height below)]
25pt
  logicalTop = 0
  logicalBottom = 37
  rect = 0,0 16.5x37

Windows:
5pt and scaled
  logicalTop = 0
  logicalBottom = 8
  rect = -1,0 5x8 [note 8 * 5 (scale factor) = 40 and 40 > 37 (see height below)]
25pt
  logicalTop = 0
  logicalBottom = 37
  rect = 0,0 17x37

drott@ or fmalita@, do you have thought on what would be best for
us to look at next? Detail below and more in bug history.

Summarizing our current understanding so that we can solicit input
from the experts:

- What we believe is happening is that Blink computes bounds of the
  text based on the pre-scaled size. This is what's used to define the
  bounds of its containing div, which will implicitly lead to clipping
  of any text rastered outside of that bounds. See above for involved
  dimensions.

- The bounds Blink-side are, I believe, sourced from harfbuzz using
  the sizing we have Blink-side pre-scaling, which will be 5pt.

- When we finally raster the glyph in Skia, we've applied a scale
  to the SkCanvas. For whatever reason re: Skia/font/platform internals,
  the bounds of the end bitmap do not match a straight multiply of
  the bounds we had from harfbuzz. If the bounds Skia ends up
  using to raster the glyph are larger, this leads to clipping.
  We validate that transform scale is applied at time of glyph
  bitmap blit via logging as:

SkBitmapDevice drawPosText [ctm=[  5.0000   0.0000   0.0000][  0.0000   5.0000   0.0000][  0.0000   0.0000   1.0000]].

And a bit more data though mainly redundant w/ what we already had:

Linux:
  Font::drawText TextRunPaintInfo bounds = 0,0 3.5x7

Win:
  Font::drawText TextRunPaintInfo bounds = 0,0 3.32813x7

Yes, upscaling based on snapped/rounded metrics is a problem. I am not sure where to hook in for a fix there. Koji and Emil may comment in more detail but IIUC the plan is for LayoutNG to keep at least LayoutUnit-grade precision for font metrics. So I would assume that we could improve the upscaled bounding box calculation from that. 

I don't think it makes sense to clip/round-out all bounding boxes at non-transformed sized for fractional metrics values, as that messes with the spacing at the non-transformed size.

The right fix in pre-layout-NG land would probably be to re-measure (and re-raster?) the text at the effective 25pt size and get a higher-resolution result. I believe at least painting does re-raster at the effective actual size when the layer is not animated or so.

Hope this helps at least a little bit.

Yeah, we round ascent and descent to int, so cutting glyphs off is possible. I don't think it's platform dependent, but since platforms may have different font metrics, you may not see it for common fonts on other platforms.

In LayoutNG, as drott@ commented, we're planning to change "round to int" to "ceil to LayoutUnit", so more precision and avoid cut off, but it's too early to tell if doing so can survive web-compat.

hmm...thinking a bit more, there could be two separate issues.

For the font metrics, on Linux, ascent=5.93481 and descent=1.44062, then SkScalarRoundToScalar() each. This gives 7 as the height. I need to check Windows later, but probably it gives slightly higher descent, computes 8 as the height. Different fonts, or the same name fonts on different platforms, having different metrics isn't surprise, so reproducing only on Linux, in this case, is just a coincidence.

However, logically speaking, fonts can have any metrics, independently from what glyphs are. If glyph bounding box overflow metrics, we should return glyph bounding box as visualOverflowRect(). This bug indicates that this mechanism isn't working as expected.

The second problem I think is that, glyph bounding box isn't linear by definition, but IIUC your analysis, we assume it is. The linearity of glyph bounding box depends on platforms, so it's possible that Linux may reproduce more often than Windows. #6 says it's seen before on Linux, but I've never seen this on Windows.

But we don't want to re-compute visualOverflowRect() when transform changes, right? It will involve shaping, so it's not a cheap operation.

I don't know how much additional, unnecessarily larger visualOverflowRect() can degrade our performance, or can cause any other problems, but would it be possible to consider adding margins if >1 scale is applied? We could try to minimize it, such as:
* As above, it depends on platform. We could determine heuristically for each platform.
* 1px * scale factor is probably ok, though, as above, this is also heuristic. Fonts are allowed to use larger difference by size.
* Fonts can provide hints if needed. From this case, either original ascent/descent having "0.44" fraction is likely to cause this problem when scaled.

This could cover both rounding problem and non-linear glyph bounding box problem.

WDYT?

Thanks for the research, this is helpful.

On this part: "...we don't want to re-compute visualOverflowRect() when transform changes, right? It will involve shaping, so it's not a cheap operation." my semi-naive thought is, if we're wrong without recomputing it, I'd say we should recompute it. And given we have shaper caching it shouldn't be too bad.

My hesitation would be that the involved Blink transform computation seem to all rely on consistently not incorporating the css transform Blink-side, and rather trusting that it's all taken care of compositor-side. So if we think we need to embark on modifying/specializing logic in Blink just for this case, we should consider carefully and make sure the complexity and maintenance tradeoff ongoing is worth it. And with LayoutNG in work I would default vote for just waiting for that to develop unless we see greater real world impact than this sample test case.

"... If glyph bounding box overflow metrics, we should return glyph bounding box as visualOverflowRect(). This bug indicates that this mechanism isn't working as expected."

This could be worth looking into more closely, interesting.

"...would it be possible to consider adding margins if >1 scale is applied?"

I'd be open to something like that as an interim fix until LayoutNG + more robust solution but naively it seems it would be prone to web compat issue.

> my semi-naive thought is, if we're wrong without recomputing it, I'd say we should recompute it. And given we have shaper caching it shouldn't be too bad.

I see, and you're right. Let me take back #13. With your comments, I looked into the code a bit more, and learned that we do have a bunch of code to recompute overflow already.

Normally, we compute glyph overflow during layout, and propagate them to box's visual overflow, so I thought it'd not be easy to compute visual overflow for inline without performing layout.

I was wrong. We have duplicated part of layout code to compute glyph overflow without performing full layout. What it does looks reasonable overall, more than that requires stepping through it.

But in short, if glyphs are overflowing what LayoutText::visualOverflowRect() returns, my current best assessment is that it should be a problem in the logic mentioned above, or in LayoutText::visualOverflowRect().

kojii@ can you point me to the code you mention above re: "We have duplicated part of layout code to compute glyph overflow without performing full layout"?

Sorry for a slow response. My code reading so far is:
1. LayoutObject::SetStyle() calls SetNeedsOverflowRecalcAfterStyleChange() if diff.TransformChanged().
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutObject.cpp?type=cs&l=1734
2. We then go LayoutBlock::RecalcOverflowAfterStyleChange()
3. LayoutBlock::RecalcChildOverflowAfterStyleChange()
4. Since it's ChildrenInline(), it should go to LayoutBlockFlow::RecalcInlineChildrenOverflowAfterStyleChange()
  Here, we have a FIXME saying:
  // FIXME: Glyph overflow will get lost in this case, but not really a big
  // deal.
5. InlineFlowBox::ComputeOverflow
6. If IsText(), if the given map is empty, it calls static void ComputeGlyphOverflow(), which does re-shape.

So my best guess at this point is, there's a logic error somewhere in this path which prevents us going down to #6, or maybe #6 computes glyph overflows correctly for InlineTextBox but #4 discards it without propagating to LayoutBlockFlow as the FIXME indicates.

I didn't really debug-through the code, sorry if this was inaccurate in advance.

Issue 707807 could be related.

Unassigning self from bugs that I don't expect to be able to get to soon in case someone else is able to pick them up.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Looks fine now. Closing.