Bug 89572

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

(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.

Created attachment 148570 [details] Rounding for ascent calculation.

This is really best for Levi to review.

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

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

(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.

(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.

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.

(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?

(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.

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?

(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;

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.

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.

(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.

(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?

(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.

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.

Ping? :-)

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

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

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 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.

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.

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

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

(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 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

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 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).