Closed Bug 950079 Opened 10 years ago Closed 10 years ago

Add release Fence handling to SurfaceStream on gonk

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 10 obsolete files)

33.57 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
33.26 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #925444 +++
Depends on: KillSharedSurface
No longer depends on: 925444, 946720, 893301, 946328
It seems necessary to v1.3 like Bug 925444.
blocking-b2g: --- → 1.3?
Depends on: 925444
Summary: Add release Fence handling to SurfaceStream → Add release Fence handling to SurfaceStream on gonk
For the time being, this bug handle only b2g v1.3 like Bug 925444.
To deliver Fence object to correct gralloc buffer. gecko need a way to identify gralloc buffer from CompositableParentManager. In b2g v1.3, deprecated texture is used for WebGL rendering. new texture had a ID to identify a Texture client and most recent mater can identify TextureClient by using PTexture protocol(Bug 897452). PTexture is very risky change and can not uplift to b2g v1.3.
SurfaceStream does not using TextureClient as buffer wrapper, therefore Texture client's ID can not be used in SurfaceStream now.
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> SurfaceStream does not using TextureClient as buffer wrapper, therefore
> Texture client's ID can not be used in SurfaceStream now.

Somehow need to add TextureClient to SurfaceStream. Bug 941390 is going to do it. But it seems too big change for b2g v1.3. More smaller change seems better for it.
Bug 893304 change to use new texture on canvas layer but it causes bug 950589 now and soon backed out.
blocking-b2g: 1.3? → 1.4?
Depends on: 957323
Bug 957323 will deliver the release fence to TextureClient.
Assignee: nobody → sotaro.ikeda.g
blocking-b2g: 1.4? → 1.4+
(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Created attachment 8384077 [details] [diff] [review]
> patch - Add release Fence handling to SurfaceStream on gonk

Function name comes from android's acquireFence releaseFence. From jrmuizel's comment, their names seems to come from "C++11 standard's Acquire and Release Fences".
http://preshing.com/20130922/acquire-and-release-fences/
attachment 8384077 [details] [diff] [review] handles android fence object. But current SurfaceStream does not care about buffer usage end in compositor side. Therefore attachment 8384077 [details] [diff] [review] does not work as expected.
Depends on: 974152
No longer depends on: KillSharedSurface
Current implementation still lacking buffer owner ship handling between content and compositor. It is going to be handled at Bug 974152.
Attachment #8384077 - Flags: review?(nical.bugzilla)
Attachment #8384077 - Flags: review?(jgilbert)
(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> Current implementation still lacking buffer owner ship handling between
> content and compositor. It is going to be handled at Bug 974152.

I rethink about it. Right now, there are no canvas's tearing bug on gonk like Bug 962152. Therefore it might be better to put off handle this to MozSurface. By the change, Bug 974152 can focus to video rendering problem.
No longer depends on: 974152
Sounds good.  We can wait for an actual bug, otherwise catch it in 1.5.
blocking-b2g: 1.4+ → 1.5?
Comment on attachment 8384077 [details] [diff] [review]
patch - Add release Fence handling to SurfaceStream on gonk

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

I did a rather quick review because I am not a SurfaceStream expert, the few TextureClient-y part of things look good, I'll let Jeff worry about the details of SurfaceStream's logic

::: gfx/gl/SharedSurfaceANGLE.h
@@ +69,5 @@
>      virtual void LockProdImpl();
>      virtual void UnlockProdImpl();
>  
> +    virtual void FenceAcquire();
> +    virtual void FenceRelease() {}

Please add MOZ_OVERRIDE when applicable.
Attachment #8384077 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8384077 [details] [diff] [review]
patch - Add release Fence handling to SurfaceStream on gonk

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

::: gfx/gl/SharedSurface.h
@@ +77,5 @@
>      virtual void LockProdImpl() = 0;
>      virtual void UnlockProdImpl() = 0;
>  
> +    virtual void FenceAcquire() = 0;
> +    virtual void FenceRelease() = 0;

Fence and WaitSync are fairly self-explanatory for those who've fiddled with GL fences before, but I don't know what FenceAcquire and FenceRelease are. I need a brief overview of what these do, (generally and specifically for us!) and it needs to go in a comment here.

I read the article on Acquire and Release fences, very cool. However, our Fence calls are not FenceAcquire calls. FenceSync fences are full synchronization points (prevents reordering of previous read/writes with subsequent read/writes), which we poll/wait-on with (Client)WaitSync.

That said, we actually do want Acquire-Fence behavior (eventually), though we don't do any of this sort of synchronization currently. 

From the point of view of the producer of a ShSurf, they want to:
AcquireFence (prevents the compositor from reading subsequent writes)
draw
Issue a FenceSync full sync point fence
//ReleaseFence not really needed, since we it's a subset of a full sync.

I believe that ShSurf's AcquireFence needs to call Gralloc's ReleaseFence, since we need to make sure that our writes are barriered from being reordered before compositor reads (or writes, though the compositor doesn't write).

I think we need to name these functions more specifically, with respect to whether they're from a Producer or Consumer point of view. If the producer has to call the consumer's ReleaseFence, it should call it in the *producer's AcquireFence*.

TL;DR:
Fence should stay Fence. The mechanics are different from AcquireFence.
FenceRelease should become ConsReleaseFence, or similar.

ReleaseFence (

::: gfx/gl/SharedSurfaceEGL.h
@@ +69,5 @@
>      virtual void UnlockProdImpl() {}
>  
>  
> +    virtual void FenceAcquire();
> +    virtual void FenceRelease() {}

MOZ_OVERRIDE

::: gfx/gl/SharedSurfaceGL.h
@@ +150,5 @@
>      virtual void UnlockProdImpl() {}
>  
>  
> +    virtual void FenceAcquire();
> +    virtual void FenceRelease() {}

MOZ_OVERRIDE

@@ +230,5 @@
>      virtual void UnlockProdImpl() {}
>  
>  
> +    virtual void FenceAcquire();
> +    virtual void FenceRelease() {}

MOZ_OVERRIDE

::: gfx/gl/SharedSurfaceGralloc.h
@@ +65,5 @@
>  public:
>      virtual ~SharedSurface_Gralloc();
>  
> +    virtual void FenceAcquire();
> +    virtual void FenceRelease();

MOZ_OVERRIDE

::: gfx/gl/SharedSurfaceIO.h
@@ +24,5 @@
>      virtual void LockProdImpl() { }
>      virtual void UnlockProdImpl() { }
>  
> +    virtual void FenceAcquire();
> +    virtual void FenceRelease() {}

MOZ_OVERRIDE

::: gfx/gl/SurfaceFactory.cpp
@@ +27,5 @@
>      while (!mScraps.empty()) {
>          SharedSurface* cur = mScraps.front();
>          mScraps.pop();
> +        if (cur->Size() == size) {
> +            cur->FenceRelease();

Why here?

::: gfx/layers/opengl/GrallocTextureClient.h
@@ -62,5 @@
>  
>    virtual void SetReleaseFenceHandle(FenceHandle aReleaseFenceHandle) MOZ_OVERRIDE;
>  
> -  const FenceHandle& GetReleaseFenceHandle() const
> -  {

`{` should probably be on the previous line.
Attachment #8384077 - Flags: review?(jgilbert) → review-
un-bitrot and add wip sending fences from parent to child side.
Attachment #8384077 - Attachment is obsolete: true
Attachment #8418491 - Attachment description: patch v2 - Add release Fence handling to SurfaceStream on gonk → wip patch v2 - Add release Fence handling to SurfaceStream on gonk
attachment 8418491 [details] [diff] [review] add handling release Fence to SurfaceStream. But it is not an ideal solution. Even when fixed, there is no mechanism to handle buffer ownership correctly between SurfaceStream and CanvasClient. and there is a risk to miss Fence delivery before TextureCliet is reused by SurfaceStream. It seems better to be handled by a new bug.
Blocks: 1006957
Status: NEW → ASSIGNED
blocking-b2g: 2.0? → 1.4?
Attached patch roll up patch (obsolete) — Splinter Review
Fix nits.
Attachment #8418491 - Attachment is obsolete: true
Attachment #8418960 - Attachment is obsolete: true
Attachment #8418961 - Attachment description: patch v4 - Add release Fence handling to SurfaceStream on gonk → roll up patch
Comment on attachment 8418963 [details] [diff] [review]
patch part 1 - Change to SurfaceStream

jgilbert, can you review the patch soon? This bug blocks b2g 1.4 blocker. Thanks.
Attachment #8418963 - Flags: review?(jgilbert)
> 
> ::: gfx/gl/SurfaceFactory.cpp
> @@ +27,5 @@
> >      while (!mScraps.empty()) {
> >          SharedSurface* cur = mScraps.front();
> >          mScraps.pop();
> > +        if (cur->Size() == size) {
> > +            cur->FenceRelease();
> 
> Why here?

I want to defer the sync wait as long as possible. Is there other correct place to wait?
(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> Created attachment 8418961 [details] [diff] [review]
> roll up patch
> 
> Fix nits.

attachment 8418961 [details] [diff] [review] fixes bug 1006164.

But it regressed youtube video playback. It failed to deliver Fences for OMXCodec in correct timing. Some changes in attachment 8418961 [details] [diff] [review] has a dependency to Bug 984434. But the change is relatively big and risky for b2g v1.4. I am going to recover some functions that removed by attachment 8418961 [details] [diff] [review].
Fix Comment 24.
Attachment #8418964 - Attachment is obsolete: true
Attached patch roll up patch (obsolete) — Splinter Review
Attachment #8418961 - Attachment is obsolete: true
Attachment #8419010 - Flags: review?(nical.bugzilla)
Blocks a blocker.
blocking-b2g: 1.4? → 1.4+
Move WaitBufferOwnership() calling place to SurfaceStream::New().
Attachment #8418963 - Attachment is obsolete: true
Attachment #8418963 - Flags: review?(jgilbert)
Attachment #8420329 - Flags: review?(jgilbert)
Attached patch roll up patch (obsolete) — Splinter Review
Attachment #8419012 - Attachment is obsolete: true
Attachment #8419010 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8420329 [details] [diff] [review]
patch part 1 v2 - Change to SurfaceStream

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

The code is fine, but it NEEDS some comments.

::: gfx/gl/SharedSurface.h
@@ +79,5 @@
>  
>      virtual void Fence() = 0;
>      virtual bool WaitSync() = 0;
>  
> +    virtual void WaitBufferOwnership() {}

Leave a comment on what this function does.
Also, "WaitForBufferOwnership" is a better name, though still not quite right even still.

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +156,5 @@
> +
> +void
> +SharedSurface_Gralloc::WaitBufferOwnership()
> +{
> +  mTextureClient->WaitReleaseFence();

4-space indent here.

::: gfx/gl/SurfaceStream.cpp
@@ +80,5 @@
>      MOZ_ASSERT(!surf);
>      surf = factory->NewSharedSurface(size);
>  
> +    if (surf) {
> +        surf->WaitBufferOwnership();

This NEEDS a comment regarding why it's here.
Attachment #8420329 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #31)
> Comment on attachment 8420329 [details] [diff] [review]
> patch part 1 v2 - Change to SurfaceStream
> 
> Review of attachment 8420329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code is fine, but it NEEDS some comments.
> 
> ::: gfx/gl/SharedSurface.h
> @@ +79,5 @@
> >  
> >      virtual void Fence() = 0;
> >      virtual bool WaitSync() = 0;
> >  
> > +    virtual void WaitBufferOwnership() {}
> 
> Leave a comment on what this function does.
> Also, "WaitForBufferOwnership" is a better name, though still not quite
> right even still.

I will add a comment in a next patch.

> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +156,5 @@
> > +
> > +void
> > +SharedSurface_Gralloc::WaitBufferOwnership()
> > +{
> > +  mTextureClient->WaitReleaseFence();
> 
> 4-space indent here.

Will update.

> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +80,5 @@
> >      MOZ_ASSERT(!surf);
> >      surf = factory->NewSharedSurface(size);
> >  
> > +    if (surf) {
> > +        surf->WaitBufferOwnership();
> 
> This NEEDS a comment regarding why it's here.

Will add a comment.
Apply the comments. Carry "r=jgilbert,nical".
Attachment #8419010 - Attachment is obsolete: true
Attachment #8420329 - Attachment is obsolete: true
Attachment #8420345 - Attachment is obsolete: true
Attachment #8422754 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b8c53d38745f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
See Also: → 1000640
See Also: → 1142071
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: