+++ This bug was initially created as a clone of Bug #925444 +++ Bug 925444 is only for b2gv1.3. It is decided like that by the following reasons. - Bug 897452 change a lot around gfx layer code. - From b2g v1.3's deadline, we do not have enough time to prepare also for master.

Fix nits.

Fix nits.

The patch is almost same as in Bug 925444 except PTexture related parts.

Comment on attachment 8377853 [details] [diff] [review] patch part 3 - Change to OmxDecoder Review of attachment 8377853 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/OmxDecoder.cpp @@ +843,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. It doesn't parse for me. Can you rephrase it? @@ +1075,5 @@ > + buffer->graphicBuffer().get(), > + fenceFd); > + // Mark MediaBuffer as rendered. > + // When gralloc buffer is directly returned to ANativeWindow, > + // this mark is necesary. 'necessary' is spelt incorrectly.

Comment on attachment 8377851 [details] [diff] [review] patch part 1 - change to gfx layers Review of attachment 8377851 [details] [diff] [review]: ----------------------------------------------------------------- What is the timeline for this bug? I would like that we take the time to make a Fence abstraction that is cross platform (not saying this patch is not cross platform, I still need to process and digest it, although i know the 1.3 version was not cross-platform), because this is a fairly big change and we should avoid having to deeply refactor it when we start using fences in other places (d3d11 maybe?). If we have the time to discuss it, it'd be great and I'd like to involve at least Bas (because he seems to know how fencing works with d3d, but another d3d expert would do) and someone involved with SurfaceStream (jgilbert?) in the discussion. I am not saying we should implement/design fencing for every use cases right now, just trying to minimize the amount of refactoring work needed in the future. In any case, it's gonna take me a few days to digest this, so i'll review this in several steps like the 1.3 version (luckily I still have some of the 1.3 version of the patch in my head so it should be easier this time). ::: gfx/layers/client/TextureClient.h @@ +348,5 @@ > + * During the deallocation phase, a TextureChild may hold its recently destroyed > + * TextureClient's data until the compositor side confirmed that it is safe to > + * deallocte or recycle the it. > + */ > +class TextureChild : public PTextureChild I'd like TextureChild to not be exposed outside of TextureClient (so, I want it to stay in TextureClient.cpp) This is to enforce that only TextureClient (and IPDL glue) ever own or fiddle with the PTexture actor. If you need to expose it in the header in order to handle EditReply::TReturnReleaseFence, please add a static TextureClient::AsTextureClient(PTextureChild* aActor) method instead, just like TextureHost does here: http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.h?from=TextureHost.h#395 I know that logically it is the same, but we have a history of wrongly manipulating IPDL actors by hand (cf. PGrallocBufferActor). So I'd rather be extra careful with PTexture and hide it as much as possible :)

(In reply to Nicolas Silva [:nical] from comment #11) > What is the timeline for this bug? > [...] cc Milan, see comment 11

It seem to be used for MWC's demo. So I want to commit this gonk specific implementation at first.

(In reply to Sotaro Ikeda [:sotaro] from comment #13) > It seem to be used for MWC's demo. So I want to commit this gonk specific > implementation at first. Therefore it seems necessary as soon as possible.

I tend to like to put off the platform independent implementation until other platform actually implements the fence mechanism.

Particular implementation first, and now, if it's ready. Once we need the second one, we do the proper abstraction.

+1 for comment 16

Apply the comment.

> ::: gfx/layers/client/TextureClient.h > @@ +348,5 @@ > > + * During the deallocation phase, a TextureChild may hold its recently destroyed > > + * TextureClient's data until the compositor side confirmed that it is safe to > > + * deallocte or recycle the it. > > + */ > > +class TextureChild : public PTextureChild > > I'd like TextureChild to not be exposed outside of TextureClient (so, I want > it to stay in TextureClient.cpp) > This is to enforce that only TextureClient (and IPDL glue) ever own or > fiddle with the PTexture actor. > > If you need to expose it in the header in order to handle > EditReply::TReturnReleaseFence, please add a static > TextureClient::AsTextureClient(PTextureChild* aActor) method instead, just > like TextureHost does here: > http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ > TextureHost.h?from=TextureHost.h#395 > > I know that logically it is the same, but we have a history of wrongly > manipulating IPDL actors by hand (cf. PGrallocBufferActor). So I'd rather be > extra careful with PTexture and hide it as much as possible :) Applied to the new patch.

Comment on attachment 8378681 [details] [diff] [review] patch part 1 ver 2 - change to gfx layers Review of attachment 8378681 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/GrallocTextureClient.cpp @@ +221,5 @@ > + if (mReleaseFenceHandle.IsValid()) { > + android::sp<Fence> fence = mReleaseFenceHandle.mFence; > + fence->waitForever("GrallocTextureClientOGL::Lock"); > + mReleaseFenceHandle = FenceHandle(); > + } nit: trailing space

Comment on attachment 8377853 [details] [diff] [review] patch part 3 - Change to OmxDecoder Review of attachment 8377853 [details] [diff] [review]: ----------------------------------------------------------------- I am not a good reviewer for this. Chris's review is enough I think.

Apply the comment.

https://hg.mozilla.org/integration/b2g-inbound/rev/612c8bdd4fe2

(In reply to Sotaro Ikeda [:sotaro] from comment #24) > https://hg.mozilla.org/integration/b2g-inbound/rev/612c8bdd4fe2 backed-out https://hg.mozilla.org/integration/b2g-inbound/rev/e27a0c0bb0a1

Fix gonk ics build failure.

Fix build failure on tryserver.