Closed
Bug 1160102
Opened 9 years ago
Closed 9 years ago
Fix the wrong vsync contorl when screen on/off
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox38.0.5 | --- | unaffected |
firefox39 | --- | unaffected |
firefox40 | --- | fixed |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: jerry, Assigned: jerry)
References
Details
Attachments
(2 files, 3 obsolete files)
3.07 KB,
patch
|
kats
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
3.28 KB,
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1155797 +++ In https://bugzilla.mozilla.org/show_bug.cgi?id=1155797#c22 , it still call the hwc vsync control even we use software timer. There will have problem if both sw and hwc are turned on together.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8599811 -
Flags: review?(mwu)
Updated•9 years ago
|
Attachment #8599811 -
Flags: review?(mwu) → review?(bugmail.mozilla)
Comment 2•9 years ago
|
||
Comment on attachment 8599811 [details] [diff] [review] use VsyncDisplay interface to turn on/off vsync. v1 Review of attachment 8599811 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsWindow.cpp @@ +137,5 @@ > + VsyncSource::Display &display = gfxPlatform::GetPlatform()->GetHardwareVsync()->GetGlobalDisplay(); > + if (enabled) { > + display.EnableVsync(); > + } else { > + display.DisableVsync(); Please rebase on top of bug 1157874, this code is different now. Also EnableVsync()/DisableVsync() need to be called on the main thread, and it doesn't look like this runs on the main thread.
Attachment #8599811 -
Flags: review?(bugmail.mozilla) → review-
Assignee | ||
Comment 3•9 years ago
|
||
rebase to current m-c.
Attachment #8599811 -
Attachment is obsolete: true
Attachment #8600839 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8600839 [details] [diff] [review] use VsyncDisplay interface to turn on/off vsync. v2 Review of attachment 8600839 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsWindow.cpp @@ +1115,5 @@ > } > > void > nsScreenManagerGonk::DisplayEnabled(bool aEnabled) > { All hal function already runs at main thread. Should we care about the thread model here? https://dxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#378
Assignee | ||
Comment 5•9 years ago
|
||
unlike attachment 8600839 [details] [diff] [review], use runnable method here.
Attachment #8600851 -
Flags: review?(bugmail.mozilla)
Comment 6•9 years ago
|
||
Comment on attachment 8600839 [details] [diff] [review] use VsyncDisplay interface to turn on/off vsync. v2 Review of attachment 8600839 [details] [diff] [review]: ----------------------------------------------------------------- This is ok since you have the MOZ_ASSERT to make sure it is on the main thread. ::: widget/gonk/nsWindow.cpp @@ +1131,1 @@ > NS_DispatchToMainThread(aEnabled ? mScreenOnEvent : mScreenOffEvent); Why is this code doing a DispatchToMainThread if this function already runs on the main thread? Is it just to delay processing until later? If so that should probably be commented.
Attachment #8600839 -
Flags: review?(bugmail.mozilla) → review+
Updated•9 years ago
|
Attachment #8600851 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6) > Comment on attachment 8600839 [details] [diff] [review] > use VsyncDisplay interface to turn on/off vsync. v2 > ::: widget/gonk/nsWindow.cpp > @@ +1131,1 @@ > > NS_DispatchToMainThread(aEnabled ? mScreenOnEvent : mScreenOffEvent); > > Why is this code doing a DispatchToMainThread if this function already runs > on the main thread? Is it just to delay processing until later? If so that > should probably be commented. Hi Michael, The [1] comes from [2]. I think we call PHal function on main thread. Should we need to post the runnable to main thread here? Like: NS_DispatchToMainThread(aEnabled ? mScreenOnEvent : mScreenOffEvent); [1] https://hg.mozilla.org/mozilla-central/annotate/dc5f85980a82/widget/gonk/nsWindow.cpp#l1120 [2] https://hg.mozilla.org/mozilla-central/rev/40a51c5dca9e#l6.77
Flags: needinfo?(mwu)
Comment 8•9 years ago
|
||
Check GonkDisplayICS - it fires a screen on/off event from a separate thread.
Flags: needinfo?(mwu)
Assignee | ||
Updated•9 years ago
|
Attachment #8600839 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8600851 [details] [diff] [review] use VsyncDisplay interface to turn on/off vsync. v3 Thanks to :mwu, SetEnabled() will call at non-main-thread in ICS[1]. [1] https://hg.mozilla.org/mozilla-central/annotate/34828fed1639/widget/gonk/libdisplay/GonkDisplayICS.cpp#l164
Attachment #8600851 -
Flags: review?(bugmail.mozilla)
Updated•9 years ago
|
Attachment #8600851 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 10•9 years ago
|
||
try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc3ed95b6994
Assignee | ||
Comment 11•9 years ago
|
||
Please land the attachment 8600851 [details] [diff] [review] to m-c. try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc3ed95b6994 I saw the similar failed for m1 m11 m13 m20 test.
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb1e6f993a7
Keywords: checkin-needed
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8600851 [details] [diff] [review] use VsyncDisplay interface to turn on/off vsync. v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1147753. We get flickering on n5-l if we turn on hw vsync. User impact if declined: If we init vsync hw timer failed, we will turn on both hw and sw timer after screen turn off and on. Then, compositor will compose more than once and cpu will always be busy. Testing completed: Manual test at flame-kk and nexus5-l. It seems work well. Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none
Attachment #8600851 -
Flags: approval-mozilla-b2g37?
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8cb1e6f993a7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 15•9 years ago
|
||
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #13) > Comment on attachment 8600851 [details] [diff] [review] > use VsyncDisplay interface to turn on/off vsync. v3 > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): > Bug 1147753. We get flickering on n5-l if we turn on hw vsync. Given vsync is off in 2.2 I am not sure if we need this , unless there is a plan to enable it. Can you confirm? > > User impact if declined: > If we init vsync hw timer failed, we will turn on both hw and sw timer after > screen turn off and on. Then, compositor will compose more than once and cpu > will always be busy. > > Testing completed: > Manual test at flame-kk and nexus5-l. It seems work well. > > Risk to taking this patch (and alternatives if risky): > low > String or UUID changes made by this patch: > none
Updated•9 years ago
|
Flags: needinfo?(hshih)
Comment 16•9 years ago
|
||
(In reply to bhavana bajaj [:bajaj] from comment #15) > (In reply to Jerry Shih[:jerry] (UTC+8) from comment #13) > > Comment on attachment 8600851 [details] [diff] [review] > > use VsyncDisplay interface to turn on/off vsync. v3 > > > > [Approval Request Comment] > > Bug caused by (feature/regressing bug #): > > Bug 1147753. We get flickering on n5-l if we turn on hw vsync. > Given vsync is off in 2.2 I am not sure if we need this , unless there is a > plan to enable it. Can you confirm? Vsync is enabled on 2.2, just not on L devices. Other devices may hit this same problem if they are on kit-kat and encounter the same issue as the Nexus 5. This seems to be the case from https://bugzilla.mozilla.org/show_bug.cgi?id=1155797#c55.
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to bhavana bajaj [:bajaj] from comment #15) > (In reply to Jerry Shih[:jerry] (UTC+8) from comment #13) > > Comment on attachment 8600851 [details] [diff] [review] > > use VsyncDisplay interface to turn on/off vsync. v3 > > > > [Approval Request Comment] > > Bug caused by (feature/regressing bug #): > > Bug 1147753. We get flickering on n5-l if we turn on hw vsync. > Given vsync is off in 2.2 I am not sure if we need this , unless there is a > plan to enable it. Can you confirm? > > We already enable vsync on kk device in 2.2. If the device has the problem to init vsync, we will hit the problem as [1]. Vsync works well on Flame-kk, but "nexus5-kk" hits [1]. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1155797#c22
Flags: needinfo?(hshih) → needinfo?(bbajaj)
Updated•9 years ago
|
Flags: needinfo?(bajaj.bhavana)
Attachment #8600851 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment 18•9 years ago
|
||
Needs rebasing for b2g37 uplift.
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
status-firefox38:
--- → unaffected
status-firefox38.0.5:
--- → unaffected
status-firefox39:
--- → unaffected
Flags: needinfo?(hshih)
Assignee | ||
Comment 19•9 years ago
|
||
v2.2 doesn't have bug 1157874. Create a vsync control runnable object to handle the main thread task.
Attachment #8604485 -
Flags: review?(bugmail.mozilla)
Comment 20•9 years ago
|
||
Comment on attachment 8604485 [details] [diff] [review] use VsyncDisplay interface to turn on/off vsync for v2.2. v1 Review of attachment 8604485 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nsWindow.cpp @@ +147,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + VsyncSource::Display &display = > + gfxPlatform::GetPlatform()->GetHardwareVsync()->GetGlobalDisplay(); > + if(aEnabled) { nit: space between if and (
Attachment #8604485 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 21•9 years ago
|
||
fix indent.
Assignee | ||
Updated•9 years ago
|
Attachment #8604485 -
Attachment is obsolete: true
Flags: needinfo?(hshih)
Assignee | ||
Comment 22•9 years ago
|
||
Please land attachment 8604692 [details] [diff] [review] to gecko 2.2. carry r+ and approval from comment 20 and attachment 8600851 [details] [diff] [review].
Flags: needinfo?(ryanvm)
Comment 23•9 years ago
|
||
Thanks :) https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7cb9621d7e1b
Flags: needinfo?(ryanvm)
You need to log in
before you can comment on or make changes to this bug.
Description
•