Fix forced frame rendering

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
16 days ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

(Blocks 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Bug 1461239 added capability to skip WR frame rendering if there is no update. But it forgot to add necessary "force frame rendering".
Assignee: nobody → sotaro.ikeda.g
Attachment #9016141 - Attachment is obsolete: true
Summary: Add necessary force frame rendering → Add necessary forced frame rendering
Blocks: 1461239
Attachment #9016228 - Flags: review?(nical.bugzilla)
Comment on attachment 9016228 [details] [diff] [review]
patch - Add necessary forced frame rendering

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

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +941,5 @@
>      return;
>    }
>  
>    if (mWrBridge) {
> +    mWrBridge->ScheduleForcedGenerateFrame();

Could you explain why we need to force generating the frame in this case?
Comment on attachment 9016228 [details] [diff] [review]
patch - Add necessary forced frame rendering

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

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +941,5 @@
>      return;
>    }
>  
>    if (mWrBridge) {
> +    mWrBridge->ScheduleForcedGenerateFrame();

Oh, sorry, it is a mistake.
Attachment #9016228 - Flags: review?(nical.bugzilla)
Attachment #9016228 - Attachment is obsolete: true
Comment on attachment 9016481 [details] [diff] [review]
patch - Add necessary forced frame rendering

:nical, can you review the patch again?
Attachment #9016481 - Flags: review?(nical.bugzilla)
Comment on attachment 9016481 [details] [diff] [review]
patch - Add necessary forced frame rendering

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

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +1389,5 @@
>  
>  mozilla::ipc::IPCResult
>  WebRenderBridgeParent::RecvScheduleComposite()
>  {
> +  ScheduleForcedGenerateFrame();

As far as I can tell we receive this message for APZ related things for which the render backend should be able to figure out on its own whether the frame has changed without the need to invalidate the rendered frame manually. Could you explain why we need this here as well?
Comment on attachment 9016481 [details] [diff] [review]
patch - Add necessary forced frame rendering

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

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +1389,5 @@
>  
>  mozilla::ipc::IPCResult
>  WebRenderBridgeParent::RecvScheduleComposite()
>  {
> +  ScheduleForcedGenerateFrame();

I agree. It is better to change to ScheduleGenerateFrame().
Attachment #9016481 - Flags: review?(nical.bugzilla)
Comment on attachment 9016481 [details] [diff] [review]
patch - Add necessary forced frame rendering

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

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +1389,5 @@
>  
>  mozilla::ipc::IPCResult
>  WebRenderBridgeParent::RecvScheduleComposite()
>  {
> +  ScheduleForcedGenerateFrame();

In Bug 1461239, I misunderstood this function is use for RecvForcePresent().
Addressed the comment.
Attachment #9016481 - Attachment is obsolete: true
Attachment #9016827 - Flags: review?(nical.bugzilla)
Priority: -- → P2
Attachment #9016827 - Flags: review?(nical.bugzilla) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad88a9f8f35
Add necessary forced frame rendering r=nical
https://hg.mozilla.org/mozilla-central/rev/3ad88a9f8f35
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Summary: Add necessary forced frame rendering → Fix forced frame rendering
Depends on: 1500520
Regressed by: 1571331
No longer regressed by: 1571331
Regressions: 1571331
You need to log in before you can comment on or make changes to this bug.