Bug 89572 – Breaking LayoutUnits down to ints for baseline position and ascents needs rounding

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[Dominik Röttsches (drott)]

When activating sub-pixel layout on EFL, I found that in a lot of cases, there was a vertical offset of -1 from the baseline (I attached an example compositing_generated-content-wdiff.html). This results from a by-one-mismatch between the return value of RenderBlock::baselinePosition (which is assigned to maxAscent later), and fontMetrics.ascent(baselineType).

InlineFlowBox::placeBoxesInBlockDirection sets the logical top:
setLogicalTop(roundToInt(top + maxAscent - fontMetrics.ascent(baselineType)));

Here, maxAscent in this particular case is 14, while the ascent is 15 - shifting the logical top up to -1.

In my understanding, the reason is the default toInt() of LayoutUnit being called when the RenderBlock::baselinePosition() is assigned to ascent values in 
RootInlineBox::ascentAndDescentForBox. If we consistently use rounding here, I can get the test failures on EFL down from initially around 3800 to 563. My understanding is still limited - but I guess that the proper fix would be to also transition all the ascent/descent/maxAscent/maxDescent calculations to LayoutUnits and round later? Is that something that you're planning to do, Levi & Emil?

[Dominik Röttsches (drott)]

Created attachment 148568 [details]
Example test failure on EFL with ENABLE_SUBPIXEL_LAYOUT

[Emil A Eklund]

(In reply to comment #0)
> In my understanding, the reason is the default toInt() of LayoutUnit being called when the RenderBlock::baselinePosition() is assigned to ascent values in 
> RootInlineBox::ascentAndDescentForBox. If we consistently use rounding here, I can get the test failures on EFL down from initially around 3800 to 563.

Good catch, it really seems we should fix that.

>My understanding is still limited - but I guess that the proper fix would be to also transition all the ascent/descent/maxAscent/maxDescent calculations to LayoutUnits and round later? Is that something that you're planning to do, Levi & Emil?

Yes, we plan to transition ascent/descent to LayoutUnits once the majority of the ports has enabled subpixel layout.

[Dominik Röttsches (drott)]

Created attachment 148570 [details]
Rounding for ascent calculation.

[Eric Seidel (no email)]

This is really best for Levi to review.

[WebKit Review Bot]

Comment on attachment 148570 [details]
Rounding for ascent calculation.

Attachment 148570 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13008245

New failing tests:
compositing/geometry/abs-position-inside-opacity.html
compositing/geometry/fixed-in-composited.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/direct-image-compositing.html
compositing/geometry/fixed-position-transform-composited-page-scale-down.html
compositing/geometry/fixed-position-transform-composited-page-scale.html
animations/additive-transform-animations.html
compositing/text-on-large-layer.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/sibling-positioning.html
compositing/generated-content.html
compositing/self-painting-layers.html
animations/cross-fade-webkit-mask-box-image.html
animations/3d/change-transform-in-end-event.html
animations/missing-values-last-keyframe.html
animations/cross-fade-list-style-image.html
animations/missing-values-first-keyframe.html
compositing/geometry/fixed-position-composited-page-scale.html
animations/cross-fade-background-image.html
compositing/geometry/clipping-foreground.html
animations/cross-fade-border-image-source.html
compositing/geometry/composited-html-size.html
compositing/compositing-visible-descendant.html
animations/cross-fade-webkit-mask-image.html
compositing/color-matching/image-color-matching.html
compositing/color-matching/pdf-image-match.html

[WebKit Review Bot]

Created attachment 148592 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Levi Weintraub]

(In reply to comment #2)
> (In reply to comment #0)
> > In my understanding, the reason is the default toInt() of LayoutUnit being called when the RenderBlock::baselinePosition() is assigned to ascent values in 
> > RootInlineBox::ascentAndDescentForBox. If we consistently use rounding here, I can get the test failures on EFL down from initially around 3800 to 563.

Sounds like an improvement... sadly this would mean tons of rebaselining for Chromium :( I need to look at the test diffs (not just the first 20 the EWS bot spits out) to feel more comfortable.
 
> >My understanding is still limited - but I guess that the proper fix would be to also transition all the ascent/descent/maxAscent/maxDescent calculations to LayoutUnits and round later? Is that something that you're planning to do, Levi & Emil?
> 
> Yes, we plan to transition ascent/descent to LayoutUnits once the majority of the ports has enabled subpixel layout.

These plans are news to me, but the notion seems quite reasonable.

[Dominik Röttsches (drott)]

(In reply to comment #7)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > In my understanding, the reason is the default toInt() of LayoutUnit being called when the RenderBlock::baselinePosition() is assigned to ascent values in 
> > > RootInlineBox::ascentAndDescentForBox. If we consistently use rounding here, I can get the test failures on EFL down from initially around 3800 to 563.
> 
> Sounds like an improvement... sadly this would mean tons of rebaselining for Chromium :( I need to look at the test diffs (not just the first 20 the EWS bot spits out) to feel more comfortable.

Thanks - yes, it'd be great if you can dig into those. I was (sadly, as well) expecting this might require a number of Chromium cases to be rebaselined - or effectively be reverted to the previous baseline before subpixel layout enabled. On the positive side - it probably means less rebaselining effort for new ports that activate the subpixel feature.

[Levi Weintraub]

I tried this locally on Chromium Mac. It causes 182 tests to fail (not unexpected), but at least one new result -- fast/forms/form-element-geometry.html -- appears wrong.

[Dominik Röttsches (drott)]

(In reply to comment #9)
> I tried this locally on Chromium Mac. It causes 182 tests to fail (not unexpected), but at least one new result -- fast/forms/form-element-geometry.html -- appears wrong.

Thanks for checking - could you elaborate a bit on what you think goes wrong for that case?

[Dominik Röttsches (drott)]

(In reply to comment #9)
> I tried this locally on Chromium Mac. It causes 182 tests to fail (not unexpected), but at least one new result -- fast/forms/form-element-geometry.html -- appears wrong.

Btw, on EFL, I see no change for this test case - it passes with the same expected result before and after this change I'm proposing.

[Dominik Röttsches (drott)]

Created attachment 149514 [details]
fast/forms/form-element-geometry.html failure on Linux Debug

(In reply to comment #9)
> I tried this locally on Chromium Mac. It causes 182 tests to fail (not unexpected), but at least one new result -- fast/forms/form-element-geometry.html -- appears wrong.

I don't have access to a mac build right now. I ran this test on r121252 chromium linux 64bit debug new-run-webkit-tests (built from a webkit checkout using update-webkit --chromium + build-webkit) + my rounding patch. 

Is the attached diff similar to what you see on mac? 
Can you explain what do you consider wrong? I assume the vertical by-one offsets for RenderText and text runs?

[Dominik Röttsches (drott)]

(In reply to comment #9)
> I tried this locally on Chromium Mac. It causes 182 tests to fail (not unexpected), but at least one new result -- fast/forms/form-element-geometry.html -- appears wrong.

Levi, Emil - I'd really appreciate if you could comment and explain in more detail what you think goes wrong in this case.

I am trying to make it easier for new ports to enable subpixel rendering by reducing the amount of rebaselining that's required. I think that's something you would probably have an interest in as well.

If the issue is the by-one difference: As far as I was able to find out, the issue originates from RenderBlock::baselinePosition which averages lineHeight & fontHeigth, resulting in a fractional componnet of .5 - which then is rounded up to 1, differing by one, because of which the Text Run is moved 1 pixel down. To me that doesn't seem wrong.

return fontMetrics.ascent(baselineType) + (lineHeight(firstLine, direction, linePositionMode) - fontMetrics.height()) / 2;

[Emil A Eklund]

Levi is away for the next couple of weeks and asked me to help with this while he's gone. I plan to start looking into the chromium failures with your change later today.

Thank you.

[Emil A Eklund]

This change causes several thousand (8000+) tests to fail on chromium linux, will take some time to go through them all to see if any of them are regressions.

[Dominik Röttsches (drott)]

(In reply to comment #15)
> This change causes several thousand (8000+) tests to fail on chromium linux, will take some time to go through them all to see if any of them are regressions.

Thanks Emil, really appreciate it. I will look through the remaining ~500 in EFL that fail after enabling subpixel layout and see if I find issues there.

[Dominik Röttsches (drott)]

(In reply to comment #15)
> This change causes several thousand (8000+) tests to fail on chromium linux, will take some time to go through them all to see if any of them are regressions.

Any news on this, Emil?

[Emil A Eklund]

(In reply to comment #17)
> (In reply to comment #15)
> > This change causes several thousand (8000+) tests to fail on chromium linux, will take some time to go through them all to see if any of them are regressions.
> 
> Any news on this, Emil?

Just started going through the list of tests again, so far I'm about half way through and it looks good.

[Emil A Eklund]

I've looked through about 6000 of the 8000 tests at this point and they all look good on linux. Including the form tests that levi was worried about. I'll do a quick scan on chromium mac tomorrow. My biggest concern at this time is how we make the change and update the expectations without breaking the world.

[Dominik Röttsches (drott)]

Ping? :-)

[Emil A Eklund]

The test differences all look good to me but to submit this we'll need to rebaseline a couple of thousand tests for chromium.

[Dominik Röttsches (drott)]

Yes, when can we do that? I can invite the gardeners/sheriffs who help with that for dinner :-)

[Xianzhu Wang]

Comment on attachment 148570 [details]
Rounding for ascent calculation.

View in context: https://bugs.webkit.org/attachment.cgi?id=148570&action=review

> Source/WebCore/rendering/RootInlineBox.cpp:733
> +            descent = roundToInt(box->lineHeight()) - ascent;

I wonder if the rounding would cause the same bug as in https://bugs.webkit.org/show_bug.cgi?id=88684.

[Allan Sandfeld Jensen]

Comment on attachment 148570 [details]
Rounding for ascent calculation.

View in context: https://bugs.webkit.org/attachment.cgi?id=148570&action=review

>> Source/WebCore/rendering/RootInlineBox.cpp:733
>> -            ascent = box->baselinePosition(baselineType());
>> -            descent = box->lineHeight() - ascent;
>> +            ascent = roundToInt(box->baselinePosition(baselineType()));
>> +            descent = roundToInt(box->lineHeight()) - ascent;
> 
> I wonder if the rounding would cause the same bug as in https://bugs.webkit.org/show_bug.cgi?id=88684.

You might want to calculate the descent in LayoutUnit before rounding. Imagine ascent is rounded up and lineHeight down, you can end up with descent off its accurate value by just short of one.

> Source/WebCore/rendering/RootInlineBox.cpp:783
> +        int ascentWithLeading = roundToInt(box->baselinePosition(baselineType()));
> +        int descentWithLeading = roundToInt(box->lineHeight() - ascentWithLeading);

Same here.

[Dominik Röttsches (drott)]

Created attachment 160124 [details]
Patch attempt using LayoutUnits

(In reply to comment #24)
> (From update of attachment 148570 [details])
> You might want to calculate the descent in LayoutUnit before rounding. Imagine ascent is rounded up and lineHeight down, you can end up with descent off its accurate value by just short of one.

I've tried this approach (see this patch), and I'd be back to ~3800 failures on EFL. The underlying bigger problem is that ascents and descents are still ints, see my first comment on this bug. At least in my understanding, until this is fixed the benefit of doing it the earlier way is that it's closer to what the integer/non-subpixel version does and thus causing less need for rebaselines when enabling subpixel rendering.

[Dominik Röttsches (drott)]

This patch might not be needed anymore after bug 99767 was landed in r132112.

[Dominik Röttsches (drott)]

Looks like r132112 has at least not helped to bring the required rebaselines on EFL down, rather up. :-(

[Emil A Eklund]

(In reply to comment #27)
> Looks like r132112 has at least not helped to bring the required rebaselines on EFL down, rather up. :-(

Oh noes :(

[Build Bot]

Comment on attachment 148570 [details]
Rounding for ascent calculation.

Attachment 148570 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15908414

New failing tests:
mathml/presentation/fractions.xhtml
css3/flexbox/flexbox-baseline.html
fast/borders/fieldsetBorderRadius.html
fast/block/basic/fieldset-stretch-to-legend.html
mathml/presentation/subsup.xhtml
http/tests/security/cross-origin-plugin-private-browsing-toggled.html
fast/forms/basic-buttons.html

[Dominik Röttsches (drott)]

Patch and approach have become invalid since the original code has been changed, also, the root cause of this issue was more likely a baseline / ascent calculation bug in freetyp <= 2.4.5.

[Eric Seidel (no email)]

Comment on attachment 148570 [details]
Rounding for ascent calculation.

Cleared review? from attachment 148570 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).