New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 684112 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Regression

Blocked on:
issue skia:6158



Sign in to add a comment

rx attribute for svg rect breaks rendering when visualizing large numbers of rectangles

Reported by o...@signalfx.com, Jan 23 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Steps to reproduce the problem:
1. Open reduced test case.
2. Both SVGs do not render same number of boxes, even though they should. Only difference is the red boxes do not have an "rx" attribute.

What is the expected behavior?
Both SVGs should render the same number of boxes.

What went wrong?
The SVG with blue boxes experiences a rendering error manifesting in  white areas where the rect elements aren't rendered. These areas of white seem to vary with viewport size. We've been able to reproduce it whenever viewport size is larger than the svg width (700px).

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 55.0.2883.95  Channel: stable
OS Version: OS X 10.12.0
Flash Version: Shockwave Flash 24.0 r0

Reproduced on a few different machines:
Chrome 55.0.2883.95 on OS X 10.11.6 (El Capitan)
Chrome 55.0.2883.95 on OS X 10.12 (Sierra)
Chrome 55.0.2883.95 on OS X 10.12.2 (Sierra)

JSFiddle of test case: https://jsfiddle.net/r0qbteuv/3/
 
rx-in-rect-breaks-rendering.html
1.5 KB View Download
rx_in_rect_breaks_rendering.png
36.6 KB View Download

Comment 1 by pdr@chromium.org, Jan 23 2017

Labels: Needs-Bisect
Thanks for taking the time to file this. I wonder if this is a dupe of  https://crbug.com/675499 ?

Comment 2 by o...@signalfx.com, Jan 24 2017

pdr: Not sure, but for what it's worth, I'm not able to repro  https://crbug.com/675499  on Chrome 55.0.2883.95 on OS X 10.12.
Labels: Needs-Triage-M55
Cc: ranjitkan@chromium.org
Labels: -Type-Bug -Needs-Bisect -Needs-Triage-M55 has-Bisect M-58 Type-Bug-Regression
Owner: fmalita@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on 55.0.2883.95 on MAC 12.2. Below are the bisect details for the same:

Bisect Info:
============
55.0.2860.0 - Good Build
55.0.2864.0 - Bad build

Change Log:  https://chromium.googlesource.com/chromium/src/+log/db00688793f63e1f3ccd71f883ec3129bd169abe..846a59b93f9911517c7aeb05b47af9b4c0cad175

Suspecting the below change could be a possible culprit:

https://chromium.googlesource.com/skia.git/+/ceb93abddc81ccc0b4f93d958135632ed4cebfd4

@ fmalita: Assigning to you, kindly have a look into it. Please help us to find an owner if not with respect to your change.

P.S: Issue is not observed on Windows and Linux OS. Also hasbisect per revision bisect script invoked builds where chrome was showing network error beteween build 55.0.2862.0 and 55.0.2863.0
Cc: fmalita@chromium.org
Components: -Blink>SVG Internals>Skia Internals>GPU>Rasterization
Labels: -OS-Mac OS-All
Owner: jvanverth@chromium.org
The suspected CL in c#4 doesn't touch code used in Chromium.  Looks like a bad bisect.  I've retried, and got

You are probably looking for a change made after 415138 (known good), but no later than 415162 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/712fc6049b32036a00ee1684bc1db726c969db59..72b7c4e88a9543a11a649fadc2ac5c2eed0e15f9

(note: the repro requires GPU rasterization)

Skia roll in that range: https://chromium.googlesource.com/skia.git/+log/839d3ad854c0..6f5df6acb7de

Most likely culprit: https://chromium.googlesource.com/skia.git/+/84839f6fb3d0de9088be3d53e81df195331ed1c9
Blockedon: skia:6158
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/8cefe40ab094bfbea532761dad1a857eb3d4b831

commit 8cefe40ab094bfbea532761dad1a857eb3d4b831
Author: Jim Van Verth <jvanverth@google.com>
Date: Thu Feb 09 17:29:31 2017

Don't batch circles and circular rrects beyond index limit

BUG= skia:6158 ,  chromium:690144 , chromium:688582,  chromium:684112 

Change-Id: I7a6d1fb73cbe6cb4328848acd153ff2505b5fea2
Reviewed-on: https://skia-review.googlesource.com/8256
Reviewed-by: Florin Malita <fmalita@chromium.org>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>

[modify] https://crrev.com/8cefe40ab094bfbea532761dad1a857eb3d4b831/src/gpu/ops/GrOvalOpFactory.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7273c6c6210628e39f02afa086ed025413cf8611

commit 7273c6c6210628e39f02afa086ed025413cf8611
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Thu Feb 09 23:24:58 2017

Roll src/third_party/skia/ 1df161ab8..2c49a4185 (15 commits).

https://skia.googlesource.com/skia.git/+log/1df161ab8a6a..2c49a4185865

$ git log 1df161ab8..2c49a4185 --date=short --no-merges --format='%ad %ae %s'
2017-02-09 bsalomon Remove inner/outer threshold restriction on SkAlphaThresholdFilter.
2017-02-09 kjlubick Prevent waiting for NexusPlayers indefinitely
2017-02-09 fmalita [4fGradient] Relax interval checks for SkGradientShaderBase also
2017-02-09 mtklein exclude _none.cpp in G3 build
2017-02-09 ethannicholas re-land of skslc type constructor cleanups
2017-02-09 bsalomon Temporarily don't mark alpha threshold fp as modulating
2017-02-09 mtklein Don't build sksl if we're not building Ganesh.
2017-02-09 msarett Pixel conversion refactors: use raster pipeline for 565 and gray
2017-02-09 mtklein Make src/effects explicitly optional.
2017-02-09 ethannicholas added support for sk_ClipDistance
2017-02-09 bsalomon Re-enable processor optimization test with some fixes.
2017-02-09 fmalita [4fGradient] Relax interval checks
2017-02-09 herb Remove last use of ktx.h
2017-02-09 robertphillips Fix simple-magnification GM in "--preAbandonGpuContext" mode
2017-02-09 jvanverth Don't batch circles and circular rrects beyond index limit

Created with:
  roll-dep src/third_party/skia
BUG= 690144 ,688582, 684112 

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=robertphillips@google.com

Review-Url: https://codereview.chromium.org/2680673006
Cr-Commit-Position: refs/heads/master@{#449461}

[modify] https://crrev.com/7273c6c6210628e39f02afa086ed025413cf8611/DEPS

Status: Fixed (was: Assigned)
Looks to be working in current Chrome Canary
Labels: TE-Verified-M58 TE-Verified-58.0.3012.0
Verified the issue on Mac Retina 10.12.2 using Chrome version 58.0.3012.0 as per the comment #0.

Observed that the fix is working as expected.

Attaching the screen cast for reference.

Hence, adding the verified labels.

Note: Issue is specific to Mac Retina.
Issue is not observed on Windows-7 and Linux Ubuntu-14.04 and MacBookAir-10.12.2.

Thanks.

684112.mp4
2.2 MB View Download
Labels: Merge-Request-57
Project Member

Comment 12 by sheriffbot@chromium.org, Feb 21 2017

Labels: -Merge-Request-57 Hotlist-Merge-Review Merge-Review-57
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This bug exists since M55 and marked as P2. Can this wait until M58 as we're getting closer to M57 Stable promotion?
Sure, no great hurry.
Actually, this is related to chromium:688582, which is marked Priority 1.
Thank you  jvanverth@. Bug 688582 marked as P1 but also exists on current M56 Stable. 
 Could you please confirm this change will be a safe merge to M57?

There are reports that it is causing a perf slowdown, so no, at this point I can't say that it's a safe merge.
Labels: -Merge-Review-57 Merge-Rejected-57
Rejecting merge to M57 based on comment #17. Please let me know if there is any concern here. Thank you.
If some change completely broke rendering and the fix makes it so that we render correctly, why isn't the thing reverted completely and the revert merged?

As-is, we'll end up in the slower-but-correct rendering mode in m58, and we'll have busted rendering in m57.
I think the fix should be merged. The root cause of the performance regression is yet to be determined but we know the fix for this bug is not the cause.
Labels: -Merge-Rejected-57 Merge-Request-57
Flipping back to requested then. Please seem comment 19 and 20 -- we're fixing a very visible drawing regression, and the perf regression is by now verified to be caused by something else.
Project Member

Comment 22 by sheriffbot@chromium.org, Mar 7 2017

Labels: -Merge-Request-57 Merge-Review-57
This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This bug exists on M55 and M56 and is P2. Plan is to cut M57 Stable RC today for Desktop. Will this be a safe merge to M57 (no functional, perf or stability regression)?
Yes, this is a safe change.
Labels: -Merge-Review-57 Merge-Approved-57
Approving merge to M57 branch 2987 based on comment #21 and #24. Please merge ASAP. Thank you.
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 7 2017

Labels: merge-merged-m57
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/ae9cc5d3588d52f4b371b55845704b25d88cf06d

commit ae9cc5d3588d52f4b371b55845704b25d88cf06d
Author: Brian Salomon <bsalomon@google.com>
Date: Tue Mar 07 16:20:54 2017

Don't batch circles and circular rrects beyond index limit

M57 Cherry pick

BUG= skia:6158 ,  chromium:690144 , chromium:688582,  chromium:684112 

Change-Id: I7a6d1fb73cbe6cb4328848acd153ff2505b5fea2
Reviewed-on: https://skia-review.googlesource.com/8256
Reviewed-by: Florin Malita <fmalita@chromium.org>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Reviewed-on: https://skia-review.googlesource.com/9383
Reviewed-by: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/ae9cc5d3588d52f4b371b55845704b25d88cf06d/src/gpu/ops/GrOvalOpFactory.cpp

Labels: -Merge-Approved-57
This is already merged to M57 per comment #26. So removing "Merge-Approved-57" label.

Sign in to add a comment