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)
Tracking
()
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 +++
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
It seems necessary to v1.3 like Bug 925444.
blocking-b2g: --- → 1.3?
Assignee | ||
Updated•10 years ago
|
Summary: Add release Fence handling to SurfaceStream → Add release Fence handling to SurfaceStream on gonk
Assignee | ||
Comment 2•10 years ago
|
||
For the time being, this bug handle only b2g v1.3 like Bug 925444.
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
SurfaceStream does not using TextureClient as buffer wrapper, therefore Texture client's ID can not be used in SurfaceStream now.
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
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?
Assignee | ||
Comment 7•10 years ago
|
||
Bug 957323 will deliver the release fence to TextureClient.
Updated•10 years ago
|
Assignee: nobody → sotaro.ikeda.g
Updated•10 years ago
|
blocking-b2g: 1.4? → 1.4+
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
(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/
Assignee | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
No longer depends on: KillSharedSurface
Assignee | ||
Comment 11•10 years ago
|
||
Current implementation still lacking buffer owner ship handling between content and compositor. It is going to be handled at Bug 974152.
Assignee | ||
Updated•10 years ago
|
Attachment #8384077 -
Flags: review?(nical.bugzilla)
Attachment #8384077 -
Flags: review?(jgilbert)
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Comment 13•10 years ago
|
||
Sounds good. We can wait for an actual bug, otherwise catch it in 1.5.
blocking-b2g: 1.4+ → 1.5?
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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-
Assignee | ||
Comment 16•10 years ago
|
||
un-bitrot and add wip sending fences from parent to child side.
Attachment #8384077 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 17•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Updated•10 years ago
|
blocking-b2g: 2.0? → 1.4?
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Fix nits.
Attachment #8418491 -
Attachment is obsolete: true
Attachment #8418960 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8418961 -
Attachment description: patch v4 - Add release Fence handling to SurfaceStream on gonk → roll up patch
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
>
> ::: 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?
Assignee | ||
Comment 24•10 years ago
|
||
(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].
Assignee | ||
Comment 25•10 years ago
|
||
Fix Comment 24.
Attachment #8418964 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8418961 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8419010 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 28•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=492142f8b352
Assignee | ||
Comment 29•10 years ago
|
||
Move WaitBufferOwnership() calling place to SurfaceStream::New().
Attachment #8418963 -
Attachment is obsolete: true
Attachment #8418963 -
Flags: review?(jgilbert)
Assignee | ||
Updated•10 years ago
|
Attachment #8420329 -
Flags: review?(jgilbert)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8419012 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8419010 -
Flags: review?(nical.bugzilla) → review+
Comment 31•10 years ago
|
||
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+
Assignee | ||
Comment 32•10 years ago
|
||
(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.
Assignee | ||
Comment 33•10 years ago
|
||
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+
Assignee | ||
Comment 35•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/b8c53d38745f
https://hg.mozilla.org/mozilla-central/rev/b8c53d38745f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 37•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1b071a4ff240
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox30:
--- → wontfix
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•