979489 - (can-tiles) Implement SimpleTiledContentClient

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

SimpleTiledContentClient is a variation on TiledContentClient that doesn't use buffer re-use strategies.

Comment 1

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=1d5c8ea656d4

Comment 2

[Benoit Girard (:BenWa)]

Attachment: WIP

Comment 3

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=9c36d78172ff

Comment 4

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=7ad3350983d8

Comment 5

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=f7becba585d3

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Latest push and latest patches: https://tbpl.mozilla.org/?tree=Try&rev=afc1b6c4b11d

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: WIP, v2 (on top of euro tiles)

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

With unified fix:

https://tbpl.mozilla.org/?tree=Try&rev=5e77ed9fe477

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Android R2 failure:

border-image/border-image-element.html | image comparison (==), max difference: 125, number of differing pixels: 5808

This test compares   { border-image: -moz-element:(#src); } <img id="src" src="diamonds.png">   and { border-image: url("diamonds.png"); }.   The one using -moz-element seems to be using NEAREST when scaled up, and the second seems to be using LINEAR.  There is maybe relevant code:

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp#1117

Or something else that isn't setting LINEAR properly..

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Simple Tiles

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Latest patch: https://gist.github.com/vvuk/9471359

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Simple tiles v4

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8388660 [details] [diff] [review]
Simple tiles v4

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

This is not really much of a review as this will be off when it lands and getting the testing soon is very valuable. We should review this more once it is in place. We should be able to come kind of consensus about the pooling.

::: gfx/layers/client/SimpleTextureClientPool.cpp
@@ +31,5 @@
> +/* static */ void
> +SimpleTextureClientPool::RecycleCallback(TextureClient* aClient, void* aClosure)
> +{
> +  SimpleTextureClientPool* pool =
> +    reinterpret_cast<SimpleTextureClientPool*>(aClosure);

static_cast<SimpleTextureClientPool*>

::: gfx/layers/client/SimpleTextureClientPool.h
@@ +76,5 @@
> +  //     if we see that, then we should use push_back() to add new elements
> +  //   when we shrink this list, we use pop(), but should use pop_back() to
> +  //   nuke the oldest.
> +  // We may need to switch to a std::deque
> +  std::stack<RefPtr<TextureClient> > mTextureClients;

mAvailableTextureClients

@@ +77,5 @@
> +  //   when we shrink this list, we use pop(), but should use pop_back() to
> +  //   nuke the oldest.
> +  // We may need to switch to a std::deque
> +  std::stack<RefPtr<TextureClient> > mTextureClients;
> +  std::list<RefPtr<TextureClient> > mAutoRecycle;

mOutstandingTextureClients

::: gfx/layers/client/SimpleTiledContentClient.cpp
@@ +81,5 @@
> +  bool fullPaint = false;
> +
> +  RefPtr<TextureClient> textureClient = mManager->GetSimpleTileTexturePool(tileFormat)->GetTextureClientWithAutoRecycle();
> +  //RefPtr<TextureClient> textureClient = mManager->GetTexturePool(tileFormat)->GetTextureClient();
> +  //mManager->GetTexturePool(tileFormat)->AutoRecycle(textureClient);

Comments slipped in

Comment 14

[Benoit Girard (:BenWa)]

Attachment: review comments

Please merge with tiling and flag as obsolete.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8388753 [details] [diff] [review]
review comments

got it merged in, thanks

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Landed with fixes: https://hg.mozilla.org/integration/mozilla-inbound/rev/73d02445dc1c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73d02445dc1c

Comment 18

[Benoit Girard (:BenWa)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> Android R2 failure:
> 
> border-image/border-image-element.html | image comparison (==), max
> difference: 125, number of differing pixels: 5808
> 
> This test compares   { border-image: -moz-element:(#src); } <img id="src"
> src="diamonds.png">   and { border-image: url("diamonds.png"); }.   The one
> using -moz-element seems to be using NEAREST when scaled up, and the second
> seems to be using LINEAR.  There is maybe relevant code:
> 
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/
> CompositorOGL.cpp#1117
> 
> Or something else that isn't setting LINEAR properly..

I haven't been able to reproduce this test locally on my GN

Comment 19

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=fdcbd12f7e96

Comment 20

[Benoit Girard (:BenWa)]

Ruling out difference in doBufferedDrawing path (It doesn't use the CreateOffscreen call like euro tiles)
https://tbpl.mozilla.org/?tree=Try&rev=291fe9a234ab

Comment 21

[Jeff Muizelaar [:jrmuizel]]

The background-image-element.html test passes if you comment out the filter change here
http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxDrawable.cpp#156

Comment 22

[Jeff Muizelaar [:jrmuizel]]

I'm not sure why.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Turns out the background-image-element.html was already mostly failing the same way before and after. Adding one pixel of fuzz makes it go away.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/20d7e584a25d

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Weird. I don't really like that that test has that much fuzz. Makes it not really useful as a reftest, IMO.