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

RESOLVED FIXED in Firefox 30, Firefox OS v1.4

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 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
Sushil
: 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
(Assignee)

Description

4 years ago
+++ 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.
(Assignee)

Updated

4 years ago
Severity: major → normal
Priority: P1 → --
(Assignee)

Updated

4 years ago
blocking-b2g: --- → 1.4?
(Assignee)

Updated

4 years ago
Summary: Gecko layers should honor hwc releaseFenceFd on master → Gecko layers should honor hwc releaseFenceFd on on b2g v1.4(master)
(Assignee)

Updated

4 years ago
Summary: Gecko layers should honor hwc releaseFenceFd on on b2g v1.4(master) → Gecko layers should honor hwc releaseFenceFd on master(b2g v1.4)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Updated

4 years ago
Depends on: 946720
(Assignee)

Updated

4 years ago
Depends on: 959171
(Assignee)

Updated

4 years ago
Depends on: 962101
(Assignee)

Updated

4 years ago
Depends on: 971946
(Assignee)

Comment 1

4 years ago
Created attachment 8375825 [details] [diff] [review]
temporary patch - enable hwc on nexus-4
(Assignee)

Comment 2

4 years ago
Created attachment 8377829 [details] [diff] [review]
path - handle android's fence
(Assignee)

Comment 3

4 years ago
Created attachment 8377836 [details] [diff] [review]
rollup patch - handle android's fence

Fix nits.
Attachment #8377829 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 8377849 [details] [diff] [review]
rollup patch - handle android's fence

Fix nits.
Attachment #8377836 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
Created attachment 8377851 [details] [diff] [review]
patch part 1 - change to gfx layers
(Assignee)

Updated

4 years ago
Attachment #8377851 - Attachment description: patch - change to gfx layers → patch part 1 - change to gfx layers
(Assignee)

Comment 6

4 years ago
Created attachment 8377852 [details] [diff] [review]
patch part 2 - Change to HwcComposer2D
(Assignee)

Comment 7

4 years ago
Created attachment 8377853 [details] [diff] [review]
patch part 3 - Change to OmxDecoder
(Assignee)

Comment 8

4 years ago
Created attachment 8377857 [details] [diff] [review]
patch part 4 - Change to GonkNativeWindow
(Assignee)

Updated

4 years ago
Attachment #8377851 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8377852 - Flags: review?(sushilchauhan)
(Assignee)

Updated

4 years ago
Attachment #8377853 - Flags: review?(nical.bugzilla)
Attachment #8377853 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Attachment #8377857 - Flags: review?(pchang)
Attachment #8377857 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 9

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

Comment 10

4 years ago
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+
(Assignee)

Updated

4 years ago
Blocks: 974152

Updated

4 years ago
Attachment #8377852 - Flags: review?(sushilchauhan) → review+

Updated

4 years ago
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)
(Assignee)

Comment 13

4 years ago
It seem to be used for MWC's demo. So I want to commit this gonk specific implementation at first.
(Assignee)

Comment 14

4 years ago
(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.
(Assignee)

Comment 15

4 years ago
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)

Comment 17

4 years ago
+1 for comment 16
(Assignee)

Comment 18

4 years ago
Created attachment 8378681 [details] [diff] [review]
patch part 1 ver 2 - change to gfx layers

Apply the comment.
Attachment #8377851 - Attachment is obsolete: true
Attachment #8377851 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8378681 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 19

4 years ago
> ::: 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.

Updated

4 years ago
Attachment #8377857 - Flags: review?(pchang) → review+
(Assignee)

Updated

4 years ago
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+

Updated

4 years ago
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)
(Assignee)

Comment 22

4 years ago
Created attachment 8380634 [details] [diff] [review]
patch part 1 ver 3 - change to gfx layers (r=nical)

Apply the comment.
Attachment #8378681 - Attachment is obsolete: true
Attachment #8380634 - Flags: review+
(Assignee)

Comment 26

4 years ago
Created attachment 8380769 [details] [diff] [review]
patch part 1 ver 4 - change to gfx layers (r=nical)

Fix gonk ics build failure.
Attachment #8380634 - Attachment is obsolete: true
Attachment #8380769 - Flags: review+
(Assignee)

Comment 27

4 years ago
Created attachment 8380850 [details] [diff] [review]
patch part 1 ver 5 - change to gfx layers (r=nical)

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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Updated

4 years ago
Depends on: 977393
status-b2g-v1.4: --- → fixed
status-firefox28: --- → wontfix
status-firefox29: --- → wontfix
status-firefox30: --- → fixed
You need to log in before you can comment on or make changes to this bug.