Closed Bug 1067286 Opened 5 years ago Closed 5 years ago

CSS axis of rotation of rotate3d seems to be shifted, and image is wrongly clipped

Categories

(Core :: Layout, defect)

34 Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox33 --- unaffected
firefox34 + wontfix
firefox35 + wontfix
firefox36 + fixed
firefox37 --- fixed

People

(Reporter: alice0775, Assigned: kip)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

[Tracking Requested - why for this release]: regression

CSS  axis of rotation of rotate3d seems to be shifted, so, Image is wrongly clipped  during rotation.

This bug happens on Ubuntu ONLY. Not happen on Windows.

*NOTE*
Regarding wrong angle of the rotation axis was another bug and it was fixed by Bug 781701.

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/326c91338df0
Mozilla/5.0 (X11; Linux i686; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140722220945
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06b540d3667a
Mozilla/5.0 (X11; Linux i686; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140722222448
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=326c91338df0&tochange=06b540d3667a

Regressed by:
Bug 1042423 - Do css background clipping using DisplayItemClip


Graphics
--------

Adapter Description: VMware, Inc. -- Gallium 0.4 on SVGA3D; build: RELEASE;
Device ID: Gallium 0.4 on SVGA3D; build: RELEASE;
Driver Version: 2.1 Mesa 8.0.4
GPU Accelerated Windows: 0/2 Basic
Vendor ID: VMware, Inc.
WebGL Renderer: VMware, Inc. -- Gallium 0.4 on SVGA3D; build: RELEASE;
windowLayerManagerRemote: false
AzureCanvasBackend: cairo
AzureContentBackend: cairo
AzureFallbackCanvasBackend: none
AzureSkiaAccelerated: 0
Steps To Reproduce:
1. Open attachment 650741 [details] of Bug 781701
2. Mouse the image and mouse out 

Actual Results:
Image is wrongly clipped  during rotation.
Screen capture: http://youtu.be/L_7H0upuZ2k

Expected Results:
The image should be 
White Circle - white Ellipse - nothing - Black Ellipse - Black Circle.
Summary: CSS axis of rotation of rotate3d seems to be shifted → CSS axis of rotation of rotate3d seems to be shifted, and image is wrongly clipped
See Also: → 781701
Summary: CSS axis of rotation of rotate3d seems to be shifted, and image is wrongly clipped → CSS axis of rotation of rotate3d seems to be shifted, and image is wrongly clipped
Jet - Do you have someone who can take a look at this?
Flags: needinfo?(bugs)
(In reply to Lawrence Mandel [:lmandel] from comment #2)
> Jet - Do you have someone who can take a look at this?

Yes.
Flags: needinfo?(bugs) → needinfo?(kgilbert)
Assigning to kip based on comment 3.
Assignee: nobody → kgilbert
Have reproduced, am investigating now.
Flags: needinfo?(kgilbert)
This appears to be sensitive to the backend selection.  Occurs with cairo, but not skia.
Also, the issue is introduced in the Part 4 patch of Bug 1042423.
Attachment #8517798 - Attachment is obsolete: true
Kip - We're at the end of the 34 beta cycle. Any update on this bug?
Flags: needinfo?(kgilbert)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #9)
> Kip - We're at the end of the 34 beta cycle. Any update on this bug?
I was hopeful that the cause of this bug was the same as Bug 1086723; however, my fix for 1086723 has not corrected this bug.

I am returning to this bug now and expect to have a fix shortly.
Flags: needinfo?(kgilbert)
FYI, we go to build with our final beta on Thursday. I will consider a low risk fix if you have one tomorrow. Otherwise, we'll likely ship with this regression in Firefox 34.
This is now wontfix for 34. We can consider a fix in 35.
Updating FillRectWithMask() in BasicLayersImpl.cpp such that it doesn't set and restore the transform results in correct rendering for this case. (Commenting out the two SetTransform lines)

The mEffectiveTransform value for the mask layer appears to be calculated incorrectly, although the transformed clipping rectangle appears correct.

I have tried setting an identity matrix or copying the Color Layer's mEffectiveTransform to the mask layer without seeing the correct result.  I suspect that either ComputeEffectiveTransformFormMaskLayer is not being called in this particular case or the incorrect transform is being passed into it.

Also, I notice that the back-side rounded rect is rendered correctly when it flips -- perhaps the issue relates to preserve-3d and CSS transforms on the mask layer.

Matt: Would you be able to spare some time to review my findings and perhaps give me some background on this area of the code?
Flags: needinfo?(matt.woodrow)
Also, enabling E10S will correct the issue.  Need to disable E10S to reproduce.
Call stack to draw call that renders the masked transformed layer:

#0  mozilla::layers::FillRectWithMask (aDT=0x7fcd66f02800, aRect=..., aColor=..., aOptions=..., aMaskSource=0x7fcd695cec80, 
    aMaskTransform=0x7ffff5dbc820) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayersImpl.cpp:74
#1  0x00007fcd899d633f in mozilla::layers::FillRectWithMask (aDT=0x7fcd66f02800, aDeviceOffset=..., aRect=..., aColor=..., aOptions=..., 
    aMaskLayer=0x7fcd6abc1c00) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayersImpl.cpp:93
#2  0x00007fcd899c1b20 in mozilla::layers::BasicColorLayer::Paint (this=0x7fcd6abc0000, aDT=0x7fcd66f02800, aDeviceOffset=..., 
    aMaskLayer=0x7fcd6abc1c00) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicColorLayer.cpp:65
#3  0x00007fcd899d4f22 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=0x7fcd698dc700, aPaintContext=..., 
    aGroupTarget=0x7fcd65ba8ce0) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:727
#4  0x00007fcd899d554e in mozilla::layers::BasicLayerManager::PaintLayer (this=0x7fcd698dc700, aTarget=0x7fcd65ba8ce0, aLayer=0x7fcd6abc0000, 
    aCallback=0x0, aCallbackData=0x0) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:848
#5  0x00007fcd899d4fba in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=0x7fcd698dc700, aPaintContext=..., 
    aGroupTarget=0x7fcd65ba8ce0) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:736
#6  0x00007fcd899d554e in mozilla::layers::BasicLayerManager::PaintLayer (this=0x7fcd698dc700, aTarget=0x7fcd65ba8ce0, aLayer=0x7fcd6abbf800, 
    aCallback=0x0, aCallbackData=0x0) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:848
#7  0x00007fcd899d4fba in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=0x7fcd698dc700, aPaintContext=..., 
    aGroupTarget=0x7fcd65ba8ce0) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:736
#8  0x00007fcd899d54eb in mozilla::layers::BasicLayerManager::PaintLayer (this=0x7fcd698dc700, aTarget=0x7fcd65ba8ce0, aLayer=0x7fcd632af800, 
    aCallback=0x0, aCallbackData=0x0) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:844
#9  0x00007fcd899d4fba in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=0x7fcd698dc700, aPaintContext=..., 
    aGroupTarget=0x7fcd65ba8ce0) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:736
#10 0x00007fcd899d554e in mozilla::layers::BasicLayerManager::PaintLayer (this=0x7fcd698dc700, aTarget=0x7fcd65ba8ce0, aLayer=0x7fcd69516800, 
    aCallback=0x0, aCallbackData=0x0) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:848
#11 0x00007fcd899d4272 in mozilla::layers::BasicLayerManager::EndTransactionInternal (this=0x7fcd698dc700, aCallback=0x0, aCallbackData=0x0, 
    aFlags=mozilla::layers::LayerManager::END_DEFAULT) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:528
#12 0x00007fcd899d45ff in mozilla::layers::BasicLayerManager::EndEmptyTransaction (this=0x7fcd698dc700, 
    aFlags=mozilla::layers::LayerManager::END_DEFAULT) at /home/kearwood/dev/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:595
#13 0x00007fcd8b42ebcd in PresShell::Paint (this=0x7fcd70189800, aViewToPaint=0x7fcd701d2080, aDirtyRegion=..., aFlags=2)
    at /home/kearwood/dev/mozilla-central/layout/base/nsPresShell.cpp:6267
#14 0x00007fcd8b0d42bf in nsViewManager::Refresh (this=0x7fcd701f9680, aView=0x7fcd701d2080, aRegion=...)
    at /home/kearwood/dev/mozilla-central/view/nsViewManager.cpp:337
#15 0x00007fcd8b0d51ea in nsViewManager::PaintWindow (this=0x7fcd701f9680, aWidget=0x7fcd7f02d380, aRegion=...)
    at /home/kearwood/dev/mozilla-central/view/nsViewManager.cpp:706
#16 0x00007fcd8b0d2dee in nsView::PaintWindow (this=0x7fcd701d2080, aWidget=0x7fcd7f02d380, aRegion=...)
    at /home/kearwood/dev/mozilla-central/view/nsView.cpp:1049
#17 0x00007fcd8b1171ab in nsWindow::OnExposeEvent (this=0x7fcd7f02d380, aEvent=0x7fcd651b7090)
    at /home/kearwood/dev/mozilla-central/widget/gtk/nsWindow.cpp:2222
#18 0x00007fcd8b11e905 in expose_event_cb (widget=0x7fcd79cb9330, event=0x7fcd651b7090)
    at /home/kearwood/dev/mozilla-central/widget/gtk/nsWindow.cpp:5151
#19 0x00007fcd85b00815 in _gtk_marshal_BOOLEAN__BOXED (closure=0x7fcd70148ee0, return_value=0x7ffff5dbe230, n_param_values=<optimized out>, 
    param_values=0x7ffff5dbe2e0, invocation_hint=<optimized out>, marshal_data=0x0) at /build/buildd/gtk+2.0-2.24.23/gtk/gtkmarshalers.c:86
#20 0x00007fcd863273b8 in g_closure_invoke (closure=0x7fcd70148ee0, return_value=0x7ffff5dbe230, n_param_values=2, param_values=0x7ffff5dbe2e0, 
    invocation_hint=0x7ffff5dbe280) at /build/buildd/glib2.0-2.40.2/./gobject/gclosure.c:768
#21 0x00007fcd86338d3d in signal_emit_unlocked_R (node=node@entry=0x7fcd7f031b20, detail=detail@entry=0, instance=instance@entry=0x7fcd79cb9330, 
    emission_return=emission_return@entry=0x7ffff5dbe3b0, instance_and_params=instance_and_params@entry=0x7ffff5dbe2e0)
    at /build/buildd/glib2.0-2.40.2/./gobject/gsignal.c:3551
#22 0x00007fcd863406f9 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, 
    var_args=var_args@entry=0x7ffff5dbe478) at /build/buildd/glib2.0-2.40.2/./gobject/gsignal.c:3317
#23 0x00007fcd86340ce2 in g_signal_emit (instance=instance@entry=0x7fcd79cb9330, signal_id=<optimized out>, detail=detail@entry=0)
    at /build/buildd/glib2.0-2.40.2/./gobject/gsignal.c:3363
#24 0x00007fcd85c10724 in gtk_widget_event_internal (widget=widget@entry=0x7fcd79cb9330, event=event@entry=0x7fcd651b7090)
    at /build/buildd/gtk+2.0-2.24.23/gtk/gtkwidget.c:5010
#25 0x00007fcd85c10b41 in IA__gtk_widget_send_expose (widget=widget@entry=0x7fcd79cb9330, event=event@entry=0x7fcd651b7090)
    at /build/buildd/gtk+2.0-2.24.23/gtk/gtkwidget.c:4839
#26 0x00007fcd85a895b8 in IA__gtk_container_propagate_expose (container=<optimized out>, child=0x7fcd79cb9330, event=0x7ffff5dbeac0)
    at /build/buildd/gtk+2.0-2.24.23/gtk/gtkcontainer.c:2757
#27 0x00007fcd85a88144 in gtk_container_expose (widget=0x7fcd79c3c510, event=0x7ffff5dbeac0) at /build/buildd/gtk+2.0-2.24.23/gtk/gtkcontainer.c:2661
#28 0x00007fcd85b00815 in _gtk_marshal_BOOLEAN__BOXED (closure=0x7fcd7f02ff20, return_value=0x7ffff5dbe6f0, n_param_values=<optimized out>, 
    param_values=0x7ffff5dbe7a0, invocation_hint=<optimized out>, marshal_data=0x7fcd85c1cf80 <gtk_window_expose>)
    at /build/buildd/gtk+2.0-2.24.23/gtk/gtkmarshalers.c:86
#29 0x00007fcd863273b8 in g_closure_invoke (closure=0x7fcd7f02ff20, return_value=0x7ffff5dbe6f0, n_param_values=2, param_values=0x7ffff5dbe7a0, 
    invocation_hint=0x7ffff5dbe740) at /build/buildd/glib2.0-2.40.2/./gobject/gclosure.c:768
#30 0x00007fcd86338afb in signal_emit_unlocked_R (node=node@entry=0x7fcd7f031b20, detail=detail@entry=0, instance=instance@entry=0x7fcd79c3c510, 
    emission_return=emission_return@entry=0x7ffff5dbe870, instance_and_params=instance_and_params@entry=0x7ffff5dbe7a0)
    at /build/buildd/glib2.0-2.40.2/./gobject/gsignal.c:3589
#31 0x00007fcd863406f9 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, 
    var_args=var_args@entry=0x7ffff5dbe938) at /build/buildd/glib2.0-2.40.2/./gobject/gsignal.c:3317
#32 0x00007fcd86340ce2 in g_signal_emit (instance=instance@entry=0x7fcd79c3c510, signal_id=<optimized out>, detail=detail@entry=0)
    at /build/buildd/glib2.0-2.40.2/./gobject/gsignal.c:3363
#33 0x00007fcd85c10724 in gtk_widget_event_internal (widget=widget@entry=0x7fcd79c3c510, event=event@entry=0x7ffff5dbeac0)
    at /build/buildd/gtk+2.0-2.24.23/gtk/gtkwidget.c:5010
#34 0x00007fcd85c10b41 in IA__gtk_widget_send_expose (widget=widget@entry=0x7fcd79c3c510, event=event@entry=0x7ffff5dbeac0)
    at /build/buildd/gtk+2.0-2.24.23/gtk/gtkwidget.c:4839
#35 0x00007fcd85aff485 in IA__gtk_main_do_event (event=0x7ffff5dbeac0) at /build/buildd/gtk+2.0-2.24.23/gtk/gtkmain.c:1635
#36 0x00007fcd851ccedf in _gdk_window_process_updates_recurse (window=window@entry=0x7fcd90cfc5a0, expose_region=expose_region@entry=0x7fcd69a25630)
    at /build/buildd/gtk+2.0-2.24.23/gdk/gdkwindow.c:5443
#37 0x00007fcd851fa865 in _gdk_windowing_window_process_updates_recurse (window=window@entry=0x7fcd90cfc5a0, region=region@entry=0x7fcd69a25630)
    at /build/buildd/gtk+2.0-2.24.23/gdk/x11/gdkwindow-x11.c:5643
#38 0x00007fcd851c9b5a in gdk_window_process_updates_internal (window=0x7fcd90cfc5a0) at /build/buildd/gtk+2.0-2.24.23/gdk/gdkwindow.c:5610
#39 0x00007fcd851ca438 in IA__gdk_window_process_all_updates () at /build/buildd/gtk+2.0-2.24.23/gdk/gdkwindow.c:5716
#40 0x00007fcd851ca499 in gdk_window_update_idle (data=<optimized out>) at /build/buildd/gtk+2.0-2.24.23/gdk/gdkwindow.c:5336
- Fixed GetMaskData() function in BasicLayersImpl.cpp to use
  Matrix::PostTranslate rather than Matrix::PreTranslate when applying
  the device offset.
Attachment #8531813 - Flags: review?(matt.woodrow)
Attachment #8531813 - Flags: review?(matt.woodrow) → review+
I am attempting to write a reftest to validate the fix; however, the problem is only reproducible with a non-zero device offset.  As the reftests run without toolbars or other chrome, I may need to find another way.
I have pushed to try to run all reftests on all platforms:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=125ff6b203a9
Bug 1067286 - Part 2: Test
- Added reftest to ensure that the device offset is applied using
  Matrix::PostTranslate rather than Matrix::PreTranslate
(In reply to :kip (Kearwood Gilbert) from comment #13)
> Updating FillRectWithMask() in BasicLayersImpl.cpp such that it doesn't set
> and restore the transform results in correct rendering for this case.
> (Commenting out the two SetTransform lines)
> 
> The mEffectiveTransform value for the mask layer appears to be calculated
> incorrectly, although the transformed clipping rectangle appears correct.
> 
> I have tried setting an identity matrix or copying the Color Layer's
> mEffectiveTransform to the mask layer without seeing the correct result.  I
> suspect that either ComputeEffectiveTransformFormMaskLayer is not being
> called in this particular case or the incorrect transform is being passed
> into it.
> 
> Also, I notice that the back-side rounded rect is rendered correctly when it
> flips -- perhaps the issue relates to preserve-3d and CSS transforms on the
> mask layer.
> 
> Matt: Would you be able to spare some time to review my findings and perhaps
> give me some background on this area of the code?
Thanks for reviewing my patch and helping me track down the issue, clearing the NI..
Flags: needinfo?(matt.woodrow)
Comment on attachment 8533379 [details] [diff] [review]
Bug 1067286 - Part 2: Test

This test is passing in every case, due to a limitation in the reftest framework.  As the reftests are run without any browser Chrome, the "device offset" that was translated incorrectly is (0,0).

I have confirmed that this is the same for all platforms with a set of try pushes.

With only test:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dede2eec1d2d

With test and fix:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1a01f91f6416

I attempted to introduce a device offset through use of a scroll frame and an iframe, also resulting in a (0,0) device offset.

I am NI'ing Matt to see if he has some other suggestions that may be less extreme than adding chrome to the reftest window.
Flags: needinfo?(matt.woodrow)
With the scrollframe, did you scroll it to a non 0,0 position before finishing the reftest?

Scrollframes that are active (have been scrolled before) and aren't at 0,0 should introduce a device offset.
Flags: needinfo?(matt.woodrow)
We're too late in the 35 cycle to take anything speculative or unwieldy so wontfixing this again, we'll look for a fix on 36/37. Kip - can you pick this up again?
Flags: needinfo?(kgilbert)
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #23)
> We're too late in the 35 cycle to take anything speculative or unwieldy so
> wontfixing this again, we'll look for a fix on 36/37. Kip - can you pick
> this up again?
I can pick this up again.  The attached fix has been r+'ed, but lacks a test that verifies the effectiveness of the fix due to the nature of the reftest screen capture masking the rendering issue and always passing.

I will continue to hold this bug and try to identify the appropriate testing required for landing.
Flags: needinfo?(kgilbert)
(In reply to Matt Woodrow (:mattwoodrow) (Away until 5 Jan) from comment #22)
> With the scrollframe, did you scroll it to a non 0,0 position before
> finishing the reftest?
> 
> Scrollframes that are active (have been scrolled before) and aren't at 0,0
> should introduce a device offset.
I have used a scrollframe and can visually see the issue reproduced while the reftest is running locally in Linux; however, the Base64 encoded image logged by the reftest appears to be rendered as though no device offset is applied.

Would it be worth updating the reftest system to get a more true representation of the last rendered frame?  IMHO, the risk of this bug's patch is much less than the changes to the reftests; however, there may be value in improving the accuracy of reftests.
Flags: needinfo?(matt.woodrow)
Feel free to land the code fix immediately. Getting a regression test in is great, but not worth blocking on.

For our accelerated compositors we are reading back from the screen, so they should be very accurate in that sense.

I don't think we can guarantee being able to do that for software, so we do a separate composite to a offscreen target.

It's not obvious why we'd get different results because of that though.
Flags: needinfo?(matt.woodrow)
Keywords: checkin-needed
For the sake of sane tracking, I'd strongly suggest just spinning off landing a test for this to a new dependent bug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/698f2fe16af5
Flags: in-testsuite?
Keywords: checkin-needed
Depends on: 1118392
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #27)
> For the sake of sane tracking, I'd strongly suggest just spinning off
> landing a test for this to a new dependent bug.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/698f2fe16af5

I have added Bug 1118392 to track the change needed to improve the reftest system to correctly test this issue.  The test attached to this bug should detect regressions if the changes I suggest in Bug 1118392 are implemented.

Please advise if any further action is needed.
https://hg.mozilla.org/mozilla-central/rev/698f2fe16af5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Kip, can we have an uplift request for aurora? thanks
Flags: needinfo?(kgilbert)
Comment on attachment 8531813 [details] [diff] [review]
Bug 1067286 - Part 1: Correct application of device offset for mask layers

Approval Request Comment
[Feature/regressing bug #]: 1042423
[User impact if declined]: If declined, elements with border-radius and css transforms will be rendered with the incorrect clipping mask, causing them to have the incorrect shape and in some cases appear invisible.
[Describe test coverage new/current, TBPL]: Tested manually with a range of 2d and 3d transforms, including attachment 650741 [details] of Bug 781701 and attachment 8533379 [details] [diff] [review] of Bug 1067286.  Tested on OSX, Linux, and B2G.  An automated reftest could not be added due to the capture mechanism in reftests masking the issue during the reference image capture.  Bug 1118392 has been added to track improvements to reftest captures that would make this possible. 
[Risks and why]: Risk is low.  Change is very small (one line), and affects only the specific combination of border-radius and css transforms that have been tested.
[String/UUID change made/needed]: None
Flags: needinfo?(kgilbert)
Attachment #8531813 - Flags: approval-mozilla-aurora?
Comment on attachment 8531813 [details] [diff] [review]
Bug 1067286 - Part 1: Correct application of device offset for mask layers

thanks!
Attachment #8531813 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.