Closed
Bug 1292856
Opened 8 years ago
Closed 8 years ago
Broken redrawing of javascript svg transform matrix
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: zdnexnet, Assigned: jnicol)
References
()
Details
(Keywords: regression, Whiteboard: [gfx-noted])
Attachments
(5 files)
(deleted),
application/x-zip-compressed
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
video/x-ms-wmv
|
Details | |
(deleted),
text/x-review-board-request
|
mstange
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057
Steps to reproduce:
Open page with root SVG with multiple SVG objects which are controled via transform matrix then just try to zoom and move with pan around objects.
Actual results:
On Firefox 48 - 50 after some point of zooming out svg object is completely broken and most of time is viewbox is rendered wrong or is completely missing.
I am attaching also video from Firefox 50 and from 47 to see the difference. Also I have recorded performance of JS so you can try to see difference in utilization. When image gets broken suddenly Firefox starts to do more calculations of stylesheets and rendering. It looks to me that after certain point it makes Recordings are from current Firefox Dev 50
I have tested same wrong behavior on Windows 10/ Ubuntu 12.04 and Ubuntu 14.04 and several different computers. It really seems that there was some change between 47 and 48 code to handle transform.
Expected results:
It should stay normal, render objects as it was rendering them before without problem and make pan with visible content without flickering. Maybe this bug is connected with https://bugzilla.mozilla.org/show_bug.cgi?id=1287492
On Firefox 45->47 it works normally. Also current Chrome/Opera.
Comment 1•8 years ago
|
||
Could obtain a regression range via moz-regression(http://mozilla.github.io/mozregression/)?
Flags: needinfo?(zdnexnet)
Hello Robert,
sorry, I am not skilled enough in Firefox dev model. I can try it in another prepared binaries, if this is needed and obtain videos/profiling or provide account (record steps to see it).
Flags: needinfo?(zdnexnet)
Oh, sorry, now I see that it works with prepared builds. So I guess I can try it.
Ok, I am back with data. I am attaching log and also screenshot.
2016-08-07T07:03:54: INFO : Narrowed inbound regression window from [867a9bdc, d935da74] (4 revisions) to [50792e13, d935da74] (2 revisions) (~1 steps left)
2016-08-07T07:03:54: DEBUG : Starting merge handling...
2016-08-07T07:03:54: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=d935da74d415dcd468f706fd54c5fb52f8122a09&full=1
2016-08-07T07:03:55: DEBUG : Found commit message:
Bug 1274991 - Consider ancestor scale transforms when deciding whether to prerender transformed content. r=mstange
MozReview-Commit-ID: AnmSSy9YviP
2016-08-07T07:03:55: INFO : The bisection is done.
2016-08-07T07:03:55: INFO : Stopped
Updated•8 years ago
|
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•8 years ago
|
Has Regression Range: --- → yes
Keywords: regression
Comment 7•8 years ago
|
||
I am confused about the steps to reproduce here. There's no testcase in the zip and th URL just leads to a login page. Can you add a testcase as an attachment to the bug please?
Flags: needinfo?(zdnexnet)
Hello,
sorry with that > click on https://pripravto.cz
username: useroc2
password: oc2user2piff
click on Planview in top left area.
I have attached video with login/click and then the problem.
Flags: needinfo?(zdnexnet)
Updated•8 years ago
|
Flags: needinfo?(jnicol)
Whiteboard: [gfx-noted]
Assignee | ||
Comment 10•8 years ago
|
||
Okay here's what's going on: The page has a gigantic transform item which we are trying to prerender. Because it is so large the surface allocation fails and nothing is displayed.
nsDisplayTransform::ShouldPrerenderTransformedContent() should be returning false, and indeed it used to. Then bug 1274991 made it so that function takes into account ancestor scale transforms. Which is the correct thing to do, but even so this is still far too big to prerender. The problem is that when scale < 1 it is incorrectly calculating the scaled size as zero, so decides to prerender.
It seems like when performing the nsSize (integer) * gfxSize (float), the floats get converted to ints before performing the multiplication.
Flags: needinfo?(jnicol)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jnicol
Assignee | ||
Comment 11•8 years ago
|
||
> It seems like when performing the nsSize (integer) * gfxSize (float), the floats get converted to ints before performing the multiplication.
Ugh I was being really stupid. nsSize * gfxSize doesn't exist. I _explicitly_ converted the gfxSize to an nsSize pre-multiplication, which obviously leads to this. We want to do that post-multiplication.
Reporter | ||
Comment 12•8 years ago
|
||
Hello,
so what it means, can I somehow help with this? Is there a way how this was supposed to work before it was working wrong? If I understand correctly > this got fixed, but fix brought some problem when scale is smaller, right?
Assignee | ||
Comment 13•8 years ago
|
||
Yes that's correct, the fix for a related problem caused problems when scale < 1. The fix is really trivial so there's nothing we need you to do :). Thanks for the report, and for the bisection that really helped find the problem!
Comment hidden (mozreview-request) |
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8779343 [details]
Bug 1292856 - Don't cast scale to int in nsDisplayTransform::ShouldPrerenderTransformedContent;
https://reviewboard.mozilla.org/r/70334/#review67670
Ugh. Sorry for not catching that in my review.
Attachment #8779343 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 16•8 years ago
|
||
Keywords: checkin-needed
Comment 17•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8a2296ba77
Don't cast scale to int in nsDisplayTransform::ShouldPrerenderTransformedContent; r=mstange
Keywords: checkin-needed
Comment 18•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Reporter | ||
Comment 19•8 years ago
|
||
will this be fixed in upcoming version 49 released in one month? or we will have to wait to 51 version? As we are not sure if right now we should use ESR version instead or wait for fix?
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8779343 [details]
Bug 1292856 - Don't cast scale to int in nsDisplayTransform::ShouldPrerenderTransformedContent;
Approval Request Comment
[Feature/regressing bug #]: Bug 1292856
[User impact if declined]: Some pages won't render correctly or will use a lot of memory (because we attempt to allocate massive layers)
[Describe test coverage new/current, TreeHerder]: Patch is on nightly.
[Risks and why]: Very small. Fixes simple error and makes us more likely to use the safer code path
[String/UUID change made/needed]: N/A
Attachment #8779343 -
Flags: approval-mozilla-beta?
Attachment #8779343 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•8 years ago
|
||
(In reply to zdnex from comment #19)
> will this be fixed in upcoming version 49 released in one month? or we will
> have to wait to 51 version? As we are not sure if right now we should use
> ESR version instead or wait for fix?
I just requested we include this fix in 49 and 50. The risks will need to be considered and a decision will be made here soon.
Comment on attachment 8779343 [details]
Bug 1292856 - Don't cast scale to int in nsDisplayTransform::ShouldPrerenderTransformedContent;
Fix for recent regression, this should take care of the layer transform issue!
Attachment #8779343 -
Flags: approval-mozilla-beta?
Attachment #8779343 -
Flags: approval-mozilla-beta+
Attachment #8779343 -
Flags: approval-mozilla-aurora?
Attachment #8779343 -
Flags: approval-mozilla-aurora+
zdnex, the fix should make it to 49 beta 4, which will be available by the middle of next week. 49 release date is Sept. 13th.
Comment 24•8 years ago
|
||
bugherder uplift |
status-firefox50:
--- → fixed
Comment 25•8 years ago
|
||
bugherder uplift |
status-firefox49:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•