As part of the B2G upgrade to Jellybean, HwcComposer2D was updated to use the latest hwcomposer.h interface. Now, after HwcComposer2D renders a frame it it uses a fence sync to ensure the layer's buffer isn't written until the HWC releases it [1]. Currently on Jellybean this fence is not honored, so syncing fails and there are artifacts on the screen. The fence sync has to be honored by whoever writes to the buffer. In this case it's the gecko layers. I propose focusing first on the ImageLayer since it hits the most important use cases: video playback and camera preview. Ideally we would use a generic mechanism to do this like FramebufferSurface [2] [1] https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware/tree/include/hardware/hwcomposer.h?h=b2g_jb_mr2#n183 [2] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/libdisplay/FramebufferSurface.h#51

Hi pchang, Do you know who would be the right person I should work with here? This is blocking development HWC on JB.

Dan Glastonbury(:djg :kamidphish)[Bugmail: dglastonbury@mozilla.com] is a person who is investigated around Fence and WHC.

Hi Dan, Could you help me out with this? I'm not sure what's the best way to propagate the HWC fence sync up to the layers.

Hi Diego, sorry for taking time to reply. I'm new to this as well. I have been investigating our use of PMem on hamachi devices, and in Paris, Sotaro told me that JB uses fences to signal when buffers are free. I've been looking at other bugs (I'm currently working on 924940) and haven't had time to looked at what the JB fence requirements are yet. What information do you need from me?

Diego, I also could help you. But I also busy for other koi+ bugs. It would be great If I can debug the problem that you are facing on my side. Do you know how to enable hw composer on v1.3 nexus-4?

dan ,pchang, do you know how to enable hw composer on v1.3 nexus-4?

There is another bug to enable HWC on v1.3 for nexus 4. Sotaro, you can hard-cdoe sUsingHWC as true, but you will see HWC doesn't support R/B swap feature. https://bugzilla.mozilla.org/show_bug.cgi?id=933140#c1

(In reply to Diego Wilson [:diego] from comment #1) > Hi pchang, > > Do you know who would be the right person I should work with here? This is > blocking development HWC on JB. I'm available to study HWC fence with gecko layer. Will update my comment next week.

Thanks to all for the responses Dan, I mainly need some architectural guidance. Hwc (and the GPU compositor for that matter) mostly treat layers as read-only. releaseFenceFd introduces a new blocking concept that requires interaction with the layer. We'll either have to provide the fences to the layers or create a syncing new class that the layers can interact with (see FramebufferSurface). I'd appreciate some suggestion on which approach is preferred. [1] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/libdisplay/FramebufferSurface.h#51

I took a quick peek at how this is done in AOSP. It seems fence sync support is an intrinsic feature of SurfaceTexture via GLConsumer [1] [2]. Perhaps we can take the same approach in Gecko? [1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/core/jni/android/graphics/SurfaceTexture.cpp?h=jb_3.2#n79 [2] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/native/tree/include/gui/GLConsumer.h?h=b2g_jb_3.2#n68

At the Paris work week, we discussed a new method for surface sharing that is similar to SurfaceTexture & GLConsumer. When we implement this, fence sync support will go there.

(In reply to Dan Glastonbury :djg :kamidphish from comment #11) > At the Paris work week, we discussed a new method for surface sharing that > is similar to SurfaceTexture & GLConsumer. When we implement this, fence > sync support will go there. I see. Is this work planned for v1.3? If not, we'll need a stopgap solution in the meantime.

(In reply to Dan Glastonbury :djg :kamidphish from comment #11) > At the Paris work week, we discussed a new method for surface sharing that > is similar to SurfaceTexture & GLConsumer. When we implement this, fence > sync support will go there. Are you talking about bug 767484? It works when GLContext is created.

The following seems the related document. https://wiki.mozilla.org/Platform/GFX/Surfaces

(In reply to peter chang[:pchang] from comment #13) > (In reply to Dan Glastonbury :djg :kamidphish from comment #11) > > At the Paris work week, we discussed a new method for surface sharing that > > is similar to SurfaceTexture & GLConsumer. When we implement this, fence > > sync support will go there. > > Are you talking about bug 767484? It works when GLContext is created. It is not talking only to bug 767484. We are going to simplify the architecture my borrowing some ideas from SurfaceTexture and EGLStream.

(In reply to Sotaro Ikeda [:sotaro] from comment #15) > (In reply to peter chang[:pchang] from comment #13) > > (In reply to Dan Glastonbury :djg :kamidphish from comment #11) > > > At the Paris work week, we discussed a new method for surface sharing that > > > is similar to SurfaceTexture & GLConsumer. When we implement this, fence > > > sync support will go there. > > > > Are you talking about bug 767484? It works when GLContext is created. > > It is not talking only to bug 767484. We are going to simplify the > architecture my borrowing some ideas from SurfaceTexture and EGLStream. Sotaro, Can this be finished before Mozilla's v1.3 uplift? Would you be the person carrying forward this effort?

Considering this bug is blocking v1.3, maybe this is as good a reason as any to bump up the surface re-architecture discussion in bug 767484.

> Sotaro, > > Can this be finished before Mozilla's v1.3 uplift? Would you be the person > carrying forward this effort? Yeah, I think it is necessary. I am one of the people to do it.

(In reply to Sotaro Ikeda [:sotaro] from comment #18) > > Sotaro, > > > > Can this be finished before Mozilla's v1.3 uplift? Would you be the person > > carrying forward this effort? > > Yeah, I think it is necessary. I am one of the people to do it. Great! Let's continue discussion in bug 767484.

Diego: I'm sorry but are you still waiting for info on this, or are we moving discussion to 767484?

It seems better that the fence problem is handled in this bug, I think.

I enabled hwc on nexus4 and nexus7 and confirmed the video drawing problem. On nexus4, the problem was easily recognizable. On nexus7, I saw a rendering problem only scrolling youtube page during video playback.

From the symptom on nexus4 in Comment 22, gralloc buffers seems returned to hw codec too early.

(In reply to Sotaro Ikeda [:sotaro] from comment #23) > From the symptom on nexus4 in Comment 22, gralloc buffers seems returned to > hw codec too early. Yep, you'll see a very noticeable flicker.

We currently close the releaseFenceFd immediately after queueing a new frame to HWC rather than waiting for HWC to finish reading the buffer [1]. This is the cause of the flicker and the reason for this bug. [1] https://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#494

It looks like some of the releaseFence pipping is already there in GonkNativeWindow via GonkConsumerBase, but it's not currently used. [1] https://mxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkNativeWindowJB.h#53 [2] https://mxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkConsumerBase.cpp#206

(In reply to Diego Wilson [:diego] from comment #25) > We currently close the releaseFenceFd immediately after queueing a new frame > to HWC rather than waiting for HWC to finish reading the buffer [1]. This is > the cause of the flicker and the reason for this bug. > > [1] > https://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D. > cpp#494 [1] is full HWC Composition path and FB layer is not used in full HWC composition, so we can close releaseFenceFd of FB layer. This should not cause flicker.

(In reply to Sushil from comment #27) > (In reply to Diego Wilson [:diego] from comment #25) > > We currently close the releaseFenceFd immediately after queueing a new frame > > to HWC rather than waiting for HWC to finish reading the buffer [1]. This is > > the cause of the flicker and the reason for this bug. > > > > [1] > > https://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D. > > cpp#494 > > [1] is full HWC Composition path and FB layer is not used in full HWC > composition, so we can close releaseFenceFd of FB layer. This should not > cause flicker. Sorry about that. I guess the main point here is that the content generators are not waiting on the release fence before writing to their gralloc buffers.

Each layer need to handle "Release Fence" correctly like android does. In android, it is handled at Layer::onLayerDisplayed(). It receive "Release Fence" from HWC and set the Fence to BufferQueue. The BufferQueue use the Fence to check finish HWC's buffer reading. http://androidxref.com/4.3_r2.1/xref/frameworks/native/services/surfaceflinger/Layer.cpp#143

For the time being, we need to handle at least Image Layer.

The link to android 4.3's diagram around HWComposer. https://github.com/sotaroikeda/android-diagrams/blob/master/graphics/HWComposer_4.3.pdf?raw=true

Suhil, in b2g, previous retireFenceFd seems to be used for wait HWC rendering complete. But in android, the retireFenceFd seems not used for waiting. Each layer's releaseFenceFds are used for waiting. Is my understanding correct?

Sotaro, Yes, your understanding is correct. Android framework honors releaseFenceFds (set by the driver) of each layer that is why, no need to wait on retireFenceFd in HWC wrapper. But in Gecko, since it does not honor release fences, that is why we are waiting on retireFenceFd in HWC wrapper. It is sufficient for now because as per HWC header, it denotes that contents on display have been replaced and driver is no-longer reading it. In our wrapper, it is used as an intermediate work-around to reduce tearing. Just a heads-up, we will remove "storing release fence Fds and waiting on retire fence of previous draw cycle" from HwcComposer2D once Gecko layers start honoring HWC releaseFenceFds. So, after hwc set call, we will just set the releaseFenceFds (which are coming from driver) on the corresponding Gecko layers so that framework can wait on them. Similar to mFBSurface->setReleaseFenceFd() call right now.

Milan/Sotaro, Do you want me to start looking at implementing the honoring of releaseFenceFds in Gecko layers?

Will talk off line and post the conclusions.

Sandip Please help clarify the requirement here.

(In reply to Preeti Raghunath(:Preeti) from comment #36) > Sandip > > Please help clarify the requirement here. Qualcomm/Mvines confirmed that this is causing very noticeable visual artifacts when the hardware composer is used instead of the GPU for display composition, as Gecko is reusing buffers before they been fully output to the display. Hardware composer support is required to meet power goals. This indicates we need to fix it for 1.3.

This is JB 4.3 issue only, correct?

(In reply to Milan Sreckovic [:milan] from comment #38) > This is JB 4.3 issue only, correct? That is correct. It does not apply to ICS gonk.

Peter, can you and CJ find somebody to work on this? Most of the JB work happened in Taipei...

(This is absolutely the wrong product and component for this bug. Anything that needs Gecko core changes does not belong in the Firefox OS product; having it start and sit in Firefox OS::General ensures that people are unlikely to see this bug and act on it in a timely way.) -> Core::Graphics

(In reply to Milan Sreckovic [:milan] from comment #40) > Peter, can you and CJ find somebody to work on this? Most of the JB work > happened in Taipei... Yes, we will have people to work on this part. And we could discuss this more detail during Toronto work week.

When is the Toronto work week? We need a fix quickly here, ideally by next Monday or Tuesday. This bug was raised quite a while ago.

I have no idea when the Toronto work week is, but its not timely enough. This shouldn't have been in limbo this long. mvines, can we get help from Diego with this? He knows this code better than our teams.

:diego can certainly help and but need a gecko graphics expert to advise/assist. wdyt :diego?

Yeah, we can pair Diego with someone. Milan and CJ will find the right person for this. We should get going on this quickly before we run out of time.

Yes, the week in Toronto is too late, every day counts.

I'd be happy to assist with this bug. Sotaro understands it pretty well but he may be tied up with other work. Please ping me when we've found a driver for this on Mozilla's side.

(In reply to Milan Sreckovic [:milan] from comment #47) > Yes, the week in Toronto is too late, every day counts. yes, it is too late for this bug. BTW, do we need to honor hwc releaseFenceFd for all gecko layers(image/canvas/content/...)? Or we focus on camera/video(Image layer) first. Right now only camera/video cases uses GonkNativeWindow on b2g. To add the hwc fence supports for camera/video, we have some works to do. a. Pass the releaseFenceFd for each layer to content process after composition(hwc_set). b. Setup the releaseFenceFd thru GonkConsumerBase::addReleaseFence(base class of GonkNativeWindow) in content app(camera/video app) c. The meaiaserver (GonkNativeWindowClient) will refer the releaseFenceFd before drawing. For item a, it may be require new IPC protocol if want to have better performance. for item b/c, the effort is small and most works are done inside GonkNativeWindow/GonkNativeWindowClient. If there is something missing or incorrect, please feel free to comment on the bug.

The way I understand it right now, this is major feature work, and some parts will be destabilizing. If we're going to accomplish this for 1.3, we have to understand it will go way past the FC date, and will cause regressions along the way. I am assigning to Sotaro, but we need full involvement from Peter on this as well, so it's really a joint assignment.

(In reply to peter chang[:pchang] from comment #49) > BTW, do we need to honor hwc releaseFenceFd for all gecko > layers(image/canvas/content/...)? > Or we focus on camera/video(Image layer) first. Sushil can comment on this, but I believe that at a minimum we need to add it to ImageLayer and ThebesLayer in order to support the two key use cases: full MDP composition of video playback and camera preview. If we don't support CanvasLayer then it'll have to fallback to GPU. And according to some Mozilla analysis the performance of WebGL with HWC is much better than with GPU composition.

(In reply to Milan Sreckovic [:milan] from comment #50) > The way I understand it right now, this is major feature work, and some > parts will be destabilizing. If we're going to accomplish this for 1.3, we > have to understand it will go way past the FC date, and will cause > regressions along the way. It is a hard blocker.

Today, I am working for different bugs. From next Monday, I will work for this bug.

(In reply to Diego Wilson [:diego] from comment #51) > (In reply to peter chang[:pchang] from comment #49) > > BTW, do we need to honor hwc releaseFenceFd for all gecko > > layers(image/canvas/content/...)? > > Or we focus on camera/video(Image layer) first. > > Sushil can comment on this, but I believe that at a minimum we need to add > it to ImageLayer and ThebesLayer in order to support the two key use cases: > full MDP composition of video playback and camera preview. > > If we don't support CanvasLayer then it'll have to fallback to GPU. And > according to some Mozilla analysis the performance of WebGL with HWC is much > better than with GPU composition. This is needed for all layer types (Image,Canvas,etc.) except Color layer which does not use buffer anyway. If we want to prioritize some use cases like Video playback and Camera preview, then we need to have this support for all layer types present in the frame in that particular use-case. And as Diego mentioned, we need to fall back to GPU Composition, if there is any layer present in frame, for which we are not adding support.

(In reply to Michael Vines [:m1] [:evilmachines] from comment #52) > (In reply to Milan Sreckovic [:milan] from comment #50) > > The way I understand it right now, this is major feature work, and some > > parts will be destabilizing. If we're going to accomplish this for 1.3, we > > have to understand it will go way past the FC date, and will cause > > regressions along the way. > > It is a hard blocker. Not arguing that. Both can be true. It needs to be done and it's going to be major and destabilizing, although new ideas may surface to help simplify it.

In Comment 49, peter talked about the structure of the bug. In my understanding, this bug is consist of the following. - Add Fence Surrpot to TextureHost + Include pass Fence from HwComposer to TextureHost. - Add Fence Surrpot to TextureClient(Bug 941398) + Include deliver Fence from TextureHost to TextureClient - Replace all bad uses of SurfaceDescriptor by TextureClient(Bug 941389) + GonkNativeWindow sould hold a TextureClient, not a SurfaceDescriptor. + GraphicBufferLocked should hold a TextureClient, not a SurfaceDescriptor. + In OmxDecoder, replace usage of SurfaceDescriptor by TextureClient's(Bug 941413) + Inside SharedSurface's, replace SurfaceDescriptor's by TextureClient's.(Bug 941400)

There is already some bugs related to this bug as in Comment 56. But it might be better to keep the changes in this bug, for the time being.

I am going to implement based on new Texture. If PTexture protocol(Bug 897452) is going to land soon, based on PTexutre protocol.

It seem better to implement it depends on PTexture protocol(Bug 897452).

This patch is just a wip patch.

Remove Bug 767484 from dependent bug.

Add code around IPC. But IPC part does not work.

I understand why the IPC did not work. Handling of file descriptor has problems. By fixing the problems, I confirmed the Fence objects are passed to content process.

Fix IPC problems. Fence is delivered until content process.

Add delivering Fence to OMXCodec. Confirmed the Fence is delivered to ANativeWindow. Some bug seems to present. When HwComposer is disabled, Rendering seems correct. But when HwComposer is enabled, rendering still has the problem.

By attachment 8346304 [details] [diff] [review], Fence objects are delivered to OMXCodec's ANativeWindow. But the video rendering flickering problem was not fixed. Need to investigate about it. And attachment 8346304 [details] [diff] [review] have the following problem. - When video app is set to background during the video app playing video, the video app often crashes at OMXCodec.

Oh, b2g process returns incorrect TextureID.

(In reply to Sotaro Ikeda [:sotaro] from comment #67) > Oh, b2g process returns incorrect TextureID. I have to send old TextureHost's TextureID, but sent new TextureHost's TextureID. By fixing it, I locally confirmed the flickering problem is fixed!

(In reply to Sotaro Ikeda [:sotaro] from comment #68) > (In reply to Sotaro Ikeda [:sotaro] from comment #67) > > Oh, b2g process returns incorrect TextureID. > > I have to send old TextureHost's TextureID, but sent new TextureHost's > TextureID. By fixing it, I locally confirmed the flickering problem is fixed! Awesome! Can you share your latest patch? I'd like to see if it fixes some other display artifacts I've seen.

Yeah, after some clean up I am going to upload the patch. But still have some problems and restrictions. - enabled only for hw codec video playback - frequent crash during seeking and an app set to background during playback.

To focus the fix, for the time being, I am going to use b2g v1.3. Because master's code is changed a lot like the following bug. - Bug 897452 - (PTexture) Implement the PTexture protocol

The patch has some modification to enable hw compoesr on nexus-4. By applying the patch hw video rendering uses Fence. But still have some problems and limitations. - Fence is enabled only for hw codec video playback. - crashes on some situations like during seeking

Sotaro, ask for some help from the silicon vendor to debug the crashes inside their code.

Thanks for the advice. If it becomes necessary, I am going to ask them. The crash happened by OMXCodec calling abort because of detecting buffer status inconsistency in OMXCodec. It seems to be caused by gecko side's bug. > F/OMXCodec( 1372): frameworks/av/media/libstagefright/OMXCodec.cpp:1911 CHECK_EQ( (int)bufInfo->mStatus,(int)OWNED_BY_NATIVE_WINDOW) failed: 3 vs. 2

I have no doubt this is broken on our end but the vendor might be able to help us find out whats broken :)

I understand a cause of the crash. There is a case that MediaBuffer's ref count is 2, before calling MediaBuffer::release(). My patch expects that MediaBuffer's ref count is 1. If it is not, MediaBuffer is not returned to the buffer, but gralloc buffer is returned to the buffer. Therefore, OMXCodec's buffer state detect a contradiction and assert is called. MediaBuffer's ref count is incremented by OMXCodec and VideoGraphicBuffer. Therefore MediaBuffer's ref count is 2 at first. MediaBuffer's ref count is decremented by OmxDecoder::ReleaseVideoBuffer() and OmxDecoder::ReleaseAllPendingVideoBuffersLocked(). OmxDecoder::ReleaseVideoBuffer() is normally called soon after video decoding. Therefore when ReleaseAllPendingVideoBuffersLocked() is called, normally MediaBuffer's ref count is 1. But during seeking or app go to background cases, sometimes the ref count is 2, when ReleaseAllPendingVideoBuffersLocked() is called.

Fixed the crash.

Oh, I forgot to add new files to the last patch.

Replace by a correct one.

Enable fence for camera preview.

Update to correct one.

It might be better to handle Fence within SurfaceStream in a different bug.

(In reply to Sotaro Ikeda [:sotaro] from comment #82) > It might be better to handle Fence within SurfaceStream in a different bug. Created bug 950079 for it.

I changed my mind. In b2g v1.3, it seems better to handle fence by deprecated texture in thebes and canvas layers than uplifting new textures. Remove related bugs.

(In reply to Sotaro Ikeda [:sotaro] from comment #84) > I changed my mind. In b2g v1.3, it seems better to handle fence by > deprecated texture in thebes and canvas layers than uplifting new textures. > Remove related bugs. Yeah I wouldn't uplift the new ContentClient/Host for b2g 1.3, as you said it's a bit risky

Add ContentClient/ContentHost support.

Create TextureHostCommon as common base class of TextureHost and DeprecatedTextureHost.

Comment on attachment 8347799 [details] [diff] [review] patch part 1 v1 - Change to TextureHost I add TextureHostCommon as common base class of TextureHost and DeprecatedTextureHost. It becomes necessary to handle them same way in HwcComposer2D, CompositableHost and CompositableParentManager. It should be remove after deprecated texture is removed. How do you think about this idea?

Divided the patch to each parts. The each parts needs to polish more.

Current patches have a problem of CrystallSkull crash.

Comment on attachment 8347812 [details] [diff] [review] patch part 13 v1 - Change to CompositableClient Review of attachment 8347812 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/CompositableClient.h @@ +141,5 @@ > virtual void RemoveTextureClient(TextureClient* aClient); > > + virtual TemporaryRef<TextureClient> GetAddedTextureClient(uint64_t aTextureID); > + > + virtual TextureClientData* GetRemoveingTextureClientData(uint64_t aTextureID); Beware that texture IDs have been removed by the PTexture work. Also, remember that a texture can be used by several layers at the same time (at least in the ImageLayer case). I don't know how that should play with the fence stuff.

(In reply to Sotaro Ikeda [:sotaro] from comment #102) > Comment on attachment 8347799 [details] [diff] [review] > patch part 1 v1 - Change to TextureHost > > I add TextureHostCommon as common base class of TextureHost and > DeprecatedTextureHost. It becomes necessary to handle them same way in > HwcComposer2D, CompositableHost and CompositableParentManager. It should be > remove after deprecated texture is removed. How do you think about this idea? Makes sense. Let's add a comment saying that it should be removed along with the deprecated textures.

Dan and Jeff should have a look at this. Textures are going to need fence stuff on all platforms as we merge them with SurfaceStream, so it'd be nice to identify what part of this work should be made cross-platform to avoid refactoring it in a month :)

Fix CrystallSkull crash.

Comment on attachment 8347802 [details] [diff] [review] patch part 4 v1 - Change to HwcComposer2D Review of attachment 8347802 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +581,5 @@ > > int err = mHwc->set(mHwc, HWC_NUM_DISPLAY_TYPES, displays); > > + > + mPrevDisplayFence = mPrevRetireFence; Are you using mPrevDisplayFence ? When are you closing it ?

(In reply to Sushil from comment #110) > Are you using mPrevDisplayFence ? When are you closing it ? It is actually not using, current patch just mimics the implementation of andorid one's. In android's implementation, it just reference the fences, but does not use it. When a reference to Fence object is removed like the following. Fence::~Fence() { if (mFenceFd != -1) { close(mFenceFd); } }

Between master(b2g v1.4) and b2g v1.3, source code is already different a lot around gfx layers. And this bug is 1.3+ bug. With a talk with milan. We are going to commit to b2g 1.3 first and then commit to master.

I tried out the roll up patch and it fixes video flickering for me. Looking good!

Based on patches, created a diagram around release Fence handling classes. Some classes are omitted just for simplicity.

Fix a comment of GrallocTextureClientOGL.

Diagrams in the following might help understand attachment 8349842 [details]. https://github.com/sotaroikeda/firefox-diagrams/wiki/Firefox-Diagrams

Add "Camera Hw" to the diagram.

Comment on attachment 8349689 [details] [diff] [review] patch part 15 v2 - Change to GonkNativeWindow Review of attachment 8349689 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp @@ +176,5 @@ > + // If the window doesn't exist any more, release the buffer > + // directly. > + ImageBridgeChild *ibc = ImageBridgeChild::GetSingleton(); > + ibc->DeallocSurfaceDescriptorGralloc(mSurfaceDescriptor); > + } nit: space after '}'

I would like that this patch queue gets also a review from someone involved with SurfaceStream, since fence support is also important in this area (and also because with that much code, I don't feel at ease being the only reviewer).

Comment on attachment 8349695 [details] [diff] [review] roll up patch Review of attachment 8349695 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/GrallocImages.h @@ +45,5 @@ > { > return mSurfaceDescriptor; > } > > + void SetFenceHandle(const FenceHandle& aFenceHandle) if this is about a ReleaseFence, please call this SetReleasFence. @@ +47,5 @@ > } > > + void SetFenceHandle(const FenceHandle& aFenceHandle) > + { > + mFenceHandle = aFenceHandle; nit: trailing space ::: gfx/layers/client/CompositableClient.cpp @@ +294,5 @@ > aClient->MarkInvalid(); > } > > +TemporaryRef<TextureClient> > +CompositableClient::GetAddedTextureClient(uint64_t aTextureID) I am very glad that won't have to do that with PTexture! ::: gfx/layers/client/TextureClient.h @@ +123,5 @@ > */ > class TextureClientData { > public: > virtual void DeallocateSharedData(ISurfaceAllocator* allocator) = 0; > + virtual void SetFenceHandle(FenceHandle aFenceHandle) {} Please rename this SetReleaseFenceHandle (assuming we will have AcquireFence at some point in the future). ::: gfx/layers/composite/CompositableHost.h @@ +91,5 @@ > + return mPendingReturnTextureData.clear(); > + } > +protected: > + /** > + * Store a TextureHost currently used for Composition by nit: trailing space ::: gfx/layers/ipc/CompositableTransactionParentFence.cpp @@ +2,5 @@ > + * vim: sw=2 ts=8 et : > + */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ nit: If this file is going to stay roughly that small, I would prefer to just put it in CompositableTransactionParent.cpp since the latter also happen to be where we call ReturnTextureClientDataIfNecessary so having it in the same files doesn't make us jump from file to file when reading the code. Plus, CompositableTransactionParent is a rather small file so it doesn't need to be split. @@ +24,5 @@ > + if (!aCompositable || !aCompositable->GetCompositableBackendSpecificData()) { > + return; > + } > + > + const std::vector< RefPtr<TextureHostCommon> > textureList = nit: trailing space ::: gfx/layers/ipc/LayersMessages.ipdlh @@ +404,5 @@ > PCompositable compositable; > uint64_t textureId; > }; > > +struct ReturnTextureData { Please rename this into ReturnReleaseFence or ReplyReleaseFence, along with other things that are named after this struct. ::: gfx/layers/opengl/GrallocTextureClient.h @@ +54,5 @@ > virtual bool ToSurfaceDescriptor(SurfaceDescriptor& aOutDescriptor) MOZ_OVERRIDE; > > virtual TextureClientData* DropTextureData() MOZ_OVERRIDE; > > + virtual void SetFenceHandle(FenceHandle aFenceHandle); nit: MOZ_OVERRIDE ::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp @@ +176,5 @@ > + // If the window doesn't exist any more, release the buffer > + // directly. > + ImageBridgeChild *ibc = ImageBridgeChild::GetSingleton(); > + ibc->DeallocSurfaceDescriptorGralloc(mSurfaceDescriptor); > + } nit: trailing space

Overall it looks good, I'll need a bit more time to give a thorough review, but I don't have any fundamental problem with the architecture. Please be explicit about the type of fence in all the method names and members (ReleaseFence/AcquireFence) even though this patch only focuses on the ReleaseFence.

(In reply to Nicolas Silva [:nical] from comment #131) > Comment on attachment 8349695 [details] [diff] [review] > roll up patch > > Review of attachment 8349695 [details] [diff] [review]: > ----------------------------------------------------------------- Comment will apply in next patches.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.

Apply the comment.