1474532 - Youtube 30fps video playback rendered with 60fps

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Sotaro Ikeda [:sotaro]]

Crated by Bug 1470327 comment 4.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Suppress redundant frame generation with async-scene-build enabled

Comment 2

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5798ea5ca988c3167434ec2bb8513d42d1e00bc

Comment 3

[Nicolas Silva [:nical]]

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?

Comment 4

[Sotaro Ikeda [:sotaro]]

With Bug 1474576, AsyncSceneBuild became an only way to build scene.

Comment 5

[Sotaro Ikeda [:sotaro]]

(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()

Comment 6

[Sotaro Ikeda [:sotaro]]

(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 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Suppress redundant frame generation with async-scene-build enabled

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Suppress redundant frame generation with async-scene-build enabled

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 11

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=472febcc69fa93f429c5f551a8a0b6aea6b37ca4

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c269521673a
Suppress redundant frame generation with async-scene-build enabled r=kats

Comment 13

[Nicolas Silva [:nical]]

> 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.

Comment 14

[Sotaro Ikeda [:sotaro]]

Yup :)

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0c269521673a

Comment 16

[Kartikaya Gupta (email:[e-mail])]

https://bugzilla.mozilla.org/show_bug.cgi?id=1467768 covers the polish work mentioned in comment 13, I think.