Closed Bug 1017351 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
tracking-b2g backlog

People

(Reporter: BenWa, Assigned: sotaro)

References

Details

Attachments

(2 files, 10 obsolete files)

1.60 KB, patch
Details | Diff | Splinter Review
27.80 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
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.
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.
Dying to hear the continuation of the story.
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
(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.
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: nobody → sotaro.ikeda.g
Nominate to "b2g-v2.1+". This bug blocks "b2g-v2.1+".
blocking-b2g: --- → 2.1?
Status: NEW → ASSIGNED
This is a wip patch. Add TextureHost::RegisterConsumer() and code clean up.
Attachment #8432575 - Attachment is obsolete: true
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
organize the change.
Attachment #8473776 - Attachment is obsolete: true
Upload correct patch.
Attachment #8495280 - Attachment is obsolete: true
Attachment #8495284 - Attachment is obsolete: true
Fix build failure.
Attachment #8495376 - Attachment is obsolete: true
Fix ImageHost destructor.
Attachment #8495387 - Attachment is obsolete: true
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+
(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 :-)
(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.
Apply the comments.
Attachment #8495430 - Attachment is obsolete: true
Fix a build failure. Carry "r=nical".
Attachment #8495549 - Attachment is obsolete: true
Attachment #8495633 - Flags: review+
Remove incorrect assert checks. Carry "r=nical".
Attachment #8495633 - Attachment is obsolete: true
Attachment #8495641 - Flags: review+
Blocks: 1010966
https://hg.mozilla.org/mozilla-central/rev/f678cf7909eb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.