862952 - Composer2D never called to render after layers refactoring

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Diego Wilson [:diego]]

The recent layer refactoring in bug 825928 broke the Composer2D support (added in bug 804852).

Composer2D was hooked into LayerManagerOGL. Apparently after the refactoring gecko no longer uses LayerManagerOGL for OpenGL composition.

The hook may need to be added to the new OGL composition component(s) to carry this feature forward.

Comment 1

[Nick Cameron [:nrc]]

And I tried so hard to keep this working... :-(

Comment 2

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #0)
> The recent layer refactoring in bug 825928 broke the Composer2D support
> (added in bug 804852).
> 
> Composer2D was hooked into LayerManagerOGL. Apparently after the refactoring
> gecko no longer uses LayerManagerOGL for OpenGL composition.
> 
We do not, we now use a platform independent layer manager backed by a compositor, in this case CompositorOGL.

> The hook may need to be added to the new OGL composition component(s) to
> carry this feature forward.

The hook is still there, but it is probably broken. I'll investigate.

Comment 3

[Nick Cameron [:nrc]]

Attachment: WIP patch

Does this fix the problem? I couldn't find anything else wrong.

We never use the offset stuff in LayerRenderState, were you expecting that to ever be present?

Comment 4

[Diego Wilson [:diego]]

Hi Nick!

(In reply to Nick Cameron [:nrc] from comment #3)
> And I tried so hard to keep this working... :-(

Yeah, I can see that. We're not too far off though. Just some tweaks and we'll be there :)

> We never use the offset stuff in LayerRenderState, were you expecting that
> to ever be present?

https://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#196
^^^^ I think its used at least here

https://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#93
^^^ GetComposer2D needs to happen after mCompositor->Initialize(). With this change the render calls are coming through

https://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#303
^^^ Apparently HwcComposer2D can no longer assume OGL Layers

Are you able to test out HwcComposer2D?

Comment 5

[Nick Cameron [:nrc]]

Attachment: WIP patch v2

Comment 6

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #4)
> Hi Nick!
> 
> > We never use the offset stuff in LayerRenderState, were you expecting that
> > to ever be present?
> 
> https://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/
> ThebesLayerOGL.cpp#196
> ^^^^ I think its used at least here
> 
Yeah, so we had lost that, added now.

> https://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> LayerManagerComposite.cpp#93
> ^^^ GetComposer2D needs to happen after mCompositor->Initialize(). With this
> change the render calls are coming through
> 
> https://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.
> cpp#303
> ^^^ Apparently HwcComposer2D can no longer assume OGL Layers
> 

Both changes in the patch.

> Are you able to test out HwcComposer2D?

I can, but not very easily. If this works for you then we can get review and land it. If not, then I try and get my FxOS setup going again and dive in

Comment 7

[Diego Wilson [:diego]]

Comment on attachment 738835 [details] [diff] [review]
WIP patch v2

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

::: gfx/layers/opengl/TextureHostOGL.h
@@ +601,5 @@
>    }
>  
> +  virtual LayerRenderState GetRenderState() MOZ_OVERRIDE
> +  {
> +    return LayerRenderState(mGraphicBuffer,

mGraphicBuffer.get()

::: widget/gonk/HwcComposer2D.cpp
@@ +17,5 @@
>  #include <android/log.h>
>  
>  #include "Framebuffer.h"
>  #include "HwcComposer2D.h"
> +#include "mozilla/layers/LayerManagerComposite.h"

#include "LayerManagerComposite.h"

@@ +299,5 @@
>          }
>          return true;
>      }
>  
> +    LayerComposite* hostLayer = static_cast<LayerComposite*>(aLayer->ImplData());

aLayer->AsLayerComposite() is safer

@@ +300,5 @@
>          return true;
>      }
>  
> +    LayerComposite* hostLayer = static_cast<LayerComposite*>(aLayer->ImplData());
> +    LayerRenderState state = hostLayer->GetRenderState();

This method is missing from LayerComposite

@@ +388,5 @@
>          region.rects = &(hwcLayer.displayFrame);
>          hwcLayer.visibleRegionScreen = region;
>      } else {
>          hwcLayer.flags |= HWC_COLOR_FILL;
> +        ColorLayer* colorLayer = static_cast<ColorLayer*>(hostLayer->GetLayer());

aLayer->AsColorLayer() is safer

Comment 8

[Diego Wilson [:diego]]

https://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.h#81

When I open the camera app it crashes at this point because mTextureHost is null. Even if I return LayerRenderState(), it never returns a valid graphic buffer. Something may be missing from ImageLayerComposite

Comment 9

[Diego Wilson [:diego]]

Also, I'm generally curious. What's the reason for the refactoring?

Comment 10

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #9)
> Also, I'm generally curious. What's the reason for the refactoring?

To make it easy to implement OMTC on more platforms, pre-refactoring we would have to write the entire layers backend from scratch for a new platform, now a lot of code is shared which means it is much easier to implement OMTC for other platforms.

Comment 11

[Nick Cameron [:nrc]]

Attachment: patch

Comment 12

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #8)
> https://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> ImageHost.h#81
> 
> When I open the camera app it crashes at this point because mTextureHost is
> null. Even if I return LayerRenderState(), it never returns a valid graphic
> buffer. Something may be missing from ImageLayerComposite

I think the code is right when mTextureHost is non-null. This patch does that, I am hopeful. ImageLayerComposite looks right, I think (although I improved a nit).

Comment 13

[Diego Wilson [:diego]]

No crash, so things look better. But it still always fails to retrieve a graphic buffer from ShadowImageLayer. Any suggestion on where to debug?

Comment 14

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #13)
> No crash, so things look better. But it still always fails to retrieve a
> graphic buffer from ShadowImageLayer. Any suggestion on where to debug?

I wonder if this is related to bug 864017, since we will only a valid LayerRenderState if we have a gralloc texture host, and bug 864017 causes that not to happen for some images.

If you break in ImageLayerComposite::GetRenderState and follow the calls down, you should see why we don't get a valid LayerRenderState. It is important to note the kind of image host and texture host - I would expect either an ImageHostSingle or ImageHostBuffered which would own a GrallocTextureHostOGL. The GetRenderState calls should trickle down to GrallocTextureHostOGL::GetRenderState, which should return the LayerRenderState with a correct graphic buffer, that should then end up back at the hwcComposer.

Comment 15

[Peter Chang[:pchang]]

With WIP v3 and latest m-c, camera goes to HWC with colorfill enabled.

I guess something was fixed in latest m-c. :-)

Comment 16

[Diego Wilson [:diego]]

Attachment: Camera to camcorder crash stack

Comment 17

[Diego Wilson [:diego]]

Better! I can see HWC rendering now works in the camera as pchang remarks

One last hiccup. When I switch from camera to camcorder I see a crash. I attached the stack.

Comment 18

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #16)
> Created attachment 740533 [details]
> Camera to camcorder crash stack

And this doesn't happen without the patch?

Looks like it must be a lifetime issue for the GraphicBuffer - nothing else significant changes. Could you change LayerRenderState::mSurface from android::GraphicBuffer* to android::sp and see if that prevents the crash? (I thought the texture host should stay alive until the end of the transaction, but perhaps it does not).

Comment 19

[Diego Wilson [:diego]]

Comment on attachment 740533 [details]
Camera to camcorder crash stack

I can reproduce this issue before the patch. I submitted bug 864598 to track that.

Comment 20

[Diego Wilson [:diego]]

Comment on attachment 739301 [details] [diff] [review]
patch

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

Looks good to me!

Comment 21

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #20)
> Comment on attachment 739301 [details] [diff] [review]
> patch
> 
> Review of attachment 739301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me!

Awesome, thanks for the help debugging this!

Comment 22

[Andreas Gal :gal]

Thanks for jumping on this nrc. Much appreciated!

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 739301 [details] [diff] [review]
patch

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

I have to cancel this review and ask a few questions. I hadn't seen this code before so I may not be the right reviewer.

::: gfx/layers/LayersTypes.h
@@ +56,5 @@
>    Mask3d,         // mask layer for layers with 3D transforms
>    NumMaskTypes
>  };
>  
>  // LayerRenderState for Composer2D

If LayerRenderState is only for Composer2D then 1) isn't it a misnomer? 2) does it belong here? Let's not reproduce the mistakes of old-layers...

This class also lacks any documentation, which I would need to make a meaningful review. What's mOffset?

@@ +80,5 @@
>  
>    bool BufferRotated() const
>    { return mFlags & LAYER_RENDER_STATE_BUFFER_ROTATION; }
>  
> +  android::GraphicBuffer* mSurface;

Why is this a raw pointer? Who keeps it alive?

::: widget/gonk/HwcComposer2D.cpp
@@ +325,5 @@
>              return false;
>          }
>      }
>  
> +    sp<GraphicBuffer> buffer = fillColor ? nullptr : state.mSurface;

Just below, you have an if(fillColor) that looks like the right existing place for this check.

Comment 24

[Nick Cameron [:nrc]]

(In reply to Benoit Jacob [:bjacob] from comment #23)
> Comment on attachment 739301 [details] [diff] [review]
> patch
> 
> Review of attachment 739301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have to cancel this review and ask a few questions. I hadn't seen this
> code before so I may not be the right reviewer.

I don't think anyone on the graphics side except me has spent much time with this, and I haven't spent much. I'm putting you as the reviewer for pretty much anything with gralloc in the patch :-)

> 
> ::: gfx/layers/LayersTypes.h
> @@ +56,5 @@
> >    Mask3d,         // mask layer for layers with 3D transforms
> >    NumMaskTypes
> >  };
> >  
> >  // LayerRenderState for Composer2D
> 
> If LayerRenderState is only for Composer2D then 1) isn't it a misnomer? 2)
> does it belong here? Let's not reproduce the mistakes of old-layers...
> 

It is only for Composer2D (the API) but not only for hwcComposer (this particular, and currently only, implementation). It contains the state of rendering a layer, so I'm not sure there is a better name, and I'm not sure where else I would put it.

> This class also lacks any documentation, which I would need to make a
> meaningful review. What's mOffset?

It's the offset within the texture to the logical origin.

> 
> @@ +80,5 @@
> >  
> >    bool BufferRotated() const
> >    { return mFlags & LAYER_RENDER_STATE_BUFFER_ROTATION; }
> >  
> > +  android::GraphicBuffer* mSurface;
> 
> Why is this a raw pointer? Who keeps it alive?
> 

LayerRenderState should never outlast the layers they refer to, but this could probably be a strong pointer to be safe.

> ::: widget/gonk/HwcComposer2D.cpp
> @@ +325,5 @@
> >              return false;
> >          }
> >      }
> >  
> > +    sp<GraphicBuffer> buffer = fillColor ? nullptr : state.mSurface;
> 
> Just below, you have an if(fillColor) that looks like the right existing
> place for this check.

OK

I'll add some comments and make these changes, patch coming up...

Comment 25

[Nick Cameron [:nrc]]

Attachment: patch

Comment 26

[Diego Wilson [:diego]]

Comment on attachment 741033 [details] [diff] [review]
patch

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

::: gfx/layers/LayersTypes.h
@@ +68,5 @@
>  struct LayerRenderState {
>    LayerRenderState() : mSurface(nullptr), mFlags(0), mHasOwnOffset(false)
>    {}
>  
> +  LayerRenderState(android::GraphicBuffer* aSurface, uint32_t aFlags)

Looks like we'll need the SurfaceDescriptor after all! It appears the dimensions in the gralloc buffer are not quite right, and the correct dimensions are in the SurfaceDescriptor. See bug 850566 and bug 865112

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Diego Wilson [:diego] from comment #26)
> Looks like we'll need the SurfaceDescriptor after all! It appears the
> dimensions in the gralloc buffer are not quite right, and the correct
> dimensions are in the SurfaceDescriptor. See bug 850566 and bug 865112

As we're currently experiencing in bug 862324, SurfaceDescriptors have raw pointers to PGrallocBufferParent's that can become dangling pointers at any time, causing crashes. Please do not use SurfaceDescriptors in any more places outside of IPDL-related code where you actually know that the actors are not already gone!

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Let me try to expand what I mean here.

On certain IPC events such as "channel errors", typically happening in the main process when a content process crashes, all the PGrallocBufferParents get destroyed. That's because they are IPDL managed objects.

SurfaceDescriptor's have raw pointers to PGrallocBufferParent's. When such events occur, all SurfaceDescriptor's typically become bad: their raw PGrallocBufferParent* pointers become dangling.

This means that the main process should not have SurfaceDescriptor's whose lifetime can extend across receiving IPC message. It's OK to work with SurfaceDescriptor's but not OK to keep them long-term, where long-term means "the next IPC event".

From talking with :sotaro it seems that the SurfaceDescritor here is short-lived so that you should plausibly be safe.

If you ever need to keep gralloc surfaces long-term on one side of the IPC divide in mozilla-central code, the right type is android::GraphicBuffer.

I hope that that was of any interest and I didn't just waste your time :-)

Comment 29

[Diego Wilson [:diego]]

OK, looks like we get away with it then :)

:nrc can we get the SurfaceDescriptor back in LayerRenderState?

Comment 30

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #29)
> OK, looks like we get away with it then :)
> 
> :nrc can we get the SurfaceDescriptor back in LayerRenderState?

I would prefer to extract the dimensions in the texture host and add them to LayerRenderState along with the gralloc buffer. Otherwise, this whole setup just becomes a ticking time bomb when for some reason the lifetimes of one or the other thing change. Would that work for you?

Comment 31

[Diego Wilson [:diego]]

SGTM as long as said dimensions are valid the same way the dimensions in SurfaceDecriptorGralloc are. See bug 850566

Comment 32

[Nick Cameron [:nrc]]

Attachment: patch with SurfaceDescriptor's size

Comment 33

[Diego Wilson [:diego]]

Waiting for bug 832383 will smooth the landing

Comment 34

[Diego Wilson [:diego]]

Comment on attachment 742859 [details] [diff] [review]
patch with SurfaceDescriptor's size

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

::: widget/gonk/HwcComposer2D.cpp
@@ +333,5 @@
>          bufferRect = nsIntRect(visibleRect);
>      } else {
>          if(state.mHasOwnOffset) {
>              bufferRect = nsIntRect(state.mOffset.x, state.mOffset.y,
> +                                   state.mSize.width, state.mSize.height);

This change and the one below is already included in a patch in bug 832383. It'll be easier to wait for it to land there since the patches in that bug will be uplifted to v1 and v1-train while this patch is for m-c only.

Otherwise the patch works fine for me

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 742859 [details] [diff] [review]
patch with SurfaceDescriptor's size

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

r=me; keep in mind that I'm not a competent reviewer for composer2D stuff, I looked specifically at the gralloc-facing and TextureHost-facing aspects here.

::: gfx/layers/composite/TextureHost.h
@@ -230,2 @@
>    {
> -    return LayerRenderState(mBuffer,

Please comment on why you don't need to check for null mBuffer pointers and for invalid *mBuffer surfacedescriptor here.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +770,5 @@
> +LayerRenderState
> +GrallocTextureHostOGL::GetRenderState()
> +{
> +  return LayerRenderState(mGraphicBuffer.get(),
> +                          mBuffer->get_SurfaceDescriptorGralloc().size(),

Please comment on why you don't need to check for null mBuffer pointers and for invalid *mBuffer surfacedescriptor here.

Comment 36

[Nick Cameron [:nrc]]

(In reply to Benoit Jacob [:bjacob] from comment #35)
> Comment on attachment 742859 [details] [diff] [review]
> patch with SurfaceDescriptor's size
> 
> Review of attachment 742859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me; keep in mind that I'm not a competent reviewer for composer2D stuff, I
> looked specifically at the gralloc-facing and TextureHost-facing aspects
> here.
> 
> ::: gfx/layers/composite/TextureHost.h
> @@ -230,2 @@
> >    {
> > -    return LayerRenderState(mBuffer,
> 
> Please comment on why you don't need to check for null mBuffer pointers and
> for invalid *mBuffer surfacedescriptor here.

I'm just removing code here, so I don't think it needs a comment.
> 
> ::: gfx/layers/opengl/TextureHostOGL.cpp
> @@ +770,5 @@
> > +LayerRenderState
> > +GrallocTextureHostOGL::GetRenderState()
> > +{
> > +  return LayerRenderState(mGraphicBuffer.get(),
> > +                          mBuffer->get_SurfaceDescriptorGralloc().size(),
> 
> Please comment on why you don't need to check for null mBuffer pointers and
> for invalid *mBuffer surfacedescriptor here.

Added checks rather than a comment.

Comment 37

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #34)
> Comment on attachment 742859 [details] [diff] [review]
> patch with SurfaceDescriptor's size
> 
> Review of attachment 742859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +333,5 @@
> >          bufferRect = nsIntRect(visibleRect);
> >      } else {
> >          if(state.mHasOwnOffset) {
> >              bufferRect = nsIntRect(state.mOffset.x, state.mOffset.y,
> > +                                   state.mSize.width, state.mSize.height);
> 
> This change and the one below is already included in a patch in bug 832383.
> It'll be easier to wait for it to land there since the patches in that bug
> will be uplifted to v1 and v1-train while this patch is for m-c only.
> 
> Otherwise the patch works fine for me

OK, it looks trivial to rebase this patch on top of those, but I won't do it now because it looks like those will change before landing. But, I will go on PTO for a few weeks from Wednesday, so is it OK for you to rebase and land this once 832383 lands? Otherwise I will do it when I return.

Comment 38

[Nick Cameron [:nrc]]

Attachment: patch

Updated patch with bjacob's comments. Carrying r=bjacob,f=diego

Comment 39

[Diego Wilson [:diego]]

I tested the patch again. It's now always failing to retrieve the surface descriptor. There was a batch of major changes to layers about a week ago so it may have to do something with that.

I'll see if I can figure out what's going on.

Comment 41

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #39)
> I tested the patch again. It's now always failing to retrieve the surface
> descriptor. There was a batch of major changes to layers about a week ago so
> it may have to do something with that.
> 
> I'll see if I can figure out what's going on.

Did you get any further with this?

I'll take a look this week.

Comment 42

[Diego Wilson [:diego]]

(In reply to Nick Cameron [:nrc] from comment #41)
> Did you get any further with this?
Nope, sorry

On the plus side, all other HWC patches have landed

Comment 43

[Nick Cameron [:nrc]]

Attachment: rebased patch

Rebased. Carrying r=bjacob,f=diego

Comment 44

[Nick Cameron [:nrc]]

Other than quite a lot of rebasing work, I couldn't see anything obviously wrong here. Just waiting for a b2g build and I'll try and test it.

Comment 45

[Nick Cameron [:nrc]]

Hmm, well testing was 100% unsuccessful.

Diego: should we hit Composer2D/Hwc paths on a standard Unagi? Or do I need to flick a pref and/or use some fancy kernel?

When should we hit the Composer2D path? That is what do I need to do in the FfOS UI to trigger Composer2D stuff?

Thanks!

Comment 46

[Peter Chang[:pchang]]

(In reply to Nick Cameron [:nrc] from comment #45)
> Hmm, well testing was 100% unsuccessful.
> 
> Diego: should we hit Composer2D/Hwc paths on a standard Unagi? Or do I need
> to flick a pref and/or use some fancy kernel?
> 
> When should we hit the Composer2D path? That is what do I need to do in the
> FfOS UI to trigger Composer2D stuff?
> 
> Thanks!

I think we didn't run hwc on unagi device because it didn't support colorfill feature that was required for hwc.

You need the leo or buri device to verify it.
By the way, I just verified your patch on leo device. Both camera/video used hwc for composition.

Comment 47

[Nick Cameron [:nrc]]

Comment on attachment 758339 [details] [diff] [review]
rebased patch

(In reply to peter chang[:pchang] from comment #46)
> (In reply to Nick Cameron [:nrc] from comment #45)
> > Hmm, well testing was 100% unsuccessful.
> > 
> > Diego: should we hit Composer2D/Hwc paths on a standard Unagi? Or do I need
> > to flick a pref and/or use some fancy kernel?
> > 
> > When should we hit the Composer2D path? That is what do I need to do in the
> > FfOS UI to trigger Composer2D stuff?
> > 
> > Thanks!
> 
> I think we didn't run hwc on unagi device because it didn't support
> colorfill feature that was required for hwc.
> 
> You need the leo or buri device to verify it.
> By the way, I just verified your patch on leo device. Both camera/video used
> hwc for composition.

Well that is bad news and good news :-) If it works, lets ship it! No need for me to test. Lets double check with diego first because he found errors before. If it works for him, I'll land this thing.

Comment 48

[Diego Wilson [:diego]]

Comment on attachment 758339 [details] [diff] [review]
rebased patch

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

Tried out most core apps (including video playback) and it works fine. I couldn't test the camera app because it was crashing even before applying this patch and HWC disabled. Other than the patch looks good to me.

Comment 49

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8634a682e646

Comment 50

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8634a682e646