Closed
Bug 1113435
Opened 10 years ago
Closed 10 years ago
[SMS] Black flickering box appears before the keyboard pops up when composing a message
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
People
(Reporter: KTucker, Assigned: BenWa)
References
()
Details
(Keywords: regression, Whiteboard: [2.2-exploratory-2])
Attachments
(3 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
BenWa
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
A black flickering box appears before the keyboard pops up when composing a new message in SMS.
Repro Steps:
1) Updated Flame to Build ID: 20141215040201
2) Tap on the SMS app.
3) Tap on the "Compose New Message" icon.
4) Pay close attention to the bottom of the screen before the keyboard appears.
Actual:
A flickering black box appears before the keyboard pops up.
Expected:
No flickering occurs.
Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141218040201
Gaia: 58734e8a48157f99d5b733412b600c2e04c954fe
Gecko: 5c7a6294b82a
Gonk: e5c6b275d77ca95fb0f2051c3d2242e6e0d0e442
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Notes:
Repro frequency: 5/5 100%
See attached: video, logcat
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Comment 2•10 years ago
|
||
I've seen this as well, you see this also when you enter a normal thread, so it's not at all keyboard related. Thank you for filing the bug.
If you can find a regression window for this (using an existing thread because it's easier without the keyboard), I'd be happy.
blocking-b2g: --- → 2.2?
Keywords: regressionwindow-wanted
Updated•10 years ago
|
QA Contact: ckreinbring
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(dharris)
Comment 3•10 years ago
|
||
Regression window
Last working
BuildID: 20141023104735
Gaia: f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko: 9781037ac408
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
First broken
BuildID: 20141023110739
Gaia: f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko: d8de0d7e52e0
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Working Gaia / Broken Gecko = Repro
Gaia: f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko: d8de0d7e52e0
Broken Gaia / Working Gecko = No repro
Gaia: f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko: 9781037ac408
Gecko pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9781037ac408&tochange=d8de0d7e52e0
Mozilla Inbound
Last working
BuildID: 20141020171137
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: 223e2f4b0d47
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
First broken
BuildID: 20141020172632
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: fa9c6845338e
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Working Gaia / Broken Gecko = Repro
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: fa9c6845338e
Broken Gaia / Working Gecko = No repro
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: 223e2f4b0d47
Gecko pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=223e2f4b0d47&tochange=fa9c6845338e
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: regressionwindow-wanted
Comment 4•10 years ago
|
||
Caused by the patch to Bug 1085223 - can you take a look Matt
Blocks: 1085223
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(matt.woodrow)
QA Contact: ckreinbring
Updated•10 years ago
|
Component: Gaia::SMS → Graphics: Layers
Product: Firefox OS → Core
Comment 6•10 years ago
|
||
Benoit, I'm not sure if Matt has the bandwidth with MSE, so can you take a look, you reviewed the original patch...
Assignee: nobody → bgirard
Assignee | ||
Comment 7•10 years ago
|
||
I'm already assigned to a series of very similar bug. There's several things going wrong here that I'm going to be addressing in pieces. I'll leave this assigned to me until I can properly dupe it.
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 8•10 years ago
|
||
Bug 1112332 will help. It will fix it for the given STR since we're not scrolling. But we still need to deal with making sure we cull correctly when we hit paint heuristics.
Depends on: 1112332
Comment 9•10 years ago
|
||
The fix for bug 1112332 may end up dealing with bug 1114731 as well, let's keep an eye on it (the black rectangle part towards the end of the video.)
Comment 10•10 years ago
|
||
Hey, I also see black rectangle at the top when I navigate from Thread to Thread List panel in SMS using the most recent PVT build.
Not sure if it's the same issue as discussed in this bug, so attaching video just in case.
Assignee | ||
Comment 11•10 years ago
|
||
Thanks. Yes it's the same one. They're a little all over b2g, moving to getting this fixed ASAP.
Comment 15•10 years ago
|
||
I think I've seen it just today with the latest build from pvtbuilds :(
Assignee | ||
Comment 16•10 years ago
|
||
I can still reproduce this 100% too. Still a bug. I'll pick this up tomorrow.
Should keep an eye on bug 1129467 for this.
Flags: needinfo?(bgirard)
Assignee | ||
Comment 17•10 years ago
|
||
It's not bug 1129467
http://people.mozilla.org/~bgirard/cleopatra/#report=24c6f342c1e24830847051e942b55cd2ebc4fbd1&filter=[{"type"%3A"RangeSampleFilter","start"%3A3743,"end"%3A5169}]
Comment 18•10 years ago
|
||
I don't know if this is related, I see also something similar in the Settings app when entering subpanels. Sometimes white, sometimes black.
Assignee | ||
Comment 19•10 years ago
|
||
This is fixed by comment out ApplyOcclusionCulling.
I'd rather we fix ApplyOcclusionCulling but if it comes to it the risk of disabling ApplyOcclusionCulling is nearly zero so we can take that to ship 2.2.
Assignee | ||
Comment 20•10 years ago
|
||
Ok, I found the bug. When drawing progressively we no longer adjust the layer' visible region, only the compositable.
When we cull we only look at the layers' visible region which isn't opaque in the case of progressive drawing. Thus we punch a hole in the bottom layer and get garbage.
I'm a bit surprised that the garbage I see is always the same however.
Assignee | ||
Comment 21•10 years ago
|
||
See Comment 20 for an explanation of the fix.
Attachment #8560150 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•10 years ago
|
Attachment #8560150 -
Attachment description: https://bugzilla.mozilla.org/show_bug.cgi?id=1113435#c20 → Culling fix for progressive drawing
Assignee | ||
Comment 22•10 years ago
|
||
We could also fix this by having the progressive draw update the shadow visible region. That change is however more complex and regression prone.
Comment 23•10 years ago
|
||
Comment on attachment 8560150 [details] [diff] [review]
Simple culling fix (to be uplifted)
Review of attachment 8560150 [details] [diff] [review]:
-----------------------------------------------------------------
It would be better if occlusion culling just used the correct region instead of testing whether the region that it's using is different from the correct one.
Attachment #8560150 -
Flags: review-
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8560150 -
Attachment is obsolete: true
Attachment #8560150 -
Flags: review?(matt.woodrow)
Attachment #8560985 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 25•10 years ago
|
||
Updated•10 years ago
|
Attachment #8560985 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 26•10 years ago
|
||
Try push before jeff' comment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ae9e70e1e83
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Comment 28•10 years ago
|
||
Benoit,
Is 1109516 a duplicate of this? Would love to get rid of it if so.
--Larissa
Flags: needinfo?(bgirard)
Assignee | ||
Comment 30•10 years ago
|
||
I replied in bug 1109516 with instruction how to verify if it's a dupe of this.
Flags: needinfo?(bgirard)
Updated•10 years ago
|
Assignee | ||
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
Comment on attachment 8560150 [details] [diff] [review]
Simple culling fix (to be uplifted)
The plan:
- Take this patch on central and uplift once it bakes.
- Find the failures for the second version and put that one on central after.
Attachment #8560150 -
Attachment description: Culling fix for progressive drawing → Simple culling fix (to be uplifted)
Attachment #8560150 -
Flags: review- → review?(jmuizelaar)
Updated•10 years ago
|
Attachment #8560150 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 33•10 years ago
|
||
Comment 34•10 years ago
|
||
Backed out for making the leftA/topA reftests permafail on Android.
https://hg.mozilla.org/integration/mozilla-inbound/rev/14fad3f56818
Comment 35•10 years ago
|
||
Comment on attachment 8560985 [details] [diff] [review]
culling fixes v2
Review of attachment 8560985 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +1152,5 @@
> mCompositor = aManager->GetCompositor();
> }
>
> +const nsIntRegion&
> +LayerComposite::GetRenderedVisibleRegion() {
Fly-by comment, I wonder if it's work calling this GetRenderedOpaqueRegion, as it excludes the low precision region (which is visible, but not necessarily opaque, and probably shouldn't be considered as such).
Comment 36•10 years ago
|
||
Comment on attachment 8560985 [details] [diff] [review]
culling fixes v2
Review of attachment 8560985 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +1154,5 @@
>
> +const nsIntRegion&
> +LayerComposite::GetRenderedVisibleRegion() {
> + if (TiledLayerComposer* tiled = GetTiledLayerComposer()) {
> + return tiled->GetValidRegion();
Another fly-by, maybe related to the failure; Although this should be unnecessary, I wonder if it's worth And-ing this with the ShadowVisibleRegion, for safety?
Comment 37•10 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #36)
> Comment on attachment 8560985 [details] [diff] [review]
> culling fixes v2
>
> Review of attachment 8560985 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +1154,5 @@
> >
> > +const nsIntRegion&
> > +LayerComposite::GetRenderedVisibleRegion() {
> > + if (TiledLayerComposer* tiled = GetTiledLayerComposer()) {
> > + return tiled->GetValidRegion();
>
> Another fly-by, maybe related to the failure; Although this should be
> unnecessary, I wonder if it's worth And-ing this with the
> ShadowVisibleRegion, for safety?
try seems to confirm that as a possible solution(?): https://treeherder.mozilla.org/#/jobs?repo=try&revision=04045acc659a
Updated•10 years ago
|
Assignee | ||
Comment 38•10 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #35)
> Comment on attachment 8560985 [details] [diff] [review]
> culling fixes v2
>
> Review of attachment 8560985 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +1152,5 @@
> > mCompositor = aManager->GetCompositor();
> > }
> >
> > +const nsIntRegion&
> > +LayerComposite::GetRenderedVisibleRegion() {
>
> Fly-by comment, I wonder if it's work calling this GetRenderedOpaqueRegion,
> as it excludes the low precision region (which is visible, but not
> necessarily opaque, and probably shouldn't be considered as such).
I wont be landing this version so this wont be an issue. Thanks for the feedback.
(In reply to Chris Lord [:cwiiis] from comment #37
> > Another fly-by, maybe related to the failure; Although this should be
> > unnecessary, I wonder if it's worth And-ing this with the
> > ShadowVisibleRegion, for safety?
>
> try seems to confirm that as a possible solution(?):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=04045acc659a
Nice. thanks a lot Cwiiis! I'll prepare a patch based on that.
Assignee | ||
Comment 39•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #38)
> I wont be landing this version so this wont be an issue. Thanks for the
> feedback.
>
Opps, I got the patch mixed up, I will be landing that one. I don't like having Opaque in the name since the function doesn't check if it's opaque or not nor should it. How about GetFullyRenderedRegion?
Comment 40•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #39)
> (In reply to Benoit Girard (:BenWa) from comment #38)
> > I wont be landing this version so this wont be an issue. Thanks for the
> > feedback.
> >
>
> Opps, I got the patch mixed up, I will be landing that one. I don't like
> having Opaque in the name since the function doesn't check if it's opaque or
> not nor should it. How about GetFullyRenderedRegion?
That sounds good to me.
Assignee | ||
Comment 41•10 years ago
|
||
Carrying forward r=jrmuizel
Attachment #8560985 -
Attachment is obsolete: true
Attachment #8566147 -
Flags: review+
Assignee | ||
Comment 42•10 years ago
|
||
Since https://treeherder.mozilla.org/#/jobs?repo=try&revision=04045acc659a is effectively the same patch with a different name I'm going to skip pushing the same patch to try.
Assignee | ||
Comment 43•10 years ago
|
||
In that try push culling was still disabled so the try push was just testing dead code:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9fdb7c7c30c
Assignee | ||
Comment 44•10 years ago
|
||
This is a culling problem, it's not related to bug 1112332 or paint heuristics.
No longer depends on: 1112332
Assignee | ||
Comment 45•10 years ago
|
||
Comment 46•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 47•10 years ago
|
||
Comment on attachment 8566147 [details] [diff] [review]
culling fix v3 r=jrmuizel
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1085223
User impact if declined: possible graphical glitches, particularly during animations/transitions
Testing completed: been on master for several days, some test coverage
Risk to taking this patch (and alternatives if risky): low, this patch makes us cull less.
String or UUID changes made by this patch: none
Attachment #8566147 -
Flags: approval-mozilla-b2g37?
Updated•10 years ago
|
Updated•10 years ago
|
status-b2g-master:
--- → fixed
Updated•10 years ago
|
Attachment #8566147 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment 50•10 years ago
|
||
Comment 51•10 years ago
|
||
This issue is verified fixed for the latest Nightly 3.0 and 2.2 builds.
Actual Results: There is not a black flicker before the keyboard comes up.
Environmental Variables:
Device: Flame 3.0 KK (Full Flash) (319 MB)
BuildID: 20150303010233
Gaia: c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko: 0b3c520002ad
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0
Environmental Variables:
Device: Flame 2.2 KK (Full Flash) (319 MB)
BuildID: 20150303002527
Gaia: 3d188c414e30acc392253d5389a42352fcfbc183
Gecko: c89aad487aa5
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Reporter | ||
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•