1149923 - [Flame][Search] The search bar becomes a rectangle when you switch device to landscape mode.

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[Lance（leave from Mozilla）]

[1.Description]:
[Flame v2.2 & v3.0][E.me Integration] After you launch Browser, tap the address bar, then switch device to landscape, the search bar box will become a rectangle.
Found time:10:24
See attachment: 2015-04-01-10-23-59.png and logcat_1024.txt

[2.Testing Steps]: 
1.Launch Browser.
2.Tap the address bar.
3.Switch device to landscape, input some words.

[3.Expected Result]: 
3.The corners of search box should be arc-shaped.

[4.Actual Result]: 
3.The search box becomes a rectangle.

[5.Reproduction build]: 
Flame 2.2(Affected):
Build ID               20150331002503
Gaia Revision          cc11248ab69f13e89416c8e6bb2e184187e72088
Gaia Date              2015-03-30 22:22:58
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/90a26917ee8f
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150331.034811
Firmware Date          Tue Mar 31 03:48:21 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0(Affected):
Build ID               20150331160205
Gaia Revision          03164bd160809747e6a198e0dba1b7c3ee7789f5
Gaia Date              2015-03-31 14:48:14
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/18a8ea7c2c62
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150331.191641
Firmware Date          Tue Mar 31 19:16:50 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

[Lance（leave from Mozilla）]

Attachment: logcat_1024.txt

Comment 2

[Lance（leave from Mozilla）]

Attachment: 2015-04-01-10-23-59.png

Comment 3

[Hermes Cheng (inactive after July 27, 2015)]

I think it is a CSS layout problem. Could you take a look?

Comment 4

[Dale Harvey (:daleharvey)]

I have noticed this, will take a look

Comment 5

[Candice Serran (:cserran)]

not a blocker, quick css change that team will fix

Comment 6

[Dale Harvey (:daleharvey)]

This isnt a basic CSS problem, its a graphics issue.

I tried reproducing the CSS @ http://jsbin.com/yamasikeda/1/edit and cant get it to render the same

#rocketbar-form has the background and border-radius applied, if I remove the transform from #rocketbar then the border-radius is rendered, at all other times it is as well, just in the specific expanded in landscape case it doesnt render properly (it does change the appearance but not by much)

I think we have seen this before, Chris / Kevin happen to remember the previous bug?

Comment 7

[Chris Lord [:cwiiis]]

Sounds like bug 1062475.

Comment 8

[Dale Harvey (:daleharvey)]

Sorry it autosubmitted, Chiajung is this something you would be able to look into? its not 100% but very close to consistent steps to reproduce (open browser app, go to landscape, expand the rocketbar)

Comment 9

[Kevin Grandon :kgrandon]

Ping me again if bug 1062475 turns out to be a dead end and you need further investigation here. Thanks.

Comment 10

[Dale Harvey (:daleharvey)]

Deassigning myself and putting in the right component, unless graphics can advice a possible workaround I think this is too deep in graphics for me to manage, sorry

Comment 11

[Mason Chang [Inactive] [:mchang]]

From comment 3 and 6, not sure this is actually a graphics bug versus a layout bug.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Dale Harvey (:daleharvey) from comment #8)
> its not 100% but very close to consistent steps to reproduce (open
> browser app, go to landscape, expand the rocketbar)

FWIW: I wasn't able to reproduce on 3.0 *until I typed some characters* into the rocketbar (in portrait orientation). Then I was able to reproduce consistently.

Comment 13

[Daniel Holbert [:dholbert]]

I see this bug when #rocketbar gets the "active" class.

Looking in devtools, we have this rule (from #rocketbar.active)
  #rocketbar.active {
     opacity: 1;
     [...]
  }

If I tweak that opacity (in devtools) to e.g. 0.95 or 0.5, I don't hit this bug. So I think this is a layerization-related bug.

Comment 14

[Daniel Holbert [:dholbert]]

(Or rather: this bug can be worked around by explicitly triggering layerization, I guess.)

I think that means this is more graphics than layout, as originally suspected.  Reclassifying as Gfx:Layers for now.

Comment 15

[Chiajung Hung [:chiajung]]

I can not reproduce this on my device(3/26 code base), so I think this may be a regression.

Comment 16

[Chiajung Hung [:chiajung]]

:dholbert told me that this issue may be reproducible on Flame 2.2.

However, I can not reproduce this on both local build of 2.2 (4/6) and 2.2 PVT build (4/8). I tried STR in comment 0 and comment 12, but no luck.

@Lance
Can you try to disable hardware composer and see if it still happen? And what base image do you use to test this?

Comment 17

[Lance（leave from Mozilla）]

Hi Chiajung,

No matter we disable or enable hardware composer, the bug still exists in latest Flame 2.2 & 3.0. And the base image is v18D.

Fail rate:5/5
See attachment:1042.mp4

Flame 2.2 version(Affected):
Build ID               20150407162504
Gaia Revision          ea735c21bfb0d78333213ff0376fce1eac89ead6
Gaia Date              2015-04-07 20:58:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3f86ddb7f719
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150407.195227
Firmware Date          Tue Apr  7 19:52:39 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 version(Affected):
Build ID               20150407160201
Gaia Revision          84cbd4391fb7175d5380fa72c04d68873ce77e6d
Gaia Date              2015-04-07 17:33:14
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/078128c2600a
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150407.193600
Firmware Date          Tue Apr  7 19:36:12 EDT 2015
Bootloader             L1TC000118D0

Comment 18

[Lance（leave from Mozilla）]

Attachment: 1042.MP4

Comment 19

[Chiajung Hung [:chiajung]]

That's weird, I can not reproduce this in several try, so I can not tell whether it is a gfx issue or not. 

To disable hardware composer is to make all layer composition runs on GPU, which was the cause of previous bugs.

Comment 20

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Chiajung Hung [:chiajung] from comment #15)
> I can not reproduce this on my device(3/26 code base), so I think this may
> be a regression.

I can reproduce this issue on latest 3.0 as well as on 3/26 nightly central. Removing window-wanted tag.

Reproducible on:
Device: Flame 3.0
BuildID: 20150326010205
Gaia: 8dc256a2de273be3abfa2cb2103d872d677834f7
Gecko: 37d3dcbf23a9
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Device: Flame 3.0
BuildID: 20150414072436
Gaia: c8cb0c0ebb8dd1f5c0c9037e38f8e4b237beb77b
Gecko: 388f5861dc7d
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Comment 22

[KTucker [:KTucker][Inactive 3/4/2016]]

[Blocking Requested - why for this release]:

This was marked as a blocker on the duped bug so nominating this 2.5?

Comment 23

[Johan Lorenzo [:jlorenzo]]

Carrying 2.5+ from bug 1184477.

Comment 24

[Bobby Chien]

Jerry, could you help on this?

Comment 25

[Gregor Wagner [:gwagner]]

Any updates here?

Comment 26

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I haven't check this bug yet.
Will have a look later.

Comment 27

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Jeremy and Boris,
Could you please check this issue?

Comment 28

[Jeremy Chen ([e-mail])]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #27)
> Jeremy and Boris,
> Could you please check this issue?

I can reproduce this issue on latest 3.0. As comment 12 said, typing some characters into the rocketbar (either in portrait/landscape orientation) is an essential step. I'll start the investigation today.

Build ID               20150816150205
Gaia Revision          47c91ffe7f500ca1aaa60de0aabf4d2429120733
Gaia Date              2015-08-14 18:55:02
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/0876695d1abdeb363a780bda8b6cc84f20ba51c9
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150816.182145
Firmware Date          Sun Aug 16 18:21:56 EDT 2015
Bootloader             L1TC000118D0

Comment 29

[Ken Chang[:kenkai]]

Jeremy, Since you are taking action, assign this bug to you first.

Comment 31

[Jeremy Chen ([e-mail])]

Attachment: expected_coord_transform

Comment 32

[Jeremy Chen ([e-mail])]

Attachment: actual_coord_transform

Comment 33

[Jeremy Chen ([e-mail])]

After tracing the code path back and forth, I finally found the root cause. 

In original code path, we compute mEffectiveTransform for each mask in [1], then pass it to  maskQuadTransform in [2]. The maskQuadTransform is then sent as a parameter to shader program by BindMaskForProgram() in [3]. After binding, a mis-sampled mask is generated, which leads to this bug, a broken mask effect. Looks like mask effect in rotation case is not well-handled.

To be more clear about the mis-sampling, please see pictures [4] and [5]. We expect maskQuadTransform may take rotation into consideration, like [4] shows. However, the actual result is like [5] shows. I also recorded a video (please see [6]) to illustrate this phenomenon. In [6], for observation purpose, I change the mask color. Rocketbar height and border-radius are also enlarged. 

Actions:
From patch for Bug 768079, MASK_3D will use intermediate surface, however MASK_2D is not taken into consideration. It can be seen from [7], MASK_2D on mobile will not always enter the for loop since MOZ_GFX_OPTIMIZE_MOBILE flag is set to true. In MASK_2D case, we check another condition, (!contTransform.PreservesAxisAlignedRectangles()), which is designed for checking clipRect case. I think MASK_2D should have its own logic. I made few tests and found that shader program can only handle MASK_2D with positive scaling and/or translation. It means that rotations and negative scalings (i.e. flips) should use intermediate surface. I've tested locally and the bug is fixed. I'll remove debug messages and upload a clean patch soon.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#1203
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp?from=compositorogl.cpp#1040
[3] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp?from=compositorogl.cpp#1186
[4] attached file: expected_coord_transform
[5] attached file: actual_coord_transform
[6] attached video: http://youtu.be/mx4pLx_25t4 
[7] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#1226-1245

Comment 34

[Jeremy Chen ([e-mail])]

Attachment: Let 2D mask effect can check whether to use IntermediateSurface or not in its own logic. r=roc (v1)

Separate maskLayer check logic from clipRect check logic. To avoid running for loop twice, two bool flags, checkClipRect and checkMaskLayers, are added.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8653424 [details] [diff] [review]
Let 2D mask effect can check whether to use IntermediateSurface or not in its own logic. r=roc (v1)

Review of attachment 8653424 [details] [diff] [review]:
-----------------------------------------------------------------

Please make a reftest for this

Comment 36

[Jeremy Chen ([e-mail])]

Attachment: Part1: Let 2D mask effect can check whether to use IntermediateSurface or not in its own logic. r=roc (carry r+, v2)

Correct the usage of HasNonAxisAlignedTransform() in this patch. Comments for HasNonAxisAlignedTransform() in Matrix.h could be ambiguous, so I fixed it.

Comment 37

[Jeremy Chen ([e-mail])]

Attachment: Part2: Reftest. r=roc (v1)

To simulate the phenomenon of changing from portrait mode to landscape mode, a rotation with 90 degrees is used. "will-change: transform;" is used here to avoid layer merging and force mask has its independent layer.

Comment 38

[Jeremy Chen ([e-mail])]

Comment on attachment 8654003 [details] [diff] [review]
Part2: Reftest. r=roc (v1)

Got few oranges on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0b0980b0578

Figuring out what's going on, so clear review request first.

Comment 39

[Jeremy Chen ([e-mail])]

Attachment: Part2: Add reftest to testify whether mask layer is rendered correctly. r=roc (v3)

To simulate the phenomenon of changing from portrait mode to landscape mode, a rotation with 90 degrees is used. "will-change: transform;" is used to force mask has its independent layer. According to [1], there always be few distorted pixels caused by border-raduis effect. So, I use fuzzy equivalence in this test.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac07d3dfa330

Comment 40

[Jeremy Chen ([e-mail])]

:roc, thank you for the quick review. I'm thinking about filing a follow up bug to let maskQuadTransform in [1] take ratation into consideration. Thoughts?

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp?from=compositorogl.cpp#1040-1043

Comment 41

[Jeremy Chen ([e-mail])]

try with test only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5582ece8b0f5
try with both test and patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a270b772cb46

Looks good. Please check in Part1 and Part2 patches. Thanks.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jeremy Chen [:jeremychen] from comment #40)
> :roc, thank you for the quick review. I'm thinking about filing a follow up
> bug to let maskQuadTransform in [1] take ratation into consideration.
> Thoughts?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/
> CompositorOGL.cpp?from=compositorogl.cpp#1040-1043

Yes, I guess that's worth doing for FxOS.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40446535dada
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0472553240

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40446535dada
https://hg.mozilla.org/mozilla-central/rev/bf0472553240

Comment 45

[Lee Salzman [:lsalzman]]

It looks like the reftest is causing intermittents (see bug 1202316) on Windows because the fuzz is inadequate. The intermittent started shortly after this test was added. Can you please fix?

Comment 46

[Jeremy Chen ([e-mail])]

(In reply to Lee Salzman [:eihrul] from comment #45)
> It looks like the reftest is causing intermittents (see bug 1202316) on
> Windows because the fuzz is inadequate. The intermittent started shortly
> after this test was added. Can you please fix?

Oops, sorry, It seems the reftest in this bug has not been verified on winxp. I should have verified it in all platforms. I've taken some actions. Please see Bug 1202316.

Comment 47

[Jeremy Chen ([e-mail])]

Set to resolved since Bug 1202316 has been fixed.

Comment 48

[Lance（leave from Mozilla）]

This bug has been verified as "pass" on the latest build of Flame KK master by the STR in comment 0.
Actual result: The corners of search box is arc-shaped.
Occurrence rate: 0/10.

Device: Flame KK master (pass)
Build ID               20151208150206
Gaia Revision          6b430ea7274af4c352de16b75e6bb85d7621ca83
Gaia Date              2015-12-08 06:31:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/a8965ae93c5d098a4f91ad9da72150bb43df07a7
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151208.183034
Firmware Date          Tue Dec  8 18:30:47 EST 2015
Fireware Version       v18D v4
Bootloader             L1TC000118D0