1017351 - [B2G] Sharing GrallocTextureSourceOGL degrades compositing performance, recreates EGLImage several times a frame

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

We currently have an optimization to promote single image display items from a thebes layer into an image layer to avoid copies. But they are much slower to composite on b2g.

They should be just as cheap to composite as Thebes layers. If they aren't for a good reason we should avoid doing the promotion.

In the testcase from bug 1015984, our avg frame time with image layers is 50ms, with thebes it’s 30ms (disabled thebes->image layers conversion).

Comment 1

[Matt Woodrow (:mattwoodrow)]

I think we need more information about the ImageLayers here. In particular, how much downscaling or upscaling is happening during composition?

ImageLayers are uploaded at the original image resolution, not the destination size.

This can be a big win if we're invalidating the image frequently, since downscaling on the GPU is much faster than on the CPU. Conversely though, if we're trading a one-off CPU scale for GPU scaling every frame then maybe we're worse off.

There's a lot of things that we could do here:

* Don't optimize to an ImageLayer if we think an image is unlikely to be invalidated (not sure how to write that heuristic). That will give us a slow first frame, but improve the average.

* Have the compositor draw to an intermediate surface and cache the downscaled result so we only pay the scaling cost once.

* Optimize to an ImageLayer, and notify imglib to asynchronously work on a high quality downscaled copy. Once that is ready, swap out the Image to the new one of the correct size.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Note that the majority of my comment was based on the assumption that ImageLayers are slower because of downscaling (requires a lot more texture samples etc). We should confirm that before doing too much.

Comment 3

[Benoit Girard (:BenWa)]

They are not being downscale. They are in fact being upscale. See the testcase in bug 1015984. The profile is at the office so I don't have it handy. It looked like a bunch of eglCreateImage type stuff. I wouldn't be surprised if it's optimized for video image layers where it's fine to recreate stuff every frame. We probably just need to do less work when drawing the same image layer over and over, but I'm just guessing. I'll look at this tomorrow.

Comment 4

[Andreas Gal :gal]

Dying to hear the continuation of the story.

Comment 5

[Benoit Girard (:BenWa)]

Ahh, found it. We have code to share the same GrallocTextureSourceOGL. The problem is the ImageHost that share the same GrallocTextureSourceOGL have their own CompositableDataGonkOGL which are different isntances but point to the same data.

This comparisons leads to a reset causing us to recreate the same resources several time per composite:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/GrallocTextureHost.cpp#221

I modified the test case to use the same number of layers but use different images and the performance problem goes away.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: work around (probably buggy)

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Benoit Girard (:BenWa) from comment #6)
> Created attachment 8431173 [details] [diff] [review]
> work around (probably buggy)

I have another idea about this problem. I am going to create a patch of it.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Handle Sharing GrallocTextureSourceOGL case

This is wip patch. My implementation ides like like this patch. When TextureHost is used by multiple CompositableHost, OpenGL texture ownership move to TextureHost.

Comment 9

[Sotaro Ikeda [:sotaro]]

Nominate to "b2g-v2.1+". This bug blocks "b2g-v2.1+".

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Handle Sharing GrallocTextureSourceOGL case

This is a wip patch. Add TextureHost::RegisterConsumer() and code clean up.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureSourceOGL

Fix some nits.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's target this for 2.1/35 and ask for the uplift once we have the fix.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

organize the change.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Upload correct patch.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Fix build failure.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Fix ImageHost destructor.

Comment 18

[Nicolas Silva [:nical]]

Comment on attachment 8495430 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

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

This is a complex solution to a tricky problem. I think that we can find a simpler way in the long run which will require some bigger changes. Our gralloc code is really piling complexity up and we should really clean it up eventually (I consider that I have contributed to a lot of the complexity myself, so this is not a statement about this patch in particular, I am just saying that writing and reviewing code in this part of the code is getting harder and harder).
r=me with mCompositableBackends renamed (see comment).

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +488,5 @@
> +    CompositableDataGonkOGL* backend = static_cast<CompositableDataGonkOGL*>(mCompositableBackendData.get());
> +    mTextureBackendSpecificData = backend->GetTextureBackendSpecificData();
> +  }
> +
> +  // If TextureHost shareing by multiple CompositableHosts are detected,

nit: sharing

@@ +512,5 @@
> +    // Handle a case that TextureHost has ownership of TextureSharedDataGonkOGL.
> +    MOZ_ASSERT(aBackendData->IsAllowingSharingTextureHost());
> +    (*mCompositableBackends)[aBackendData->GetId()] = aBackendData;
> +    if (mCompositableBackends->size() > 200) {
> +      NS_WARNING("Too much CompositableBackends");

nit: too many CompositableBackendDatas

::: gfx/layers/opengl/GrallocTextureHost.h
@@ +143,5 @@
>    RefPtr<GrallocTextureSourceOGL> mTextureSource;
>    gfx::IntSize mSize; // See comment in textureClientOGL.h
> +
> +  RefPtr<TextureSharedDataGonkOGL> mTextureBackendSpecificData;
> +  UniquePtr<std::map<uint64_t, RefPtr<CompositableBackendSpecificData> > > mCompositableBackends;

This name confused me when reading the cpp code because it sounds like it is a list of backend types (like enums). Please rename this into something like
* mCompositableBackendDatas
* mBackendDatas
* ... anything that ends with BackendData or BackendSpecificData rather than just Backend

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +203,5 @@
> +  MOZ_ASSERT(GetAllowSharingTextureHost());
> +
> +  if (IsEGLImageBound(aImage))
> +  {
> +    // If EGLImage is already bounded to OpenGL Texture,

nit: already bound to

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #18)
> Comment on attachment 8495430 [details] [diff] [review]
> patch - Handle Sharing GrallocTextureHostOGL among ImageHosts
> 
> Review of attachment 8495430 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a complex solution to a tricky problem. I think that we can find a
> simpler way in the long run which will require some bigger changes. Our
> gralloc code is really piling complexity up and we should really clean it up
> eventually (I consider that I have contributed to a lot of the complexity
> myself, so this is not a statement about this patch in particular, I am just
> saying that writing and reviewing code in this part of the code is getting
> harder and harder).
> r=me with mCompositableBackends renamed (see comment).

I agree. But we need to add one more tricky thing by bug 1010966 :-)

Comment 20

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #18)
> This name confused me when reading the cpp code because it sounds like it is
> a list of backend types (like enums). Please rename this into something like
> * mCompositableBackendDatas
> * mBackendDatas
> * ... anything that ends with BackendData or BackendSpecificData rather than
> just Backend
> 

mBackendDatas seens better. There is already mCompositableBackendData.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Apply the comments.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Fix a build failure. Carry "r=nical".

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Remove incorrect assert checks. Carry "r=nical".

Comment 24

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f678cf7909eb

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f678cf7909eb