Bug 131144 - Subpixel rendering: Move background images to device pixel boundaries.
Summary: Subpixel rendering: Move background images to device pixel boundaries.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks: 131209
  Show dependency treegraph
 
Reported: 2014-04-02 20:46 PDT by zalan
Modified: 2020-01-30 05:45 PST (History)
13 users (show)

See Also:


Attachments
Patch (64.99 KB, patch)
2014-04-02 21:21 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (924.93 KB, application/zip)
2014-04-02 22:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (968.75 KB, application/zip)
2014-04-02 23:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (964.32 KB, application/zip)
2014-04-03 00:11 PDT, Build Bot
no flags Details
Patch (81.95 KB, patch)
2014-04-03 09:55 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (711.91 KB, application/zip)
2014-04-03 11:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (764.04 KB, application/zip)
2014-04-03 12:09 PDT, Build Bot
no flags Details
Patch (86.08 KB, patch)
2014-04-03 13:48 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (87.03 KB, patch)
2014-04-03 14:22 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (639.23 KB, application/zip)
2014-04-03 15:37 PDT, Build Bot
no flags Details
Patch (88.68 KB, patch)
2014-04-04 07:52 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2014-04-02 20:46:13 PDT
Do not round background image size/position to integral.
Comment 1 zalan 2014-04-02 21:21:16 PDT
Created attachment 228461 [details]
Patch
Comment 2 Build Bot 2014-04-02 22:23:59 PDT
Comment on attachment 228461 [details]
Patch

Attachment 228461 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4982710040788992

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
fast/backgrounds/hidpi-bitmap-background-on-subpixel-position.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
css3/background/background-repeat-space-content.html
css3/masking/mask-repeat-space-content.html
css3/masking/mask-repeat-round-content.html
css3/background/background-repeat-round-content.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html
fast/backgrounds/hidpi-generated-gradient-background-on-subpixel-position.html
Comment 3 Build Bot 2014-04-02 22:24:02 PDT
Created attachment 228464 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 4 Build Bot 2014-04-02 23:18:09 PDT
Comment on attachment 228461 [details]
Patch

Attachment 228461 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4877718491496448

New failing tests:
css3/masking/mask-repeat-space-padding.html
css3/masking/mask-repeat-space-content.html
css3/background/background-repeat-space-padding.html
css3/background/background-repeat-space-content.html
css3/masking/mask-repeat-round-content.html
css3/background/background-repeat-round-content.html
fast/backgrounds/hidpi-generated-gradient-background-on-subpixel-position.html
Comment 5 Build Bot 2014-04-02 23:18:12 PDT
Created attachment 228469 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 6 Simon Fraser (smfr) 2014-04-02 23:49:40 PDT
Comment on attachment 228461 [details]
Patch

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

> Source/WebCore/platform/graphics/LayoutPoint.h:191
> +inline FloatSize pixelSnappedForPainting(const LayoutSize& s, const LayoutPoint& p, float pixelSnappingFactor)
> +{
> +    return FloatSize(snapSizeToDevicePixel(s.width(), p.x(), pixelSnappingFactor), snapSizeToDevicePixel(s.height(), p.y(), pixelSnappingFactor));
> +}

Is this any different from snapping a rect with location p and size s and reading the size?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1051
> +void RenderBoxModelObject::pixelSnappedBackgroundImageGeometryForPainting(BackgroundImageGeometry& geometry) const

I would call this pixelSnapBackgroundImageGeometryForPainting().

pixelSnappedBackgroundImageGeometryForPainting would be like:
BackgroundImageGeometry pixelSnappedBackgroundImageGeometryForPainting(const BackgroundImageGeometry&)

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1142
> +        int nrTiles = std::max(1, roundToInt(positioningAreaSize.width() / fillTileSize.width()));

Maybe nrTiles -> numTiles while you're here.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1154
> +        int nrTiles = std::max(1, roundToInt(positioningAreaSize.height() / fillTileSize.height()));

Ditto.

> LayoutTests/css3/background/background-repeat-round-border-expected.html:12
> -                background-size: 111px 85px;
> +                background-size: 111px 85.5px;

Hmm, should we have half pixels in non-Retina situations?
Comment 7 Build Bot 2014-04-03 00:10:58 PDT
Comment on attachment 228461 [details]
Patch

Attachment 228461 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4650390167486464

New failing tests:
css3/masking/mask-repeat-space-padding.html
css3/masking/mask-repeat-space-content.html
css3/background/background-repeat-space-padding.html
css3/background/background-repeat-space-content.html
css3/masking/mask-repeat-round-content.html
css3/background/background-repeat-round-content.html
fast/backgrounds/hidpi-generated-gradient-background-on-subpixel-position.html
Comment 8 Build Bot 2014-04-03 00:11:02 PDT
Created attachment 228477 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 zalan 2014-04-03 09:55:41 PDT
Created attachment 228513 [details]
Patch
Comment 10 zalan 2014-04-03 09:55:58 PDT
Comment on attachment 228513 [details]
Patch

EWS
Comment 11 zalan 2014-04-03 10:00:53 PDT
(In reply to comment #6)
> (From update of attachment 228461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228461&action=review
> 
> > Source/WebCore/platform/graphics/LayoutPoint.h:191
> > +inline FloatSize pixelSnappedForPainting(const LayoutSize& s, const LayoutPoint& p, float pixelSnappingFactor)
> > +{
> > +    return FloatSize(snapSizeToDevicePixel(s.width(), p.x(), pixelSnappingFactor), snapSizeToDevicePixel(s.height(), p.y(), pixelSnappingFactor));
> > +}
> 
> Is this any different from snapping a rect with location p and size s and reading the size?
No, there should not be any difference. I added this function to mirror the int snap version, but I think you are right, there's no need to introduce this extra helper to achieve the snapping for sizes. -removed.
 
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1051
> > +void RenderBoxModelObject::pixelSnappedBackgroundImageGeometryForPainting(BackgroundImageGeometry& geometry) const
> 
> I would call this pixelSnapBackgroundImageGeometryForPainting().
Done.

> 
> pixelSnappedBackgroundImageGeometryForPainting would be like:
> BackgroundImageGeometry pixelSnappedBackgroundImageGeometryForPainting(const BackgroundImageGeometry&)
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1142
> > +        int nrTiles = std::max(1, roundToInt(positioningAreaSize.width() / fillTileSize.width()));
> 
> Maybe nrTiles -> numTiles while you're here.
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1154
> > +        int nrTiles = std::max(1, roundToInt(positioningAreaSize.height() / fillTileSize.height()));
> 
> Ditto.
> 
Done.

> > LayoutTests/css3/background/background-repeat-round-border-expected.html:12
> > -                background-size: 111px 85px;
> > +                background-size: 111px 85.5px;
> 
> Hmm, should we have half pixels in non-Retina situations?
Yea, that got me thinking too. I looked into the failing cases and noticed their values are very fragile. I changed them to be stable.
Comment 12 Build Bot 2014-04-03 11:02:03 PDT
Comment on attachment 228513 [details]
Patch

Attachment 228513 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6755172176887808

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
fast/backgrounds/hidpi-bitmap-background-on-subpixel-position.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
css3/background/background-repeat-space-content.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html
fast/backgrounds/hidpi-generated-gradient-background-on-subpixel-position.html
Comment 13 Build Bot 2014-04-03 11:02:08 PDT
Created attachment 228522 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 14 Build Bot 2014-04-03 12:09:06 PDT
Comment on attachment 228513 [details]
Patch

Attachment 228513 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6562182384844800

New failing tests:
css3/background/background-repeat-space-content.html
media/adopt-node-crash.html
fast/backgrounds/hidpi-bitmap-background-on-subpixel-position.html
fast/backgrounds/hidpi-generated-gradient-background-on-subpixel-position.html
Comment 15 Build Bot 2014-04-03 12:09:10 PDT
Created attachment 228528 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 16 zalan 2014-04-03 13:48:53 PDT
Created attachment 228538 [details]
Patch
Comment 17 zalan 2014-04-03 13:49:07 PDT
Comment on attachment 228538 [details]
Patch

EWS
Comment 18 zalan 2014-04-03 14:22:35 PDT
Created attachment 228543 [details]
Patch
Comment 19 zalan 2014-04-03 14:22:50 PDT
Comment on attachment 228543 [details]
Patch

EWS
Comment 20 Build Bot 2014-04-03 15:37:28 PDT
Comment on attachment 228543 [details]
Patch

Attachment 228543 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6721347161948160

New failing tests:
media/adopt-node-crash.html
Comment 21 Build Bot 2014-04-03 15:37:33 PDT
Created attachment 228550 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 22 WebKit Commit Bot 2014-04-04 07:41:01 PDT
Comment on attachment 228543 [details]
Patch

Rejecting attachment 228543 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 228543, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ap-background-repeat-on-subpixel-position-expected.html
patching file LayoutTests/fast/backgrounds/hidpi-bitmap-background-repeat-on-subpixel-position.html
patching file LayoutTests/fast/backgrounds/hidpi-generated-gradient-background-on-subpixel-position-expected.html
patching file LayoutTests/fast/backgrounds/hidpi-generated-gradient-background-on-subpixel-position.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/6747390769889280
Comment 23 zalan 2014-04-04 07:52:56 PDT
Created attachment 228591 [details]
Patch
Comment 24 zalan 2014-04-04 07:53:27 PDT
Comment on attachment 228591 [details]
Patch

EWS
Comment 25 WebKit Commit Bot 2014-04-04 09:30:45 PDT
Comment on attachment 228591 [details]
Patch

Clearing flags on attachment: 228591

Committed r166784: <http://trac.webkit.org/changeset/166784>
Comment 26 WebKit Commit Bot 2014-04-04 09:30:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 mitz 2014-04-20 22:06:23 PDT
(In reply to comment #25)
> (From update of attachment 228591 [details])
> Clearing flags on attachment: 228591
> 
> Committed r166784: <http://trac.webkit.org/changeset/166784>

This caused bug 131924.