Gecko layers should honor hwc releaseFenceFd on master(b2g v1.4)

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla30
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed)

Details

Attachments

(6 attachments, 6 obsolete attachments)

2.18 KB, patch
Details | Diff | Splinter Review
73.70 KB, patch
Details | Diff | Splinter Review
4.79 KB, patch
sushilchauhan
: review+
Details | Diff | Splinter Review
8.61 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
5.67 KB, patch
nical
: review+
pchang
: review+
Details | Diff | Splinter Review
50.34 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
+++ 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.
Severity: major → normal
Priority: P1 → --
blocking-b2g: --- → 1.4?
Summary: Gecko layers should honor hwc releaseFenceFd on master → Gecko layers should honor hwc releaseFenceFd on on b2g v1.4(master)
Summary: Gecko layers should honor hwc releaseFenceFd on on b2g v1.4(master) → Gecko layers should honor hwc releaseFenceFd on master(b2g v1.4)
Status: NEW → ASSIGNED
Assignee: nobody → sotaro.ikeda.g
Depends on: 946720
Depends on: 959171
Depends on: 962101
Depends on: 971946
Posted patch path - handle android's fence (obsolete) — Splinter Review
Fix nits.
Attachment #8377829 - Attachment is obsolete: true
Fix nits.
Attachment #8377836 - Attachment is obsolete: true
Attachment #8377851 - Attachment description: patch - change to gfx layers → patch part 1 - change to gfx layers
Attachment #8377851 - Flags: review?(nical.bugzilla)
Attachment #8377852 - Flags: review?(sushilchauhan)
Attachment #8377853 - Flags: review?(nical.bugzilla)
Attachment #8377853 - Flags: review?(chris.double)
Attachment #8377857 - Flags: review?(pchang)
Attachment #8377857 - Flags: review?(nical.bugzilla)
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.
Attachment #8377853 - Flags: review?(chris.double) → review+
Blocks: 974152
Attachment #8377852 - Flags: review?(sushilchauhan) → review+
Blocks: 960372
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
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan)
+1 for comment 16
Apply the comment.
Attachment #8377851 - Attachment is obsolete: true
Attachment #8377851 - Flags: review?(nical.bugzilla)
Attachment #8378681 - Flags: review?(nical.bugzilla)
> ::: 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.
Attachment #8377857 - Flags: review?(pchang) → review+
Blocks: 950079
blocking-b2g: 1.4? → 1.4+
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
Attachment #8378681 - Flags: review?(nical.bugzilla) → review+
Attachment #8377857 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8377853 - Flags: review?(nical.bugzilla)
Apply the comment.
Attachment #8378681 - Attachment is obsolete: true
Attachment #8380634 - Flags: review+
Fix gonk ics build failure.
Attachment #8380634 - Attachment is obsolete: true
Attachment #8380769 - Flags: review+
Fix build failure on tryserver.
Attachment #8380769 - Attachment is obsolete: true
Attachment #8380850 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/77764c9a05b0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 977393
You need to log in before you can comment on or make changes to this bug.