Closed Bug 1292856 Opened 8 years ago Closed 8 years ago

Broken redrawing of javascript svg transform matrix

Categories

(Core :: Graphics: Layers, defect)

48 Branch
Unspecified
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: zdnexnet, Assigned: jnicol)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(5 files)

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.
Severity: normal → major
OS: Unspecified → All
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
Attached image screenshot from mozregression (deleted) —
Attached file log from mozregresion (deleted) —
Blocks: 1274991
Component: Untriaged → Graphics: Layers
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Has Regression Range: --- → yes
Keywords: regression
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)
Attached video test_case_ff50.wmv (deleted) —
Flags: needinfo?(jnicol)
Whiteboard: [gfx-noted]
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: nobody → jnicol
> 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.
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?
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 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+
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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?
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?
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: