Bug 115304

Summary: REGRESSION(SUBPIXEL_LAYOUT) Composited layers can cause one pixel shifts
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: Layout and RenderingAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, ddkilzer, eae, efidler, esprehn+autocc, glenn, hyatt, jonlee, leviw, rniwa, simon.fraser, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 94792, 101076, 113199    
Attachments:
Description Flags
test.html
none
test.html
none
Patch
none
subpixel-stutter.html
none
Patch
none
Patch
none
Patch
none
Patch hyatt: review+

Allan Sandfeld Jensen
Reported 2013-04-27 05:39:16 PDT
The issues was observed on hulu.com, the reduced test case is one div on a subpixel position with a an image below it also on a subpixel position. If the outer div is caused to become composited the image below it may change its pixel rounding, which causes it to shift. Of the issues one pixel offset observed by Mac, this is the only one reliably observed on Qt as well. Imagine element A a placed at (0, 10.6) with an image B inside placed relatively to A at (0, 4.8). The image B then has an absolute position of (0, 15.4) which causes it to rendered at (0,15). If A becomes accelerated composited, then A will be rendered at (0,5) on the composited layer, and the composited layer will be rendered at (0,11), causing image B to end up at (0,16). To avoid this issue, the position of accelerated layers must be floored to an integer position, and be given an internal subpixel padding of the remainder, making their content get the at the same fractional coordinates as if there were no compositing.
Attachments
test.html (1.47 KB, text/html)
2013-04-27 06:19 PDT, Allan Sandfeld Jensen 		no flags
Details
test.html (1.85 KB, text/html)
2013-04-27 06:54 PDT, Allan Sandfeld Jensen 		no flags
Details
Patch (25.73 KB, patch)
2013-04-30 06:11 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
subpixel-stutter.html (2.16 KB, text/html)
2013-04-30 06:16 PDT, Allan Sandfeld Jensen 		no flags
Details
Patch (34.95 KB, patch)
2013-05-02 05:24 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (35.07 KB, patch)
2013-05-02 06:29 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (35.74 KB, patch)
2013-05-07 03:41 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (33.07 KB, patch)
2013-06-07 04:57 PDT, Allan Sandfeld Jensen 		hyatt: review+
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Allan Sandfeld Jensen
Comment 1 2013-04-27 06:19:24 PDT
Created attachment 199903 [details] test.html Test case demonstrating rounding of fractional positions being affected by the presence or absence of compositing.
Allan Sandfeld Jensen
Comment 2 2013-04-27 06:54:07 PDT
Created attachment 199904 [details] test.html Updated test.
Simon Fraser (smfr)
Comment 3 2013-04-29 14:18:28 PDT
var test = document.getElementById('test'); var clas = test.getAttribute('class'); if (clas == 'composite') { test.setAttribute('class', ''); } else { test.setAttribute('class', 'composite'); } This could be document.getElementById('test').classList.toggle('composite').
Allan Sandfeld Jensen
Comment 4 2013-04-30 06:11:10 PDT
Created attachment 200103 [details] Patch Preliminary patch. Fixes all shifts are resizes in the uploaded case, except for ones ending in exactly 0.5. I am guessing a rounding somewhere gets confused by negative vs positive rounding, but I haven't found it yet.
Allan Sandfeld Jensen
Comment 5 2013-04-30 06:16:43 PDT
Created attachment 200105 [details] subpixel-stutter.html Updated test.
alan
Comment 6 2013-04-30 09:57:53 PDT
I've seen other places where double-snapping cause off-by-one issues (I try to dig them out). It'd be nice to see some common pattern to fix them, instead of different type of local fixes (if possible at all). This might be a good topic for the WebKit contr meeting.
Allan Sandfeld Jensen
Comment 7 2013-05-02 05:24:56 PDT
Created attachment 200309 [details] Patch
Build Bot
Comment 8 2013-05-02 05:59:28 PDT
Comment on attachment 200309 [details] Patch Attachment 200309 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/308160
Build Bot
Comment 9 2013-05-02 06:17:24 PDT
Comment on attachment 200309 [details] Patch Attachment 200309 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/343112
Allan Sandfeld Jensen
Comment 10 2013-05-02 06:29:06 PDT
Created attachment 200311 [details] Patch Build fix for pendantic clang
Build Bot
Comment 11 2013-05-02 07:07:28 PDT
Comment on attachment 200311 [details] Patch Attachment 200311 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/290143
Allan Sandfeld Jensen
Comment 12 2013-05-02 07:24:07 PDT
(In reply to comment #11) > (From update of attachment 200311 [details]) > Attachment 200311 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/290143 Also needs updated exported symbols it appears.
Build Bot
Comment 13 2013-05-02 07:28:05 PDT
Comment on attachment 200311 [details] Patch Attachment 200311 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/391133
Allan Sandfeld Jensen
Comment 14 2013-05-07 03:41:56 PDT
Created attachment 200888 [details] Patch Export missing symbol for mac
Levi Weintraub
Comment 15 2013-05-07 10:39:49 PDT
Comment on attachment 200888 [details] Patch This LGTM, but I'll let smfr comment.
Allan Sandfeld Jensen
Comment 16 2013-05-24 08:43:25 PDT
(In reply to comment #15) > (From update of attachment 200888 [details]) > This LGTM, but I'll let smfr comment. Simon, any comments?
Simon Fraser (smfr)
Comment 17 2013-06-06 11:01:21 PDT
Comment on attachment 200888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200888&action=review I think the spirit of the patch is OK, but not the implementation. > Source/WebCore/platform/graphics/GraphicsLayer.h:252 > IntSize offsetFromRenderer() const { return m_offsetFromRenderer; } > void setOffsetFromRenderer(const IntSize&, ShouldSetNeedsDisplay = SetNeedsDisplay); > > + LayoutSize subpixelAccumulation() const { return m_subpixelAccumulation; } > + void setSubpixelAccumulation(const LayoutSize& subpixelAccumulation) { m_subpixelAccumulation = subpixelAccumulation; } Unclear why we would need both offsetFromRenderer and subpixelAccumulation. Why is it ok for offsetFromRenderer to be IntSize? > Source/WebCore/platform/graphics/GraphicsLayer.h:502 > + // Accumulated subpixel error to be used by subpixel layout of the content. > + LayoutSize m_subpixelAccumulation; I don't think GraphicsLayer should every see Layout* > Source/WebCore/rendering/RenderLayerBacking.cpp:656 > + // We must move the bounds by the subpixel part of delta, > + // so that they can be pixel snapped to actual pixels. That comment doesn't make much sense. > Source/WebCore/rendering/RenderLayerBacking.cpp:710 > + m_graphicsLayer->setPosition(FloatPoint(relativePixelCompositingBounds.location() - graphicsLayerParentLocation)); > + m_graphicsLayer->setOffsetFromRenderer(toIntSize(localPixelCompositingBounds.location())); > + m_graphicsLayer->setSubpixelAccumulation(toLayoutSize(delta.fraction())); What about m_foregroundLayer, m_maskLayer and m_backgroundLayer? Why not store subpixel accumulation here on the RLB?
Allan Sandfeld Jensen
Comment 18 2013-06-07 03:35:44 PDT
(In reply to comment #17) > (From update of attachment 200888 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200888&action=review > > I think the spirit of the patch is OK, but not the implementation. > > > Source/WebCore/platform/graphics/GraphicsLayer.h:252 > > IntSize offsetFromRenderer() const { return m_offsetFromRenderer; } > > void setOffsetFromRenderer(const IntSize&, ShouldSetNeedsDisplay = SetNeedsDisplay); > > > > + LayoutSize subpixelAccumulation() const { return m_subpixelAccumulation; } > > + void setSubpixelAccumulation(const LayoutSize& subpixelAccumulation) { m_subpixelAccumulation = subpixelAccumulation; } > > Unclear why we would need both offsetFromRenderer and subpixelAccumulation. Why is it ok for offsetFromRenderer to be IntSize? > Making offsetFromRenderer a LayoutSize would not help since it is the relative offset, where subpixelAccumulation is the fractional part of the absolute offset. SubpixelAccumulation is what you add to the relative coordinates when painting to ensure snap-to-pixel snaps as it would if they were absolute coordinates. > > Source/WebCore/rendering/RenderLayerBacking.cpp:710 > > + m_graphicsLayer->setPosition(FloatPoint(relativePixelCompositingBounds.location() - graphicsLayerParentLocation)); > > + m_graphicsLayer->setOffsetFromRenderer(toIntSize(localPixelCompositingBounds.location())); > > + m_graphicsLayer->setSubpixelAccumulation(toLayoutSize(delta.fraction())); > > What about m_foregroundLayer, m_maskLayer and m_backgroundLayer? > Yes, I overlooked them. > Why not store subpixel accumulation here on the RLB? That might work, I will give it a try.
Allan Sandfeld Jensen
Comment 19 2013-06-07 04:57:13 PDT
Created attachment 204023 [details] Patch Subpixel accumulation moved to RenderLayerBacking as suggested by smfr. Improved variable naming and comments.
Dave Hyatt
Comment 20 2013-08-13 08:49:05 PDT
Comment on attachment 204023 [details] Patch r=me This looks great.
Allan Sandfeld Jensen
Comment 21 2013-08-13 10:07:16 PDT
Committed r154009: <http://trac.webkit.org/changeset/154009>
alan
Comment 22 2013-12-09 11:06:00 PST
*** Bug 112229 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.