Bug 89572 - Breaking LayoutUnits down to ints for baseline position and ascents needs rounding
Summary: Breaking LayoutUnits down to ints for baseline position and ascents needs rou...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominik Röttsches (drott)
URL:
Keywords:
Depends on:
Blocks: 60318 89458 94792
  Show dependency treegraph
 
Reported: 2012-06-20 07:56 PDT by Dominik Röttsches (drott)
Modified: 2013-03-01 02:44 PST (History)
8 users (show)

See Also:


Attachments
Example test failure on EFL with ENABLE_SUBPIXEL_LAYOUT (1.48 KB, text/html)
2012-06-20 07:57 PDT, Dominik Röttsches (drott)
no flags Details
Rounding for ascent calculation. (2.95 KB, patch)
2012-06-20 08:10 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.34 MB, application/zip)
2012-06-20 10:10 PDT, WebKit Review Bot
no flags Details
fast/forms/form-element-geometry.html failure on Linux Debug (17.54 KB, text/html)
2012-06-26 05:30 PDT, Dominik Röttsches (drott)
no flags Details
Patch attempt using LayoutUnits (1.51 KB, patch)
2012-08-23 03:41 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-06-20 07:56:11 PDT
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?
Comment 1 Dominik Röttsches (drott) 2012-06-20 07:57:46 PDT
Created attachment 148568 [details]
Example test failure on EFL with ENABLE_SUBPIXEL_LAYOUT
Comment 2 Emil A Eklund 2012-06-20 08:02:04 PDT
(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.
Comment 3 Dominik Röttsches (drott) 2012-06-20 08:10:44 PDT
Created attachment 148570 [details]
Rounding for ascent calculation.
Comment 4 Eric Seidel (no email) 2012-06-20 08:15:38 PDT
This is really best for Levi to review.
Comment 5 WebKit Review Bot 2012-06-20 10:10:40 PDT
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
Comment 6 WebKit Review Bot 2012-06-20 10:10:46 PDT
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
Comment 7 Levi Weintraub 2012-06-20 10:19:01 PDT
(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.
Comment 8 Dominik Röttsches (drott) 2012-06-20 11:41:47 PDT
(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.
Comment 9 Levi Weintraub 2012-06-22 10:58:57 PDT
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.
Comment 10 Dominik Röttsches (drott) 2012-06-25 02:52:08 PDT
(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?
Comment 11 Dominik Röttsches (drott) 2012-06-25 02:56:26 PDT
(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.
Comment 12 Dominik Röttsches (drott) 2012-06-26 05:30:56 PDT
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?
Comment 13 Dominik Röttsches (drott) 2012-06-27 05:01:52 PDT
(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;
Comment 14 Emil A Eklund 2012-06-27 09:18:20 PDT
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.
Comment 15 Emil A Eklund 2012-06-27 11:30:23 PDT
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.
Comment 16 Dominik Röttsches (drott) 2012-06-27 13:08:58 PDT
(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.
Comment 17 Dominik Röttsches (drott) 2012-07-09 05:08:27 PDT
(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?
Comment 18 Emil A Eklund 2012-07-18 16:17:23 PDT
(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.
Comment 19 Emil A Eklund 2012-07-18 19:21:08 PDT
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.
Comment 20 Dominik Röttsches (drott) 2012-08-17 05:23:07 PDT
Ping? :-)
Comment 21 Emil A Eklund 2012-08-21 10:59:20 PDT
The test differences all look good to me but to submit this we'll need to rebaseline a couple of thousand tests for chromium.
Comment 22 Dominik Röttsches (drott) 2012-08-22 03:59:42 PDT
Yes, when can we do that? I can invite the gardeners/sheriffs who help with that for dinner :-)
Comment 23 Xianzhu Wang 2012-08-22 08:50:38 PDT
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.
Comment 24 Allan Sandfeld Jensen 2012-08-22 14:12:53 PDT
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.
Comment 25 Dominik Röttsches (drott) 2012-08-23 03:41:27 PDT
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.
Comment 26 Dominik Röttsches (drott) 2012-10-23 01:10:09 PDT
This patch might not be needed anymore after bug 99767 was landed in r132112.
Comment 27 Dominik Röttsches (drott) 2012-10-23 01:56:39 PDT
Looks like r132112 has at least not helped to bring the required rebaselines on EFL down, rather up. :-(
Comment 28 Emil A Eklund 2012-10-23 09:37:56 PDT
(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 :(
Comment 29 Build Bot 2013-01-16 06:19:39 PST
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
Comment 30 Dominik Röttsches (drott) 2013-01-16 06:56:34 PST
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.
Comment 31 Eric Seidel (no email) 2013-03-01 02:44:14 PST
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).