Closed Bug 1474532 Opened Last year Closed Last year

Youtube 30fps video playback rendered with 60fps

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 2 obsolete files)

Assignee: nobody → sotaro.ikeda.g
Attachment #8990920 - Attachment description: patch - Suppress redundunt frame generation with async-scene-build enabled → patch - Suppress redundant frame generation with async-scene-build enabled
Attachment #8990920 - Flags: review?(nical.bugzilla)
Comment on attachment 8990920 [details] [diff] [review]
patch - Suppress redundant frame generation with async-scene-build enabled

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

::: gfx/layers/wr/AsyncImagePipelineManager.cpp
@@ +297,5 @@
>        updateDisplayList = true;
>      }
>  
>      // Request to generate frame if there is an update.
> +    if (!gfxPrefs::WebRenderAsyncSceneBuild() && (updateDisplayList || !op.isNothing())) {

Shouldn't we still schedule frame if we didn't update the display list?
Depends on: 1474576
With Bug 1474576, AsyncSceneBuild became an only way to build scene.
(In reply to Nicolas Silva [:nical] from comment #3)
> >      }
> >  
> >      // Request to generate frame if there is an update.
> > +    if (!gfxPrefs::WebRenderAsyncSceneBuild() && (updateDisplayList || !op.isNothing())) {
> 
> Shouldn't we still schedule frame if we didn't update the display list?

Schedule frame for no display list update(only resource update) is also done via WRSceneBuilder thread like the following. Then we do not need to schedule frame here.

------------------------------------

SceneBuilder::process_message()
->SceneBuilderHooks::post_resource_update();
->wr_schedule_render()
->CompositorBridgeParent::ScheduleRenderOnCompositorThread()
->CompositorBridgeParent::ScheduleComposition()
(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> (In reply to Nicolas Silva [:nical] from comment #3)
> > >      }
> > >  
> > >      // Request to generate frame if there is an update.
> > > +    if (!gfxPrefs::WebRenderAsyncSceneBuild() && (updateDisplayList || !op.isNothing())) {
> > 
> > Shouldn't we still schedule frame if we didn't update the display list?
> 
> Schedule frame for no display list update(only resource update) is also done
> via WRSceneBuilder thread like the following. Then we do not need to
> schedule frame here.
> 

On current gecko, the frame generation is duplicated for video, then became high fps.
Comment on attachment 8991172 [details] [diff] [review]
patch - Suppress redundant frame generation with async-scene-build enabled

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

This makes sense to me. s/redundunt/redundant/ in the commit message though.
Attachment #8991172 - Flags: review+
Thanks!
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c269521673a
Suppress redundant frame generation with async-scene-build enabled r=kats
> Schedule frame for no display list update(only resource update) is also done via WRSceneBuilder thread like the following. Then we do not need to schedule frame here.

Ah, we should be able to skip the scene builder thread for video frames in a lot of cases as they don't abide to the frame consistency rule, but that's something we can look into polishing later.
Yup :)
https://hg.mozilla.org/mozilla-central/rev/0c269521673a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.