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

RESOLVED FIXED in mozilla35

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: BenWa, Assigned: sotaro)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla35
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

Attachments

(2 attachments, 10 obsolete attachments)

1.60 KB, patch
Details | Diff | Splinter Review
27.80 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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).
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.
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.
(Reporter)

Comment 3

4 years ago
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

4 years ago
Dying to hear the continuation of the story.
(Reporter)

Comment 5

4 years ago
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.
Summary: [B2G] Image layers are much slower to composite then Thebes layers → [B2G] Sharing GrallocTextureSourceOGL degrades compositing performance, recreates EGLImage several times a frame
(Reporter)

Comment 6

4 years ago
Created attachment 8431173 [details] [diff] [review]
work around (probably buggy)
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
Created attachment 8432575 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Comment 9

4 years ago
Nominate to "b2g-v2.1+". This bug blocks "b2g-v2.1+".
blocking-b2g: --- → 2.1?
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 10

4 years ago
Created attachment 8473703 [details] [diff] [review]
wip patch - Handle Sharing GrallocTextureSourceOGL case

This is a wip patch. Add TextureHost::RegisterConsumer() and code clean up.
Attachment #8432575 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Created attachment 8473776 [details] [diff] [review]
patch - Handle Sharing GrallocTextureSourceOGL

Fix some nits.
Attachment #8473703 - Attachment is obsolete: true
Let's target this for 2.1/35 and ask for the uplift once we have the fix.
blocking-b2g: 2.1? → backlog
Blocks: 1062395
Blocks: 1072381
(Assignee)

Comment 13

3 years ago
Created attachment 8495280 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

organize the change.
Attachment #8473776 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8495284 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Upload correct patch.
Attachment #8495280 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Created attachment 8495376 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts
Attachment #8495284 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Created attachment 8495387 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Fix build failure.
Attachment #8495376 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Created attachment 8495430 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Fix ImageHost destructor.
Attachment #8495387 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8495430 - Flags: review?(nical.bugzilla)
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
Attachment #8495430 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Comment 19

3 years ago
(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 :-)
(Assignee)

Comment 20

3 years ago
(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.
(Assignee)

Comment 21

3 years ago
Created attachment 8495549 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Apply the comments.
Attachment #8495430 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8495633 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Fix a build failure. Carry "r=nical".
Attachment #8495549 - Attachment is obsolete: true
Attachment #8495633 - Flags: review+
(Assignee)

Comment 23

3 years ago
Created attachment 8495641 [details] [diff] [review]
patch - Handle Sharing GrallocTextureHostOGL among ImageHosts

Remove incorrect assert checks. Carry "r=nical".
Attachment #8495633 - Attachment is obsolete: true
Attachment #8495641 - Flags: review+
(Assignee)

Updated

3 years ago
Blocks: 1010966
https://hg.mozilla.org/mozilla-central/rev/f678cf7909eb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.