Closed Bug 1322504 Opened 4 years ago Closed 4 years ago

Use TextureClient in WebRenderPaintedLayer

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54

People

(Reporter: sotaro, Assigned: ethlin)

References

Details

Attachments

(3 files, 12 obsolete files)

1.03 KB, patch
Details | Diff | Splinter Review
13.74 KB, patch
Details | Diff | Splinter Review
12.83 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → sotaro.ikeda.g
Per discussion with sotaro, I'll use ContentClient to handle TextureClient in WebRenderPaintedLayer.
Assignee: sotaro.ikeda.g → ethlin
Bug 1325227 is going to make transaction of ContentClient to async.
Using SingleTiledContentClient might make the bug simpler.
(In reply to Sotaro Ikeda [:sotaro PTO 28/Dec - 4/Jan] from comment #3)
> Using SingleTiledContentClient might make the bug simpler.

I imported ContentClient first, and it's also simple after I disabled RotationBuffer. I'll upload related patch for review. And I will start to work on SingleTiledContentClient.
Attachment #8822360 - Flags: review?(sotaro.ikeda.g)
We may need to change WR interface to support RotationBuffer. So I disable it for now. And I also fix some format and boundary problem in this patch after using ContentClient.
Attachment #8822362 - Flags: review?(sotaro.ikeda.g)
Attachment #8822360 - Attachment description: Use ContentClient in WebRenderPaintedLayer → Part1. Use ContentClient in WebRenderPaintedLayer
Comment on attachment 8822362 [details] [diff] [review]
Part2. Disable RotationBuffer for WR and fix bound/format problem after using ContentClient

The patch caused the following error on linux. Review+ if the following problem is addressed.

----------------------------------- 0:04.64 /home/sotaro/quantum_gfx_src_6/graphics/gfx/layers/wr/WebRenderBridgeParent.cpp:305:22: error: variable ‘size’ set but not used [-Werror=unused-but-set-variable]
 0:04.64          gfx::IntSize size = dSurf->GetSize();
Attachment #8822362 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8822360 [details] [diff] [review]
Part1. Use ContentClient in WebRenderPaintedLayer

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

ethlin, can you address the comment?

::: gfx/layers/wr/WebRenderLayerManager.cpp
@@ +325,5 @@
>  
> +  // this may result in Layers being deleted, which results in
> +  // PLayer::Send__delete__() and DeallocShmem()
> +  mKeepAlive.Clear();
> +

It is called only during snapshot. The mKeepAlive.Clear() needs to be called alwasys after transaction end.

::: gfx/layers/wr/WebRenderPaintedLayer.h
@@ +61,5 @@
> +    }
> +  }
> +
> +  bool UsedForReadback() { return mUsedForReadback; }
> +  void SetUsedForReadback(bool aUsed) { mUsedForReadback = aUsed; }

UsedForReadback() and SetUsedForReadback() are duplicate of the following. Is there a reason for it?
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h#2044
Attachment #8822360 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro PTO 28/Dec - 4/Jan] from comment #8)
> Comment on attachment 8822360 [details] [diff] [review]
> Part1. Use ContentClient in WebRenderPaintedLayer
> 
> Review of attachment 8822360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ethlin, can you address the comment?
> 
> ::: gfx/layers/wr/WebRenderLayerManager.cpp
> @@ +325,5 @@
> >  
> > +  // this may result in Layers being deleted, which results in
> > +  // PLayer::Send__delete__() and DeallocShmem()
> > +  mKeepAlive.Clear();
> > +
> 
> It is called only during snapshot. The mKeepAlive.Clear() needs to be called
> alwasys after transaction end.
> 

Okay, I should move it to the end of EndTransaction.

> ::: gfx/layers/wr/WebRenderPaintedLayer.h
> @@ +61,5 @@
> > +    }
> > +  }
> > +
> > +  bool UsedForReadback() { return mUsedForReadback; }
> > +  void SetUsedForReadback(bool aUsed) { mUsedForReadback = aUsed; }
> 
> UsedForReadback() and SetUsedForReadback() are duplicate of the following.
> Is there a reason for it?
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h#2044

Nope, I'll remove them.
Address sotaro's comments.
Attachment #8822360 - Attachment is obsolete: true
Attachment #8822954 - Flags: review?(sotaro.ikeda.g)
It looks like we need visible rect info for creating WR image or we may get wrong image region. So I pass the visible rect in TOpDPPushExternalImageId. I also separate the change of preference from this patch.
Attachment #8822955 - Attachment is obsolete: true
Attachment #8823201 - Flags: review?(sotaro.ikeda.g)
Attachment #8822954 - Flags: review?(sotaro.ikeda.g) → review+
The patches seem to cause the regression. During scrolling http://www.yahoo.co.jp/, I saw a bit of rendering problem.
Comment on attachment 8823201 [details] [diff] [review]
Part2. fix bound/format problem after using ContentClient. r=sotaro

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

Can you explain why the patch added "IntRect visibleRect"? Since "WRRect bounds" of OpDPPushExternalImageId delivers almost same information.
Attachment #8823201 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro PTO 28/Dec - 4/Jan] from comment #15)
> Comment on attachment 8823201 [details] [diff] [review]
> Part2. fix bound/format problem after using ContentClient. r=sotaro
> 
> Review of attachment 8823201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you explain why the patch added "IntRect visibleRect"? Since "WRRect
> bounds" of OpDPPushExternalImageId delivers almost same information.

For WebRenderCanvasLayer, the 'bounds' is a transformed rect, so the size may be different from visible rect. Why we need to do the transform for the 'bounds' is because the unit of buffer data in CanvasLayer is not device pixel. Actually, I think we should do the transform for PaintedLayer and ImageLayer too, though normally the transform is identify matrix (I'm not sure when is not).

I sent the result to try server and I found that I should pass visible region to parent side for clipping. I'll change the visibleRect to visibleRegion.
I got many unexpected-pass and some unexpected-fail from try server. I'll mark these tests since unexpected-pass tests are more than unexpected-fail.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f32c540b37e11fd616c9bfac824e7bbebbc98a2&selectedJob=66989832
We should send visible region to parent for culling. For now I do the culling by Moz2D. We should do that in WebRender in the future.
Attachment #8823201 - Attachment is obsolete: true
Attachment #8824904 - Flags: review?(sotaro.ikeda.g)
Attachment #8824904 - Attachment description: Part2. fix bound/format problem after using ContentClient. r=sotaro → Part2. fix bound/format problem after using ContentClient.
Rebase the patch.
Attachment #8822954 - Attachment is obsolete: true
Attached patch Part4. Set reftest flag. (obsolete) — Splinter Review
Change some flags for reftests for passing try server tests.
Attachment #8824906 - Flags: review?(sotaro.ikeda.g)
Attached patch Part4. Set reftest flag. (obsolete) — Splinter Review
Correct some flags.
Attachment #8824906 - Attachment is obsolete: true
Attachment #8824906 - Flags: review?(sotaro.ikeda.g)
Attachment #8824913 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8824913 [details] [diff] [review]
Part4. Set reftest flag.

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

::: 0002-mask.patch
@@ +4,5 @@
> +mask
> +
> +---
> + gfx/layers/wr/WebRenderBorderLayer.cpp  | 1 +
> + gfx/layers/wr/WebRenderCanvasLayer.cpp  | 1 +

Spurious file in this patch

::: layout/reftests/bugs/reftest.list
@@ +1931,5 @@
>  skip-if(!Android) == 1133905-4-vh-rtl.html 1133905-ref-vh-rtl.html
>  skip-if(!Android) fails-if(Android) == 1133905-5-vh-rtl.html 1133905-ref-vh-rtl.html
>  skip-if(!Android) fails-if(Android) == 1133905-6-vh-rtl.html 1133905-ref-vh-rtl.html
>  == 1150021-1.xul 1150021-1-ref.xul
> +fuzzy-if(webrender,139,10) == 1151145-1.html 1151145-1-ref.html

I would prefer to mark this as fails-if(webrender) rather than fuzzy, at least until we can investigate it fully. From the try push it looks like the caps of the scrollthumb are a little off, but it's not obvious to me that it's not fixable.

::: layout/reftests/webm-video/reftest.list
@@ +41,5 @@
>  # particular (non-spec-mandated) video codec.
>  default-preferences test-pref(layout.css.object-fit-and-position.enabled,true) test-pref(gfx.ycbcr.accurate-conversion,true)
> +fails-if(layersGPUAccelerated) skip-if(Android) random-if(webrender) == object-fit-contain-webm-001.html object-fit-contain-webm-001-ref.html # Bug 1083516 for layersGPUAccelerated failures, Bug 1084564 for Android failures
> +fails-if(layersGPUAccelerated) skip-if(Android) random-if(webrender) == object-fit-contain-webm-002.html object-fit-contain-webm-002-ref.html # Bug 1083516 for layersGPUAccelerated failures, Bug 1084564 for Android failures
> +fails-if(layersGPUAccelerated) skip-if(Android) random-if(webrender) == object-fit-cover-webm-001.html object-fit-cover-webm-001-ref.html # Bug 1083516 for layersGPUAccelerated failures, Bug 1084564 for Android failures

Are these actually random-if? From the try push you linked it seems like these pass in non-e10s and continue to fail in e10s. Can we annotate them accordingly? random-if is "worse" because then if starts failing or passing consistently we won't automatically find out.
Attached patch Part4. Set reftest flag. (obsolete) — Splinter Review
Basing on try server result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc25a108a0c82bbb6cb67778d2d29606f739f530&selectedJob=67189781, update the flags.
Attachment #8824913 - Attachment is obsolete: true
Attachment #8824913 - Flags: review?(sotaro.ikeda.g)
Attachment #8825250 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> The patches seem to cause the regression. During scrolling
> http://www.yahoo.co.jp/, I saw a bit of rendering problem.

I still see some small artifacts during scrolling http://www.yahoo.co.jp/ on my linux pc with patches. It might be related to RotatedBuffer.

:ethlin, don't you see the problem on your side?
Flags: needinfo?(ethlin)
(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> (In reply to Sotaro Ikeda [:sotaro] from comment #14)
> > The patches seem to cause the regression. During scrolling
> > http://www.yahoo.co.jp/, I saw a bit of rendering problem.
> 
> I still see some small artifacts during scrolling http://www.yahoo.co.jp/ on
> my linux pc with patches. It might be related to RotatedBuffer.
> 
> :ethlin, don't you see the problem on your side?

I scrolled the page with mouse.
(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> (In reply to Sotaro Ikeda [:sotaro] from comment #24)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #14)
> > > The patches seem to cause the regression. During scrolling
> > > http://www.yahoo.co.jp/, I saw a bit of rendering problem.
> > 
> > I still see some small artifacts during scrolling http://www.yahoo.co.jp/ on
> > my linux pc with patches. It might be related to RotatedBuffer.
> > 
> > :ethlin, don't you see the problem on your side?
> 
> I scrolled the page with mouse.

Oh..I see the problem now. I'll fix the problem first.
Flags: needinfo?(ethlin)
(In reply to Ethan Lin[:ethlin] from comment #26)
> (In reply to Sotaro Ikeda [:sotaro] from comment #25)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #24)
> > > (In reply to Sotaro Ikeda [:sotaro] from comment #14)
> > > > The patches seem to cause the regression. During scrolling
> > > > http://www.yahoo.co.jp/, I saw a bit of rendering problem.
> > > 
> > > I still see some small artifacts during scrolling http://www.yahoo.co.jp/ on
> > > my linux pc with patches. It might be related to RotatedBuffer.
> > > 
> > > :ethlin, don't you see the problem on your side?
> > 
> > I scrolled the page with mouse.
> 
> Oh..I see the problem now. I'll fix the problem first.

Well, I should consider BufferRect for ContentHost. I'll send to try server first to see the result.
I didn't count BufferRect offset for visible region. I add the offset in this patch. The try result are all unexpected-pass now.
I also checked the scrolling action on www.yahoo.co.jp, and it looked also good.
Attachment #8824904 - Attachment is obsolete: true
Attachment #8824904 - Flags: review?(sotaro.ikeda.g)
Attachment #8825652 - Flags: review?(sotaro.ikeda.g)
Attached patch Part4. Remove some fails-if (obsolete) — Splinter Review
Based on the try server result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83fd679fbdc7&selectedJob=67877736, I remove some failed-if flag.
Attachment #8825250 - Attachment is obsolete: true
Attachment #8825250 - Flags: review?(sotaro.ikeda.g)
Attachment #8825653 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8825652 [details] [diff] [review]
Part2. fix bound/format problem after using ContentClient.

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

::: gfx/layers/wr/WebRenderLayerManager.h
@@ +175,5 @@
>  
>    DrawPaintedLayerCallback GetPaintedLayerCallback() const
>    { return mPaintedLayerCallback; }
>  
> +  virtual bool IsCompositingCheap() override { return false; }

Can you explain why is it necessary? It could cause the bad effect to video playback.
Attachment #8825652 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8825652 [details] [diff] [review]
Part2. fix bound/format problem after using ContentClient.

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

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +307,5 @@
> +          visibleRegion.MoveBy(-offset);
> +          visibleRegion.AndWith(IntRect(IntPoint(0,0), dSurf->GetSize()));
> +          visibleRect = visibleRegion.GetBounds().ToUnknownRect();
> +
> +          // XXX Remove it when we can put subimage in WebRender.

Can you create a bug for it?
(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> Comment on attachment 8825652 [details] [diff] [review]
> Part2. fix bound/format problem after using ContentClient.
> 
> Review of attachment 8825652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderLayerManager.h
> @@ +175,5 @@
> >  
> >    DrawPaintedLayerCallback GetPaintedLayerCallback() const
> >    { return mPaintedLayerCallback; }
> >  
> > +  virtual bool IsCompositingCheap() override { return false; }
> 
> Can you explain why is it necessary? It could cause the bad effect to video
> playback.

Opps...I should remove it! Sorry for that.
I'll re-send try to make sure the reftest flags.
Blocks: 1330261
(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> Comment on attachment 8825652 [details] [diff] [review]
> Part2. fix bound/format problem after using ContentClient.
> 
> Review of attachment 8825652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderBridgeParent.cpp
> @@ +307,5 @@
> > +          visibleRegion.MoveBy(-offset);
> > +          visibleRegion.AndWith(IntRect(IntPoint(0,0), dSurf->GetSize()));
> > +          visibleRect = visibleRegion.GetBounds().ToUnknownRect();
> > +
> > +          // XXX Remove it when we can put subimage in WebRender.
> 
> Can you create a bug for it?

Okay! I just created Bug 1330261 for it.
Rebase the patch.
Attachment #8824905 - Attachment is obsolete: true
Address sotaro's comment. I removed the override of IsCompositingCheap in WebRenderLayerManager.
Attachment #8825652 - Attachment is obsolete: true
Attachment #8825715 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8825715 [details] [diff] [review]
Part2. fix bound/format problem after using ContentClient.

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

::: gfx/layers/ipc/WebRenderMessages.ipdlh
@@ +63,5 @@
>    WRImageKey key;
>  };
>  
>  struct OpDPPushExternalImageId {
> +  LayerIntRegion visibleRegion;

name of visibleRegion seems a bit confusing. It actually seems to mean valid buffer region.
Attachment #8825715 - Flags: review?(sotaro.ikeda.g) → review+
try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8385b840f7fc76521170b4a5b7076e88a1de0ced

There is only one unexpected-pass. I'll remove the 'fails-if' flag.
(In reply to Sotaro Ikeda [:sotaro] from comment #38)
> Comment on attachment 8825715 [details] [diff] [review]
> Part2. fix bound/format problem after using ContentClient.
> 
> Review of attachment 8825715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/WebRenderMessages.ipdlh
> @@ +63,5 @@
> >    WRImageKey key;
> >  };
> >  
> >  struct OpDPPushExternalImageId {
> > +  LayerIntRegion visibleRegion;
> 
> name of visibleRegion seems a bit confusing. It actually seems to mean valid
> buffer region.

Okay, I'll rename to validBufferRegion.
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/4c9d48bb926c
Part1. Use ContentClient in WebRenderPaintedLayer. r=sotaro
https://hg.mozilla.org/projects/graphics/rev/693ed93b56db
Part2. Fix bound/format problem after using ContentClient. r=sotaro
https://hg.mozilla.org/projects/graphics/rev/81e3e207072b
Part3. Disable RotationBuffer when enabling WR. r=sotaro
https://hg.mozilla.org/projects/graphics/rev/11b1c9cffb0a
Part4. Remove a fails-if flag after using ContentClient in WebRenderPaintedLayer. r=gfx?
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1330586
These patches may have introduced or exposed some nondeterministic behaviour. There are a couple more "unexpected pass" tests showing up, but intermittently. See https://treeherder.mozilla.org/#/jobs?repo=graphics&revision=11b1c9cffb0ac0b13c35e96f92238ddee06370f8&filter-searchStr=quantum%20reftest-e10s-4&group_state=expanded
I ended up backing this out because of the intermittent failure in https://hg.mozilla.org/projects/graphics/rev/571286200177ae7ddfa1893c6b42853b60f2e81e. I really hate to do this but I think in the long run it's better to investigate this now rather than just marking it random-if. There might be other tests that are affected and will only be discovered later and cause much frustration to other people.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Ethan Lin[:ethlin] from comment #39)
> try server result:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8385b840f7fc76521170b4a5b7076e88a1de0ced
> 
> There is only one unexpected-pass. I'll remove the 'fails-if' flag.

I retriggered these a bunch, and the other failures are showing up in this try push as well. Seems to be restricted to debug-e10s reftests, both accelerated and unaccelerated.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #42)
> These patches may have introduced or exposed some nondeterministic
> behaviour. There are a couple more "unexpected pass" tests showing up, but
> intermittently. See
> https://treeherder.mozilla.org/#/
> jobs?repo=graphics&revision=11b1c9cffb0ac0b13c35e96f92238ddee06370f8&filter-
> searchStr=quantum%20reftest-e10s-4&group_state=expanded

Looks like there are two tests become intermittent unexpected-pass:
layout/reftests/bugs/581317-1.html , layout/reftests/bugs/650228-1.html. I guess it's because the timing of getting snapshot. Some active layer changes to inactive one. And this patch may fix either active or inactive case.
The Ru4 jobs [1] also has layout/reftests/bugs/728983-1.html failing a few times - from the reftest analyzer it looks like the shade of green is slightly different. I don't know how that's happening and if that's a regression from your patch or not. I don't think I've seen it any previous push.

[1] https://treeherder.mozilla.org/#/jobs?repo=graphics&revision=11b1c9cffb0ac0b13c35e96f92238ddee06370f8&filter-searchStr=quantum%20reftest%20e10s&group_state=expanded&selectedJob=68484937
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #46)
> The Ru4 jobs [1] also has layout/reftests/bugs/728983-1.html failing a few
> times - from the reftest analyzer it looks like the shade of green is
> slightly different. I don't know how that's happening and if that's a
> regression from your patch or not. I don't think I've seen it any previous
> push.
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=graphics&revision=11b1c9cffb0ac0b13c35e96f92238ddee06370f8&filter-
> searchStr=quantum%20reftest%20e10s&group_state=expanded&selectedJob=68484937

Right, these three tests are all about opacity changing. I think these failures happen now is because the patches in this bug change the timing. I'll keep investigating.
I think I found the problem. At least for 581317-1.html, 650228-1.html, we don't handle color layer correctly, so basically the testcase will fail. But after the testcase stops changing the opacity of the color layer, the color layer will be merged to a painted layer (because it's an inactive layer). So it's all about the timing of snapshot. I think 728983-1.html should be the similar problem, just reverse.
For now color layer has two problems: position and opacity. I'll create bugs to fix color layer. kats, can I mark these three testcase to 'random-if' first? And I will remove the 'random-if' in the following bugs.
Flags: needinfo?(bugmail)
See Also: → 1330921
See Also: → 1330945
(In reply to Ethan Lin[:ethlin] from comment #48)
> For now color layer has two problems: position and opacity. I'll create bugs
> to fix color layer. kats, can I mark these three testcase to 'random-if'
> first? And I will remove the 'random-if' in the following bugs.

Ok, as long as we understand the problem I think it's ok to mark them random-if. Thanks for looking into it!
Flags: needinfo?(bugmail)
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/7c573aa43807
Part1. Use ContentClient in WebRenderPaintedLayer. r=sotaro
https://hg.mozilla.org/projects/graphics/rev/d6424c3c807d
Part2. Fix bound/format problem after using ContentClient. r=sotaro
https://hg.mozilla.org/projects/graphics/rev/903355a64373
Part3. Disable RotationBuffer when enabling WR. r=sotaro
https://hg.mozilla.org/projects/graphics/rev/7b90b2b7960e
Part4. Remove a fails-if flag and set some random-if after using ContentClient in WebRenderPaintedLayer. r=gfx?
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
See Also: → 1337885
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.