925444 - Gecko layers should honor hwc releaseFenceFd on b2g v1.3

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P1)

Description

[Diego Wilson [:diego]]

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

Comment 1

[Diego Wilson [:diego]]

Hi pchang,

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

Comment 2

[Sotaro Ikeda [:sotaro]]

Dan Glastonbury(:djg :kamidphish)[Bugmail: [e-mail]] is a person who is investigated around Fence and WHC.

Comment 3

[Diego Wilson [:diego]]

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.

Comment 4

[u480271]

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?

Comment 5

[Sotaro Ikeda [:sotaro]]

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?

Comment 6

[Sotaro Ikeda [:sotaro]]

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

Comment 7

[Peter Chang[:pchang]]

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

Comment 8

[Peter Chang[:pchang]]

(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.

Comment 9

[Diego Wilson [:diego]]

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

Comment 10

[Diego Wilson [:diego]]

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

Comment 11

[u480271]

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.

Comment 12

[Diego Wilson [:diego]]

(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.

Comment 13

[Peter Chang[:pchang]]

(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.

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

(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.

Comment 16

[Diego Wilson [:diego]]

(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?

Comment 17

[Diego Wilson [:diego]]

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.

Comment 18

[Sotaro Ikeda [:sotaro]]

> 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.

Comment 19

[Diego Wilson [:diego]]

(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.

Comment 20

[u480271]

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

Comment 21

[Sotaro Ikeda [:sotaro]]

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

Comment 22

[Sotaro Ikeda [:sotaro]]

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.

Comment 23

[Sotaro Ikeda [:sotaro]]

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

Comment 24

[Diego Wilson [:diego]]

(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.

Comment 25

[Diego Wilson [:diego]]

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

Comment 26

[Diego Wilson [:diego]]

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

Comment 27

[Sushil]

(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.

Comment 28

[Diego Wilson [:diego]]

(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.

Comment 29

[Sotaro Ikeda [:sotaro]]

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

Comment 30

[Sotaro Ikeda [:sotaro]]

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

Comment 31

[Sotaro Ikeda [:sotaro]]

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

Comment 32

[Sotaro Ikeda [:sotaro]]

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?

Comment 33

[Sushil]

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.

Comment 34

[u480271]

Milan/Sotaro,

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

Comment 35

[Milan Sreckovic [:milan] (needinfo for best results)]

Will talk off line and post the conclusions.

Comment 36

[Preeti Raghunath(:Preeti)]

Sandip

Please help clarify the requirement here.

Comment 37

[Sandip Kamat]

(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.

Comment 38

[Milan Sreckovic [:milan] (needinfo for best results)]

This is JB 4.3 issue only, correct?

Comment 39

[Diego Wilson [:diego]]

(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.

Comment 40

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 41

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(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

Comment 42

[Peter Chang[:pchang]]

(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.

Comment 43

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 44

[Andreas Gal :gal]

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.

Comment 45

[Michael Vines [:m1] [:evilmachines]]

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

Comment 46

[Andreas Gal :gal]

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.

Comment 47

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 48

[Diego Wilson [:diego]]

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.

Comment 49

[Peter Chang[:pchang]]

(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.

Comment 50

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 51

[Diego Wilson [:diego]]

(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.

Comment 52

[Michael Vines [:m1] [:evilmachines]]

(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.

Comment 53

[Sotaro Ikeda [:sotaro]]

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

Comment 54

[Sushil]

(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.

Comment 55

[Milan Sreckovic [:milan] (needinfo for best results)]

(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.

Comment 56

[Sotaro Ikeda [:sotaro]]

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)

Comment 57

[Sotaro Ikeda [:sotaro]]

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.

Comment 58

[Sotaro Ikeda [:sotaro]]

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

Comment 59

[Sotaro Ikeda [:sotaro]]

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

Comment 60

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch part 1 - Handle Fence in TextureHost

This patch is just a wip patch.

Comment 61

[Sotaro Ikeda [:sotaro]]

Remove Bug 767484 from dependent bug.

Comment 62

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v2 - handle Fence on gonk JB

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

Comment 63

[Sotaro Ikeda [:sotaro]]

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.

Comment 64

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v3 - handle Fence on gonk JB

Fix IPC problems. Fence is delivered until content process.

Comment 65

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v4 - handle Fence on gonk JB

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.

Comment 66

[Sotaro Ikeda [:sotaro]]

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.

Comment 67

[Sotaro Ikeda [:sotaro]]

Oh, b2g process returns incorrect TextureID.

Comment 68

[Sotaro Ikeda [:sotaro]]

(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!

Comment 69

[Diego Wilson [:diego]]

(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.

Comment 70

[Sotaro Ikeda [:sotaro]]

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.

Comment 71

[Sotaro Ikeda [:sotaro]]

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

Comment 72

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v5 - handle Fence on gonk JB

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

Comment 73

[Andreas Gal :gal]

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

Comment 74

[Sotaro Ikeda [:sotaro]]

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

Comment 75

[Andreas Gal :gal]

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

Comment 76

[Sotaro Ikeda [:sotaro]]

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.

Comment 77

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v6 for b2g1.3 - handle Fence on gonk JB

Fixed the crash.

Comment 78

[Sotaro Ikeda [:sotaro]]

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

Comment 79

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v6 for b2g1.3 - handle Fence on gonk JB

Replace by a correct one.

Comment 80

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v7 for b2g1.3 - handle Fence on gonk JB

Enable fence for camera preview.

Comment 81

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v7 for b2g1.3 - handle Fence on gonk JB

Update to correct one.

Comment 82

[Sotaro Ikeda [:sotaro]]

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

Comment 83

[Sotaro Ikeda [:sotaro]]

(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.

Comment 84

[Sotaro Ikeda [:sotaro]]

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.

Comment 85

[Nicolas Silva [:nical]]

(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

Comment 86

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v8 for b2g1.3 - handle Fence on gonk JB

Add ContentClient/ContentHost support.

Comment 87

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v1 - Change to TextureHost

Create TextureHostCommon as common base class of TextureHost and DeprecatedTextureHost.

Comment 88

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 v1 - Change to LayerRenderState

Comment 89

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 v1 - Change to CompositableHost

Comment 90

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 v1 - Change to HwcComposer2D

Comment 91

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 v1 - Add FenceHandle for ipc

Comment 92

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 6 v1 - Add ReturnTextureData to LayersMessages

Comment 93

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 v1 - Change to CompositableTransactionParent

Comment 94

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 8 v1 - Change to /gfx/layers/moz.build

Comment 95

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 9 v1 - Change to GraphicBufferLocked

Comment 96

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 10 v1 - Change to ClientLayerManager

Comment 97

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 11 v1 - Change to ImageBridgeChild

Comment 98

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 12 v1 - Change to TextureClient

Comment 99

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 13 v1 - Change to CompositableClient

Comment 100

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 14 v1 - Change to OmxDecoder

Comment 101

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 15 v1 - Change to GonkNativeWindow

Comment 102

[Sotaro Ikeda [:sotaro]]

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?

Comment 103

[Sotaro Ikeda [:sotaro]]

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

Comment 104

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - enable hwc on nexus-4

Comment 105

[Sotaro Ikeda [:sotaro]]

Current patches have a problem of CrystallSkull crash.

Comment 106

[Nicolas Silva [:nical]]

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.

Comment 107

[Nicolas Silva [:nical]]

(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.

Comment 108

[Nicolas Silva [:nical]]

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 :)

Comment 109

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 v2 - Change to CompositableTransactionParent

Fix CrystallSkull crash.

Comment 110

[Sushil]

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 ?

Comment 111

[Sotaro Ikeda [:sotaro]]

(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);
    }
}

Comment 112

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Comment 113

[Sotaro Ikeda [:sotaro]]

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.

Comment 114

[Diego Wilson [:diego]]

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

Comment 115

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v2 - Change to TextureHost

Comment 116

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 v2 - Change to CompositableHost

Comment 117

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 v2 - Change to HwcComposer2D(r=sushil, nical)

Comment 118

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 v2 - Add FenceHandle for ipc

Comment 119

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 v3 - Change to CompositableTransactionParent

Comment 120

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 11 v2 - Change to ImageBridgeChild

Comment 121

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 13 v2 - Change to CompositableClient

Comment 122

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 14 v2 - Change to OmxDecoder

Comment 123

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 15 v2 - Change to GonkNativeWindow

Comment 124

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Comment 125

[Sotaro Ikeda [:sotaro]]

Attachment: Release Fence handling related diagram

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

Comment 126

[Sotaro Ikeda [:sotaro]]

Attachment: Release Fence handling related diagram

Fix a comment of GrallocTextureClientOGL.

Comment 127

[Sotaro Ikeda [:sotaro]]

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

Comment 128

[Sotaro Ikeda [:sotaro]]

Attachment: Release Fence handling related diagram

Add "Camera Hw" to the diagram.

Comment 129

[Peter Chang[:pchang]]

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 '}'

Comment 130

[Nicolas Silva [:nical]]

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 131

[Nicolas Silva [:nical]]

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

Comment 132

[Nicolas Silva [:nical]]

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.

Comment 133

[Sotaro Ikeda [:sotaro]]

(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.

Comment 134

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v3 - Change to TextureHost

Apply the comment.

Comment 135

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 v3 - Change to CompositableHost

Apply the comment.

Comment 136

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 6 v2 - Add ReturnReleaseFence to LayersMessages

Apply the comment.

Comment 137

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 v4 - Change to CompositableTransactionParent

Apply the comment.

Comment 138

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 8 v2 - Change to /gfx/layers/moz.build

Apply the comment.

Comment 139

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 9 v2 - Change to GraphicBufferLocked

Apply the comment.

Comment 140

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 10 v2 - Change to ClientLayerManager

Apply the comment.

Comment 141

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 11 v3 - Change to ImageBridgeChild

Apply the comment.

Comment 142

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 12 v2 - Change to TextureClient

Apply the comment.

Comment 143

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 13 v3 - Change to CompositableClient

Apply the comment.

Comment 144

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 14 v3 - Change to OmxDecoder

Apply the comment.

Comment 145

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 15 v2 - Change to GonkNativeWindow(r=pchang)

Apply the comment.

Comment 146

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Comment 147

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 14 v4 - Change to OmxDecoder

In non JB4.3 platform, return MediaBuffer and GraphicBuffer same way as before.

Comment 148

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Comment 149

[Sushil]

Comment on attachment 8349683 [details] [diff] [review]
patch part 4 v2 - Change to HwcComposer2D(r=sushil, nical)

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

Sotaro,

It seems there is fd leak.
1. Play a video.
2. Press home-screen button to exit video playback and return to Home screen.
3. Go to step 1.

Check:
adb root
adb shell lsof | grep "sync_fence"

The sync_fence fd numbers in b2g process keep on increasing. I hope this is the right way to check fd leak?
Make sure that video playback takes HWC Composition path to reproduce it easily.
I tried closing previous retire fence fd and Color layer (null texture) release fence fds. But this also does not help.
So leak seems to be somewhere else.

Comment 150

[Sushil]

Comment on attachment 8349683 [details] [diff] [review]
patch part 4 v2 - Change to HwcComposer2D(r=sushil, nical)

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

Same issue is observed with new roll up patch: attachment 8350792 [details] [diff] [review] as well. Please check.

Comment 151

[Nicolas Silva [:nical]]

Comment on attachment 8350475 [details] [diff] [review]
patch part 1 v3 - Change to TextureHost

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

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +240,5 @@
> +                  mReleaseFence, aReleaseFence);
> +    if (!mergedFence.get()) {
> +      // Failed to merge fences.
> +      // Store new fence will act like a union
> +      mReleaseFence = aReleaseFence;

it's not totally clear to what's going on here. Is failing to merge fences happening often? what can cause this to fail? ~Fence seems to take care of closing the fd so it doesn't look worrying to me. If you know about the questions above an additional comment would be nice.
I don't understand what you mean with "act like a union".

::: gfx/layers/opengl/TextureHostOGL.h
@@ +163,5 @@
> +   * Return a releaseFence's Fence and clear a reference to the Fence.
> +   */
> +  virtual android::sp<android::Fence> GetAndResetReleaseFence();
> +protected:
> +  android::sp<android::Fence> mReleaseFence;

Maybe we should think about a cross-platform mozilla::layers::Fence abstraction, let TextureHost manipulate the cross-platform abstraction, and let HWComposer  and friends downcast to, say, a AndroidFence chid class or whatever we want to call it. It's probably not a big deal in this case but since this patch queue will need to be partially rewritten for mozilla-central, we could use such a Fence abstraction at that point. It's worth discussing about this with the SurfaceStream experts who will most likely need a cross-platform way to manipulate fences. Also I don't know fence APIs very well so this is mostly food for thoughts. Let's worry about it for the post 1.2 version of this patch.

Comment 152

[Nicolas Silva [:nical]]

Comment on attachment 8350476 [details] [diff] [review]
patch part 3 v3 - Change to CompositableHost

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

::: gfx/layers/composite/CompositableHost.h
@@ +96,5 @@
> +   * by CompositableHost.
> +   */
> +  RefPtr<TextureHostCommon> mCurrentTexture;
> +  /**
> +   * Store TextureHosts might have data to be delivered to TextureClient

nit: TextureHosts that might
Also please extend this comment with the kind of data (ReleaseFences) that we expect we may want to deliver to the client side.
If you think we'll only ever return ReleaseFences, we should make it clear in the function name (by replacing TextureData by ReleaseFence).

@@ +99,5 @@
> +  /**
> +   * Store TextureHosts might have data to be delivered to TextureClient
> +   * by CompositableHost.
> +   */
> +  std::vector< RefPtr<TextureHostCommon> > mPendingReturnTextureData;

Out of curiosity, since this is not backend-specific, why placing this in CompositableBackendSpecificData rather than CompositableHost?

Comment 153

[Nicolas Silva [:nical]]

Comment on attachment 8350478 [details] [diff] [review]
patch part 7 v4 - Change to CompositableTransactionParent

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

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +358,5 @@
> +#else
> +void
> +CompositableParentManager::ReturnTextureDataIfNecessary(CompositableHost* aCompositable,
> +                                                       EditReplyVector& replyv,
> +                                                       PCompositableParent* aParent)

nit: indentation

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +72,5 @@
>    if (Compositor::GetBackend() == LAYERS_NONE) {
>      return true;
>    }
>  
> +  // Clear fence handles used in previsou transaction.

nit: previous

Comment 154

[Nicolas Silva [:nical]]

Comment on attachment 8350481 [details] [diff] [review]
patch part 10 v2 - Change to ClientLayerManager

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +386,5 @@
> +          break;
> +        }
> +        CompositableClient* compositable
> +          = static_cast<CompositableChild*>(rep.compositableChild())->GetCompositableClient();
> +        compositable->SetReleaseFence(fence);

Why does ImageBridge have extra treatment that is not appearing here?

Comment 155

[Nicolas Silva [:nical]]

Comment on attachment 8350648 [details] [diff] [review]
patch part 14 v4 - Change to OmxDecoder

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

I don't know this code very well so this review was rather quick and superficial.

::: content/media/omx/OmxDecoder.h
@@ +121,5 @@
>    //   them after use: see ReleaseVideoBuffer(), ReleaseAudioBuffer()
>    MediaBuffer *mVideoBuffer;
>    MediaBuffer *mAudioBuffer;
>  
> +  struct BufferItem {

Beware there's already a BufferItem struct declared in GonkBufferQueue.h (which is within GonkBufferQueue's namespace so no symbol collision problem). Did you give it the same name because they have similar semantics/roles?

Comment 156

[Nicolas Silva [:nical]]

Comment on attachment 8349684 [details] [diff] [review]
patch part 5 v2 - Add FenceHandle for ipc

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

::: gfx/layers/ipc/FenceUtils.h
@@ +15,5 @@
> + */
> +#if MOZ_WIDGET_GONK && ANDROID_VERSION >= 18
> +# include "mozilla/layers/FenceUtilsGonk.h"
> +#else
> +namespace mozilla { namespace layers {

nit: namespace layers on a new line

Comment 157

[Sushil]

Comment on attachment 8349683 [details] [diff] [review]
patch part 4 v2 - Change to HwcComposer2D(r=sushil, nical)

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

review- due to Comment 149. For some reason, I cannot set review flag on this attachment.

Comment 158

[Nicolas Silva [:nical]]

Comment on attachment 8349683 [details] [diff] [review]
patch part 4 v2 - Change to HwcComposer2D(r=sushil, nical)

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

I haven't worked with HWComposer code so far, so this is a very uneducated review (good thing I am not the only reviewer for this part!).

Comment 159

[Nicolas Silva [:nical]]

Comment on attachment 8350486 [details] [diff] [review]
patch part 15 v2 - Change to GonkNativeWindow(r=pchang)

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

::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp
@@ +104,5 @@
>      return mBufferQueue->setDefaultBufferFormat(defaultFormat);
>  }
>  
>  already_AddRefed<GraphicBufferLocked>
> +GonkNativeWindow::getCurrentBuffer() {

nit: the convention is to open the bracket on a new line for function bodies.

Comment 160

[Sushil]

Comment on attachment 8349683 [details] [diff] [review]
patch part 4 v2 - Change to HwcComposer2D(r=sushil, nical)

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

For some reason, I am not able to set review flag on this patch.
Sotaro, I want to 'review-' it. Please check fd leak in Comment 149.

Comment 161

[cajbir (:cajbir)]

Comment on attachment 8350648 [details] [diff] [review]
patch part 14 v4 - Change to OmxDecoder

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

::: content/media/omx/OmxDecoder.cpp
@@ +812,5 @@
>        aFrame->mTimeUs = timeUs;
>        aFrame->mKeyFrame = keyFrame;
>        aFrame->Y.mWidth = mVideoWidth;
>        aFrame->Y.mHeight = mVideoHeight;
> +      // Release to hold video buffer in OmxDecoder more.

I don't understand this comment. Can you make it clearer? What is "OmxDecoder more".

Comment 162

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #151)
> Comment on attachment 8350475 [details] [diff] [review]
> patch part 1 v3 - Change to TextureHost
> 
> Review of attachment 8350475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/TextureHostOGL.cpp
> @@ +240,5 @@
> > +                  mReleaseFence, aReleaseFence);
> > +    if (!mergedFence.get()) {
> > +      // Failed to merge fences.
> > +      // Store new fence will act like a union
> > +      mReleaseFence = aReleaseFence;
> 
> it's not totally clear to what's going on here. Is failing to merge fences
> happening often? what can cause this to fail? ~Fence seems to take care of
> closing the fd so it doesn't look worrying to me. If you know about the
> questions above an additional comment would be nice.
> I don't understand what you mean with "act like a union".

This failure handling is just same as in android. I am going update the comment in the next patch.
http://androidxref.com/4.3_r2.1/xref/frameworks/native/libs/gui/ConsumerBase.cpp#216

> ::: gfx/layers/opengl/TextureHostOGL.h
> @@ +163,5 @@
> > +   * Return a releaseFence's Fence and clear a reference to the Fence.
> > +   */
> > +  virtual android::sp<android::Fence> GetAndResetReleaseFence();
> > +protected:
> > +  android::sp<android::Fence> mReleaseFence;
> 
> Maybe we should think about a cross-platform mozilla::layers::Fence
> abstraction, let TextureHost manipulate the cross-platform abstraction, and
> let HWComposer  and friends downcast to, say, a AndroidFence chid class or
> whatever we want to call it. It's probably not a big deal in this case but
> since this patch queue will need to be partially rewritten for
> mozilla-central, we could use such a Fence abstraction at that point. It's
> worth discussing about this with the SurfaceStream experts who will most
> likely need a cross-platform way to manipulate fences. Also I don't know
> fence APIs very well so this is mostly food for thoughts. Let's worry about
> it for the post 1.2 version of this patch.

Maybe in future it might be necessary. For the time being, fence is used only on gonk and it's characteristic is heavily android specific. This bug is only for 1.3. We could discuss about it when we apply it to master.

Comment 163

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #152)
> Comment on attachment 8350476 [details] [diff] [review]
> patch part 3 v3 - Change to CompositableHost
> 
> Review of attachment 8350476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/CompositableHost.h
> @@ +96,5 @@
> > +   * by CompositableHost.
> > +   */
> > +  RefPtr<TextureHostCommon> mCurrentTexture;
> > +  /**
> > +   * Store TextureHosts might have data to be delivered to TextureClient
> 
> nit: TextureHosts that might
> Also please extend this comment with the kind of data (ReleaseFences) that
> we expect we may want to deliver to the client side.
> If you think we'll only ever return ReleaseFences, we should make it clear
> in the function name (by replacing TextureData by ReleaseFence).

I am going to update the comment in a next patch.

> 
> @@ +99,5 @@
> > +  /**
> > +   * Store TextureHosts might have data to be delivered to TextureClient
> > +   * by CompositableHost.
> > +   */
> > +  std::vector< RefPtr<TextureHostCommon> > mPendingReturnTextureData;
> 
> Out of curiosity, since this is not backend-specific, why placing this in
> CompositableBackendSpecificData rather than CompositableHost?

Current fence handling is gonk platform specific. I wanted to limit the change only to gonk related code as possible.

Comment 164

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #153)
> Comment on attachment 8350478 [details] [diff] [review]
> patch part 7 v4 - Change to CompositableTransactionParent
> 
> Review of attachment 8350478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositableTransactionParent.cpp
> @@ +358,5 @@
> > +#else
> > +void
> > +CompositableParentManager::ReturnTextureDataIfNecessary(CompositableHost* aCompositable,
> > +                                                       EditReplyVector& replyv,
> > +                                                       PCompositableParent* aParent)
> 
> nit: indentation

Fix in a next patch.

> 
> ::: gfx/layers/ipc/ImageBridgeParent.cpp
> @@ +72,5 @@
> >    if (Compositor::GetBackend() == LAYERS_NONE) {
> >      return true;
> >    }
> >  
> > +  // Clear fence handles used in previsou transaction.
> 
> nit: previous

Fix in a next patch.

Comment 165

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #154)
> Comment on attachment 8350481 [details] [diff] [review]
> patch part 10 v2 - Change to ClientLayerManager
> 
> Review of attachment 8350481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/ClientLayerManager.cpp
> @@ +386,5 @@
> > +          break;
> > +        }
> > +        CompositableClient* compositable
> > +          = static_cast<CompositableChild*>(rep.compositableChild())->GetCompositableClient();
> > +        compositable->SetReleaseFence(fence);
> 
> Why does ImageBridge have extra treatment that is not appearing here?

Thebes Layer handles gralloc buffers in CompositableClient level and still uses deprecated textures. But ImageBridge handles gralloc buffers in TextureClient(GraphicBufferLocked on gonk) level and using new textures.

Comment 166

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #155)
> Comment on attachment 8350648 [details] [diff] [review]
> patch part 14 v4 - Change to OmxDecoder
> 
> Review of attachment 8350648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know this code very well so this review was rather quick and
> superficial.
> 
> ::: content/media/omx/OmxDecoder.h
> @@ +121,5 @@
> >    //   them after use: see ReleaseVideoBuffer(), ReleaseAudioBuffer()
> >    MediaBuffer *mVideoBuffer;
> >    MediaBuffer *mAudioBuffer;
> >  
> > +  struct BufferItem {
> 
> Beware there's already a BufferItem struct declared in GonkBufferQueue.h
> (which is within GonkBufferQueue's namespace so no symbol collision
> problem). Did you give it the same name because they have similar
> semantics/roles?

Yes.

Comment 167

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #159)
> Comment on attachment 8350486 [details] [diff] [review]
> patch part 15 v2 - Change to GonkNativeWindow(r=pchang)
> 
> Review of attachment 8350486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp
> @@ +104,5 @@
> >      return mBufferQueue->setDefaultBufferFormat(defaultFormat);
> >  }
> >  
> >  already_AddRefed<GraphicBufferLocked>
> > +GonkNativeWindow::getCurrentBuffer() {
> 
> nit: the convention is to open the bracket on a new line for function bodies.

Update it in a next patch.

Comment 168

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Double (:doublec) from comment #161)
> Comment on attachment 8350648 [details] [diff] [review]
> patch part 14 v4 - Change to OmxDecoder
> 
> Review of attachment 8350648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OmxDecoder.cpp
> @@ +812,5 @@
> >        aFrame->mTimeUs = timeUs;
> >        aFrame->mKeyFrame = keyFrame;
> >        aFrame->Y.mWidth = mVideoWidth;
> >        aFrame->Y.mHeight = mVideoHeight;
> > +      // Release to hold video buffer in OmxDecoder more.
> 
> I don't understand this comment. Can you make it clearer? What is
> "OmxDecoder more".

Hmm, comment seems to incomplete. Update in a next patch. I wanted to say the following.
- MediaBuffer's reference count is already incremented by VideoGraphicBuffer's constructor.
  So, there is no need to hold the MediaBuffer in OmxDecoder.
  And by relasing the buffer at OmxDecoder. The MediBuffer's reference count is set to one.
  It is necessary to ensure to return MediaBuffer to OMXCodec in OmxDecoder::ReleaseAllPendingVideoBuffersLocked().

Comment 169

[Sushil]

(In reply to Sushil from comment #160)
> Review of attachment 8349683 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For some reason, I am not able to set review flag on this patch.
> Sotaro, I want to 'review-' it. Please check fd leak in Comment 149.

Sotaro, did you check Comment 149 ?

Comment 170

[Sotaro Ikeda [:sotaro]]

(In reply to Sushil from comment #169)
> (In reply to Sushil from comment #160)
> > Review of attachment 8349683 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > For some reason, I am not able to set review flag on this patch.
> > Sotaro, I want to 'review-' it. Please check fd leak in Comment 149.
> 
> Sotaro, did you check Comment 149 ?

After updating the patches. I am going to check Comment 149.

Comment 171

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #163)
> (In reply to Nicolas Silva [:nical] from comment #152)
> > Comment on attachment 8350476 [details] [diff] [review]
> > patch part 3 v3 - Change to CompositableHost
> > 
> > Review of attachment 8350476 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/composite/CompositableHost.h
> > @@ +96,5 @@
> > > +   * by CompositableHost.
> > > +   */
> > > +  RefPtr<TextureHostCommon> mCurrentTexture;
> > > +  /**
> > > +   * Store TextureHosts might have data to be delivered to TextureClient
> > 
> > nit: TextureHosts that might
> > Also please extend this comment with the kind of data (ReleaseFences) that
> > we expect we may want to deliver to the client side.
> > If you think we'll only ever return ReleaseFences, we should make it clear
> > in the function name (by replacing TextureData by ReleaseFence).
> 

I do not expect other data to deliver to the client side. I am going to replace TextureData by ReleaseFence.

Comment 172

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #159)
> Comment on attachment 8350486 [details] [diff] [review]
> patch part 15 v2 - Change to GonkNativeWindow(r=pchang)
> 
> Review of attachment 8350486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp
> @@ +104,5 @@
> >      return mBufferQueue->setDefaultBufferFormat(defaultFormat);
> >  }
> >  
> >  already_AddRefed<GraphicBufferLocked>
> > +GonkNativeWindow::getCurrentBuffer() {
> 
> nit: the convention is to open the bracket on a new line for function bodies.

This file is created from an andorid's file and kept as same as possible to original one. This file's convention is different from gecko's coding rule.

Comment 173

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v4 - Change to TextureHost(r=nical)

Apply the comment.

Comment 174

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 v4 - Change to CompositableHost(r=nical)

Apply the comment.

Comment 175

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 v3 - Add FenceHandle for ipc(r=nical)

Apply the comment.

Comment 176

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 v5 - Change to CompositableTransactionParent(r=nical)

Change 'Texture data' to 'ReleaseFence' to clarify the code.

Comment 177

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 14 v5 - Change to OmxDecoder (r=nical,doublec)

Update comments.

Comment 178

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 15 v4 - Change to GonkNativeWindow(r=pchang,nical)

Update nits as to be consistent style within a file.

Comment 179

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Comment 180

[Sotaro Ikeda [:sotaro]]

Going to check Comment 149 based on updated patches.

Comment 181

[Nicolas Silva [:nical]]

Ok, so I think we agree that in the very short term (1.3 branch) we want gonk-specific Fences as you implemented in this bug, and that we will make it more cross-platform before adding the feature to mozilla-central.

Comment 182

[Sotaro Ikeda [:sotaro]]

(In reply to Sushil from comment #149)
> 
> The sync_fence fd numbers in b2g process keep on increasing. I hope this is
> the right way to check fd leak?

I found the problem. Yeah, leaking happening because of mutual strong pointer reference between TextureHost and CompositableBackendSpecificData. I want to use weak reference to solve the problem. But gecko does not provide it. So, need to solve the problem by using another way.

Comment 183

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v5 - Change to TextureHost

Clear strong references when they become unnecessary. It is necessary to cut mutual strong references. gecko does not provide correct weak pointer now. So, need to cut mutual strong reference after their usage.

Comment 184

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 v5 - Change to CompositableHost

Clear strong references when they become unnecessary. It is necessary to cut mutual strong reference. gecko does not provide correct weak pointer now. So, need to cut mutual strong reference after their usage.

Comment 185

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Comment 186

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8356619 [details] [diff] [review]
patch part 1 v5 - Change to TextureHost

nical, can you review the patch again?

Comment 187

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #185)
> Created attachment 8356622 [details] [diff] [review]
> roll up patch

By applying the patch, fence file descriptor problem seems to be fixed.

Comment 188

[Sotaro Ikeda [:sotaro]]

Sushil, can you confirm the updated patches in your environment?

Comment 189

[Sushil]

Sotaro, I checked with roll up patch. I do not see that fd leak now. But there is some known regression which is blocking video & camera use cases sometimes so I would suggest you to test these use cases and make sure full HWC composition path is followed (by checking "Frame rendered" log in HwcComposer2D) and confirm fd leak. You need to enable color layer support in your roll-up patch.

Comment 190

[Sushil]

Comment on attachment 8349683 [details] [diff] [review]
patch part 4 v2 - Change to HwcComposer2D(r=sushil, nical)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +593,5 @@
> +            mList->hwLayers[j].releaseFenceFd = -1;
> +            sp<Fence> fence = new Fence(fd);
> +
> +            LayerRenderState state = mHwcLayerMap[j]->GetLayer()->GetRenderState();
> +            if (!state.mTexture) {

I think you could move this check at beginning of loop. There is no harm in closing 'releaseFenceFd' for  "!state.mTexture", then continue.

@@ +597,5 @@
> +            if (!state.mTexture) {
> +                continue;
> +            }
> +            TextureHostOGL* texture = state.mTexture->AsHostOGL();
> +            if (!texture) {

Same comment as above.

Comment 191

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Remove nexus-4 specific modification.

Comment 192

[Sotaro Ikeda [:sotaro]]

> there is some known regression which is blocking video & camera use cases
> sometimes so I would suggest you to test these use cases and make sure full
> HWC composition path is followed (by checking "Frame rendered" log in
> HwcComposer2D) and confirm fd leak. You need to enable color layer support
> in your roll-up patch.

Suchil, what is the known known regression? Can you explain about it? I do not have a hardware that have color layer support hardware. I have only nexus-4. And iirc it does not support color layers.

Comment 193

[Sotaro Ikeda [:sotaro]]

(In reply to Sushil from comment #190)
> Comment on attachment 8349683 [details] [diff] [review]
> patch part 4 v2 - Change to HwcComposer2D
> 
> Review of attachment 8349683 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +593,5 @@
> > +            mList->hwLayers[j].releaseFenceFd = -1;
> > +            sp<Fence> fence = new Fence(fd);
> > +
> > +            LayerRenderState state = mHwcLayerMap[j]->GetLayer()->GetRenderState();
> > +            if (!state.mTexture) {
> 
> I think you could move this check at beginning of loop. There is no harm in
> closing 'releaseFenceFd' for  "!state.mTexture", then continue.

What is the intent of doing it? I can not find out a value to do it. From code readability point, current code seems simple.

Comment 194

[Sotaro Ikeda [:sotaro]]

> > 
> > I think you could move this check at beginning of loop. There is no harm in
> > closing 'releaseFenceFd' for  "!state.mTexture", then continue.
> 
> What is the intent of doing it? I can not find out a value to do it. From
> code readability point, current code seems simple.

In the loop there seems no time consuming thing need to skip.

Comment 195

[Sotaro Ikeda [:sotaro]]

Current patches fail to build in other platform. I am going to fix it.

Comment 196

[Sushil]

(In reply to Sotaro Ikeda [:sotaro] from comment #192)
> 
> Suchil, what is the known known regression? Can you explain about it? I do
> not have a hardware that have color layer support hardware. I have only
> nexus-4. And iirc it does not support color layers.

Sotaro,

From what I tested, I do not see the fd leak which I had reported earlier. And this new regression is not related to your patch or sync-fence. I noticed sometimes Image layer is missing in layer tree in Camera preview & video playback. Seems to be an issue in Gaia or above framework layers before coming to HwcComposer2D.

Comment 197

[Sushil]

(In reply to Sotaro Ikeda [:sotaro] from comment #193)
> > > +            if (!state.mTexture) {
> > 
> > I think you could move this check at beginning of loop. There is no harm in
> > closing 'releaseFenceFd' for  "!state.mTexture", then continue.
> 
> What is the intent of doing it? I can not find out a value to do it. From
> code readability point, current code seems simple.

Yes, current code looks simple. As long as the unused sp<Fence> is being destroyed when it goes out of scope in loop, we should be good.

Comment 198

[Sotaro Ikeda [:sotaro]]

(In reply to Sushil from comment #196)
> 
> From what I tested, I do not see the fd leak which I had reported earlier.
> And this new regression is not related to your patch or sync-fence. I
> noticed sometimes Image layer is missing in layer tree in Camera preview &
> video playback. Seems to be an issue in Gaia or above framework layers
> before coming to HwcComposer2D.

Thanks for the clarification. Is there a bug for it? If it is not, it seems better to handle in a new bug.

Comment 199

[Sotaro Ikeda [:sotaro]]

> > > I think you could move this check at beginning of loop. There is no harm in
> > > closing 'releaseFenceFd' for  "!state.mTexture", then continue.
> > 
> > What is the intent of doing it? I can not find out a value to do it. From
> > code readability point, current code seems simple.
> 
> Yes, current code looks simple. As long as the unused sp<Fence> is being
> destroyed when it goes out of scope in loop, we should be good.

Yeah, sp<Fence> is destroyed and file descriptor is closed when it go out of scope, if the fence is not set to TextureHost int the loop.

Comment 200

[Sushil]

Sotaro,

Attachment 8349683 [details] [diff] is review+ from me. As I said, I am not able to edit the review flag. So you can remove me from reviewer's list and carry forward my review+.

Comment 201

[Sotaro Ikeda [:sotaro]]

Thank for the notification! Anyway all the patches are replaced by a new roll up patch before commit.

Comment 202

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 v6 - Change to CompositableHost

Add including mozilla/layers/TextureHost.h in CompositableHost.h.

Comment 203

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 v6 - Change to CompositableTransactionParent(r=nical)

Add forward declaration to fix build failure in other platform.

Comment 204

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Comment 205

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=4db89d966e80
By the change build become success except mac.

Comment 206

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 12 v3 - Change to TextureClient (r=nical)

To fix build failure on mac, change include path from "ipc/FenceUtils.h"  to "gfxipc/FenceUtils.h".

Comment 207

[Sotaro Ikeda [:sotaro]]

Attachment: roll up patch

Comment 208

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=e1ab9ce4eeea

Comment 209

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #208)
> https://tbpl.mozilla.org/?tree=Try&rev=e1ab9ce4eeea

Hmm, tryserver seems not working correctly now.

Comment 210

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=e84671c55043

Retry tryserver, test result seems better. But tryserver seems still unstable.

Comment 211

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=2941e6f5a61a

Comment 212

[Diego Wilson [:diego]]

Sotaro,

I just tried the latest roll up patch and it doesn't fix my video playback flicker. Does it fix it for you?

Comment 213

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #212)
> Sotaro,
> 
> I just tried the latest roll up patch and it doesn't fix my video playback
> flicker. Does it fix it for you?

With nexus-4 only temporary patch attachment 8347815 [details] [diff] [review] , I confirmed the video playback works normally without flicker on nexus-4. attachment 8347815 [details] [diff] [review] is removed from the recent roll-up patch, because it is necessary only for nexus-4.

Comment 214

[Sotaro Ikeda [:sotaro]]

Also confirmed that the fence objects are delivered to OmxDecoder on nexus-4 by applying  attachment 8356987 [details] [diff] [review] and attachment 8347815 [details] [diff] [review].

Comment 215

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8356987 [details] [diff] [review]
roll up patch

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

::: gfx/layers/client/CompositableClient.h
@@ +137,5 @@
>     */
>    virtual void RemoveTextureClient(TextureClient* aClient);
>  
> +  // XXX this function is necessary on a code that Bug 897452 is not fixed.
> +  virtual TemporaryRef<TextureClient> GetAddedTextureClient(uint64_t aTextureID);

//XXX: this function is only needed until Bug 897452 is fixed.

@@ +178,5 @@
>     */
>    virtual void OnActorDestroy() = 0;
>  
> +  /**
> +   * If a fence is valid, wait the fence is completed.

If a fence is valid, wait until the fence is completed.

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +311,5 @@
>                         : nullptr); // no region means invalidate the entire surface
>  
>  
>        compositable->UseTextureHost(texture);
> +      // return ReleaseFence to client if necessary

these comments can probably be dropped because they mostly just repeat the name of the function.

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +548,5 @@
> +        texture->SetReleaseFenceHandle(fence);
> +        break;
> +      }
> +      // If TextureClient is not present, try TextureClientData.
> +      // It happens during TextureClient removing.

This happens when the TextureClient was destroyed but the TextureClientData is still alive.

Comment 216

[Sotaro Ikeda [:sotaro]]

The patches review is already ended. From now, I am going to use only roll-up patches.

Comment 217

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch for b2g v1.3 (r=nical,sushil,doublec,pchang)

Committable roll-up patch.

Comment 218

[Ryan VanderMeulen [:RyanVM]]

Wait, so this is *only* landing on Aurora (v1.3) and not trunk (v1.4)?

Comment 219

[Sotaro Ikeda [:sotaro]]

Yes, it is only for Aurora(v1.3). Not for trunk.

Comment 220

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d75ee062cb7b

Comment 221

[Diego Wilson [:diego]]

Sotaro,

I retested the landed change on v1.3. It fixes the flickers in video playback and camera on my target.

Thanks a lot for solving this! I know it was a big change.

Comment 222

[Sotaro Ikeda [:sotaro]]

It's very good the fix also works on your side! I am going to work the release fence for master(Bug 957323) after some dependent bugs are fixed.