Closed Bug 1342380 Opened 7 years ago Closed 7 years ago

Enable VsyncScheduler again

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 1 obsolete file)

Since RenderThread was enabled, VsyncScheduler was not used. Instead, new composition is triggered for each WebRenderBridgeParent::HandleDPEnd() call. It sometimes causes too often composition.
Assignee: nobody → sotaro.ikeda.g
Blocks: webrender
Attachment #8840829 - Flags: review?(bugmail)
Comment on attachment 8840829 [details] [diff] [review]
patch - Enable VsyncScheduler again

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

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +475,5 @@
>    MOZ_ASSERT((uint32_t)(size.width * 4) == stride);
>  
> +  if (mCompositorScheduler->NeedsComposite()) {
> +    mCompositorScheduler->CancelCurrentCompositeTask();
> +    mApi->GenerateFrame();

I think we should replace this call to GenerateFrame() with mCompositorScheduler->ForceComposeToTarget(nullptr, nullptr);

It does the same thing, but it will also update the CompositorVsyncScheduler's mLastCompose timestamp which I think is important to avoid needlessly recompositing.

::: gfx/webrender_bindings/src/bindings.rs
@@ +170,5 @@
>                                preserve_frame_state);
> +}
> +
> +#[no_mangle]
> +pub unsafe extern fn wr_api_generate_frame(api: &mut RenderApi) {

does this need to be unsafe?
Attachment #8840829 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Comment on attachment 8840829 [details] [diff] [review]
> patch - Enable VsyncScheduler again
> 
> Review of attachment 8840829 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderBridgeParent.cpp
> @@ +475,5 @@
> >    MOZ_ASSERT((uint32_t)(size.width * 4) == stride);
> >  
> > +  if (mCompositorScheduler->NeedsComposite()) {
> > +    mCompositorScheduler->CancelCurrentCompositeTask();
> > +    mApi->GenerateFrame();
> 
> I think we should replace this call to GenerateFrame() with
> mCompositorScheduler->ForceComposeToTarget(nullptr, nullptr);
> 
> It does the same thing, but it will also update the
> CompositorVsyncScheduler's mLastCompose timestamp which I think is important
> to avoid needlessly recompositing.

Yea, I will update it in a next patch.
 
> ::: gfx/webrender_bindings/src/bindings.rs
> @@ +170,5 @@
> >                                preserve_frame_state);
> > +}
> > +
> > +#[no_mangle]
> > +pub unsafe extern fn wr_api_generate_frame(api: &mut RenderApi) {
> 
> does this need to be unsafe?

It is not necessary.
Attachment #8840829 - Attachment is obsolete: true
Attachment #8841093 - Flags: review+
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4f95477fb628e16bf04145da695fb039a4086b90
> 
> Needs to update also how to delete ImageKeys.

Ah, since Bug 1342246, ImageKeys allocation often causes conflicts. It is a regression of 1342246.
(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> 
> Ah, since Bug 1342246, ImageKeys allocation often causes conflicts. It is a
> regression of 1342246.

Bug 1342754 is created for it.
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/99933cb429ce
Enable VsyncScheduler again r=kats
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.