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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla38
blocking-b2g 2.2+
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: KTucker, Assigned: BenWa)

References

()

Details

(Keywords: regression, Whiteboard: [2.2-exploratory-2])

Attachments

(3 files, 2 obsolete files)

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
Attached file Keyboard Flicker logcat (deleted) —
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
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?
QA Contact: ckreinbring
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(dharris)
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)
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
triage: regression. very ugly.
blocking-b2g: 2.2? → 2.2+
Component: Gaia::SMS → Graphics: Layers
Product: Firefox OS → Core
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
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)
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
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.)
Attached file Black rectangle in Thread List (deleted) —
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.
Thanks. Yes it's the same one. They're a little all over b2g, moving to getting this fixed ASAP.
Need to check if this is fixed.
Flags: needinfo?(bgirard)
I think I've seen it just today with the latest build from pvtbuilds :(
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)
I don't know if this is related, I see also something similar in the Settings app when entering subpanels. Sometimes white, sometimes black.
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.
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.
Attached patch Simple culling fix (to be uplifted) (obsolete) (deleted) — Splinter Review
See Comment 20 for an explanation of the fix.
Attachment #8560150 - Flags: review?(matt.woodrow)
Attachment #8560150 - Attachment description: https://bugzilla.mozilla.org/show_bug.cgi?id=1113435#c20 → Culling fix for progressive drawing
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 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-
Attached patch culling fixes v2 (obsolete) (deleted) — Splinter Review
Attachment #8560150 - Attachment is obsolete: true
Attachment #8560150 - Flags: review?(matt.woodrow)
Attachment #8560985 - Flags: review?(jmuizelaar)
Attachment #8560985 - Flags: review?(jmuizelaar) → review+
Benoit, Is 1109516 a duplicate of this? Would love to get rid of it if so. --Larissa
Flags: needinfo?(bgirard)
I replied in bug 1109516 with instruction how to verify if it's a dupe of this.
Flags: needinfo?(bgirard)
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)
Attachment #8560150 - Flags: review?(jmuizelaar) → review+
Backed out for making the leftA/topA reftests permafail on Android. https://hg.mozilla.org/integration/mozilla-inbound/rev/14fad3f56818
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 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?
(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
(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.
(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?
(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.
Attached patch culling fix v3 r=jrmuizel (deleted) — Splinter Review
Carrying forward r=jrmuizel
Attachment #8560985 - Attachment is obsolete: true
Attachment #8566147 - Flags: review+
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.
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
This is a culling problem, it's not related to bug 1112332 or paint heuristics.
No longer depends on: 1112332
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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?
Attachment #8566147 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
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)
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.

Attachment

General

Created:
Updated:
Size: