Closed Bug 862952 Opened 7 years ago Closed 7 years ago

Composer2D never called to render after layers refactoring

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: diego, Assigned: nrc)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Assignee: nobody → ncameron
And I tried so hard to keep this working... :-(
(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.
Attached patch WIP patch (obsolete) — Splinter Review
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?
Attachment #738779 - Flags: feedback?(dwilson)
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?
Attached patch WIP patch v2 (obsolete) — Splinter Review
Attachment #738779 - Attachment is obsolete: true
Attachment #738779 - Flags: feedback?(dwilson)
(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
Blocks: 828876
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
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
Also, I'm generally curious. What's the reason for the refactoring?
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #738835 - Attachment is obsolete: true
(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).
No crash, so things look better. But it still always fails to retrieve a graphic buffer from ShadowImageLayer. Any suggestion on where to debug?
(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.
With WIP v3 and latest m-c, camera goes to HWC with colorfill enabled.

I guess something was fixed in latest m-c. :-)
Attached file Camera to camcorder crash stack (obsolete) —
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.
(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 on attachment 740533 [details]
Camera to camcorder crash stack

I can reproduce this issue before the patch. I submitted bug 864598 to track that.
Attachment #740533 - Attachment is obsolete: true
Comment on attachment 739301 [details] [diff] [review]
patch

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

Looks good to me!
Attachment #739301 - Flags: feedback+
Attachment #739301 - Flags: review?(bjacob)
Attachment #739301 - Attachment description: WIP patch v3 → patch
(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!
Thanks for jumping on this nrc. Much appreciated!
Status: NEW → ASSIGNED
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.
Attachment #739301 - Flags: review?(bjacob)
(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...
Attached patch patch (obsolete) — Splinter Review
Attachment #739301 - Attachment is obsolete: true
Attachment #741033 - Flags: review?(bjacob)
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
(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!
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 :-)
OK, looks like we get away with it then :)

:nrc can we get the SurfaceDescriptor back in LayerRenderState?
(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?
SGTM as long as said dimensions are valid the same way the dimensions in SurfaceDecriptorGralloc are. See bug 850566
Attachment #741033 - Attachment is obsolete: true
Attachment #741033 - Flags: review?(bjacob)
Attachment #742859 - Flags: review?(bjacob)
Attachment #742859 - Flags: feedback?(dwilson)
Waiting for bug 832383 will smooth the landing
Depends on: 865112
Depends on: 832383
No longer depends on: 865112
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
Attachment #742859 - Flags: feedback?(dwilson)
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.
Attachment #742859 - Flags: review?(bjacob)
Attachment #742859 - Flags: review+
(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.
(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.
Attached patch patch (obsolete) — Splinter Review
Updated patch with bjacob's comments. Carrying r=bjacob,f=diego
Attachment #742859 - Attachment is obsolete: true
Attachment #743417 - Flags: review+
Attachment #743417 - Flags: feedback+
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.
Duplicate of this bug: 878704
(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.
Flags: needinfo?(dwilson)
(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
Flags: needinfo?(dwilson)
Attached patch rebased patchSplinter Review
Rebased. Carrying r=bjacob,f=diego
Attachment #743417 - Attachment is obsolete: true
Attachment #758339 - Flags: review+
Attachment #758339 - Flags: feedback+
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.
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!
Flags: needinfo?(dwilson)
(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.
Flags: needinfo?(dwilson)
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.
Attachment #758339 - Flags: feedback+ → feedback?(dwilson)
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.
Attachment #758339 - Flags: feedback?(dwilson) → feedback+
https://hg.mozilla.org/mozilla-central/rev/8634a682e646
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 880502
You need to log in before you can comment on or make changes to this bug.