1416533 - Try to composite 30fps video as 60fps

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Sotaro Ikeda [:sotaro]]

WebRenderBridgeParent tries to composite 30fps video as 60fps. Non-webrender cases composite it as 30fps.

The following part check invalidation and if there is no invalid region and skip composite if there is no invalid region.
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/mlgpu/LayerManagerMLGPU.cpp#313
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#523

Comment 1

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> The following part check invalidation and if there is no invalid region and
> skip composite if there is no invalid region.
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/mlgpu/
> LayerManagerMLGPU.cpp#313
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> LayerManagerComposite.cpp#523

non-WebRender case always do drawing during getting snapshot.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Skip to generate frame if there is no update

WebRenderBridgeParent::CompositeToTarget() always tries to call mApi->GenerateFrame(). But we could skip to call the GenerateFrame() if there is no display list update, no animation, no apz update, no video update.

Comment 3

[Sotaro Ikeda [:sotaro]]

If there is an async pipeline for video, WebRenderBridgeParent::CompositeToTarget() is called regularly to check if there is an update to display list or to wr::ResourceUpdate of the video. If there is no update related to the video and no another update, we could skip to call mApi->GenerateFrame().

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Skip to generate frame if there is no update

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Skip to generate frame if there is no update

Comment 6

[Sotaro Ikeda [:sotaro]]

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

Comment 7

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

Comment on attachment 8932300 [details] [diff] [review]
patch - Skip to generate frame if there is no update

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

Some comments and questions below

::: gfx/layers/wr/AsyncImagePipelineManager.cpp
@@ +272,5 @@
>                               !!pipeline->mCurrentTexture;
>  
> +    // Request to generate frame if there is an update.
> +    if (updateDisplayList || !op.isNothing()) {
> +      SetWillGenerateFrame();

Somebody else should review this part, I don't know if this is correct

::: gfx/layers/wr/AsyncImagePipelineManager.h
@@ +179,5 @@
>  
>    nsClassHashtable<nsUint64HashKey, PipelineTexturesHolder> mPipelineTexturesHolders;
>    nsClassHashtable<nsUint64HashKey, AsyncImagePipeline> mAsyncImagePipelines;
>    uint32_t mAsyncImageEpoch;
> +  bool mWillGenerateFrame;

Is this variable threadsafe? It seems to me that SetWillGenerateFrame and GetAndResetWillGenerateFrame might be called from different threads. If not please add thread assertions in those functions.

And actually it seems weird to be putting this flag in AsyncImagePipelineManager instead of just keeping it in WebRenderBridgeParent; most of the rest of the logic regarding this is in that class. What happens if WebRenderBridgeParent::UpdateWebRender is called and a new AsyncImagePipelineManager is installed? Shouldn't we set this flag back to true on the new AsyncImagePipelineManager? Again it seems like keeping the flag in WebRenderBridgeParent would be better because it wouldn't have this problem.

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +1197,5 @@
>    mAsyncImageManager->ApplyAsyncImages();
>  
> +  if (!mAsyncImageManager->GetCompositeUntilTime().IsNull()) {
> +    // Will check if we will generate frame.
> +    mCompositorScheduler->ScheduleComposition();

This comment doesn't make sense to me. If I understand correctly, this code is scheduling another composition because there might be another frame that we want to generate after this one. This call to ScheduleComposition() will trigger another call to this CompositeToTarget function, which is the thing that will check if we actually want to generate the frame or not. If this is correct please expand the comment a little to explain this in more detail.

@@ +1308,4 @@
>  {
>    if (mCompositorScheduler) {
>      mCompositorScheduler->ScheduleComposition();
> +    mAsyncImageManager->SetWillGenerateFrame();

It would be better if the SetWillGenerateFrame call happens first, followed by the ScheduleComposition call

::: gfx/layers/wr/WebRenderImageHost.cpp
@@ +71,5 @@
>    newImages.Clear();
>  
> +  if (mWrBridge && mWrBridge->CompositorScheduler() && GetAsyncRef()) {
> +    // Will check if we will generate frame.
> +    mWrBridge->CompositorScheduler()->ScheduleComposition();

Somebody else should review this, I don't know what this code does

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> 
> ::: gfx/layers/wr/AsyncImagePipelineManager.h
> @@ +179,5 @@
> >  
> >    nsClassHashtable<nsUint64HashKey, PipelineTexturesHolder> mPipelineTexturesHolders;
> >    nsClassHashtable<nsUint64HashKey, AsyncImagePipeline> mAsyncImagePipelines;
> >    uint32_t mAsyncImageEpoch;
> > +  bool mWillGenerateFrame;
> 
> Is this variable threadsafe? It seems to me that SetWillGenerateFrame and
> GetAndResetWillGenerateFrame might be called from different threads. If not
> please add thread assertions in those functions.

mWillGenerateFrame is accessed only on Compositor thread I will add ASSERT here.

> And actually it seems weird to be putting this flag in
> AsyncImagePipelineManager instead of just keeping it in
> WebRenderBridgeParent; most of the rest of the logic regarding this is in
> that class.

It is because mWillGenerateFrame could be handled only by root WebRenderBridgeParent. Non-root WebRenderBridgeParent needs to send the info to root WebRenderBridgeParent. But Non-root WebRenderBridgeParent does not have ref of root WebRenderBridgeParent.

AsyncImagePipelineManager is shared among all WebRenderBridgeParents in a same Window. Therefore I choose AsyncImagePipelineManager as store of mWillGenerateFrame flag.


> What happens if WebRenderBridgeParent::UpdateWebRender is called
> and a new AsyncImagePipelineManager is installed?

It could happen only on non-root WebRenderBridgeParent. Old Window just trigger a GenerateFrame. New Window will trigger GenerateFrame when new display list arrives from client.

> Shouldn't we set this flag back to true on the new AsyncImagePipelineManager? Again it seems like
> keeping the flag in WebRenderBridgeParent would be better because it
> wouldn't have this problem.

I could not find a reason to do it. In this case, non-root WebRenderBridgeParent needs to own ref of root WebRenderBridgeParent. I do not find a necessity to do it now. We already use AsyncImagePipelineManager as a shared data store place for WebRenderBridgeParents in a same window.


> 
> ::: gfx/layers/wr/WebRenderBridgeParent.cpp
> @@ +1197,5 @@
> >    mAsyncImageManager->ApplyAsyncImages();
> >  
> > +  if (!mAsyncImageManager->GetCompositeUntilTime().IsNull()) {
> > +    // Will check if we will generate frame.
> > +    mCompositorScheduler->ScheduleComposition();
> 
> This comment doesn't make sense to me. If I understand correctly, this code
> is scheduling another composition because there might be another frame that
> we want to generate after this one. This call to ScheduleComposition() will
> trigger another call to this CompositeToTarget function, which is the thing
> that will check if we actually want to generate the frame or not. If this is
> correct please expand the comment a little to explain this in more detail.

Yes it is correct. I' will update the comment.

> 
> @@ +1308,4 @@
> >  {
> >    if (mCompositorScheduler) {
> >      mCompositorScheduler->ScheduleComposition();
> > +    mAsyncImageManager->SetWillGenerateFrame();
> 
> It would be better if the SetWillGenerateFrame call happens first, followed
> by the ScheduleComposition call
> 

I will update it.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Skip to generate frame if there is no update

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Skip to generate frame if there is no update

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> > What happens if WebRenderBridgeParent::UpdateWebRender is called
> > and a new AsyncImagePipelineManager is installed?
> 
> It could happen only on non-root WebRenderBridgeParent. Old Window just
> trigger a GenerateFrame.

The frame does not have resources related to the non-root WebRenderBridgeParent. They are already removed before generating frame.

Comment 12

[Sotaro Ikeda [:sotaro]]

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

Comment 13

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

Comment on attachment 8932706 [details] [diff] [review]
patch - Skip to generate frame if there is no update

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

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +1197,5 @@
>    mAsyncImageManager->ApplyAsyncImages();
>  
> +  if (!mAsyncImageManager->GetCompositeUntilTime().IsNull()) {
> +    // Trigger another CompositeToTarget() call because there might be another
> +    // frame that we want to generate after this one. 

nit: trailing whitespace

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Skip to generate frame if there is no update

Applied the comment.

Comment 15

[Nicolas Silva [:nical]]

Comment on attachment 8933149 [details] [diff] [review]
patch - Skip to generate frame if there is no update

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

::: gfx/layers/wr/WebRenderBridgeParent.h
@@ +172,5 @@
>    const WebRenderScrollData& GetScrollData() const;
>  
>    void FlushRendering(bool aIsSync);
>  
> +  void ScheduleGenerateFrame();

I think that the difference (if I understand correctly) between SchedulGenerateFrame (will trigger rendering no matter what), and ScheduleComposition (will schedule something that will generate the frame if need be but not necessarily) should be documented here,  because it's not trivial to choose between the two if you haven't read what this bug is about.

Comment 16

[Sotaro Ikeda [:sotaro]]

I am going to add a comment to ScheduleGenerateFrame().

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Skip to generate frame if there is no update

Added comment.

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Skip to generate frame if there is no update

Comment 19

[Sotaro Ikeda [:sotaro]]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e36fdc6b3e
Skip to generate frame if there is no update r=nical,kats

Comment 21

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/34e36fdc6b3e