Closed
Bug 977880
Opened 10 years ago
Closed 10 years ago
Screen flicker in FFOS Settings app
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
People
(Reporter: diego, Assigned: sotaro)
References
()
Details
(Keywords: regression, Whiteboard: [CR 623404])
Attachments
(2 files, 10 obsolete files)
56 bytes,
text/plain
|
Details | |
32.47 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
STR: 1. Go to Settings -> Display -> Screen timeout 2. Select any of the options There is a distinct flicker that shows the wallpaper briefly. It is reproducible with HWC enabled and disabled.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Sotaro, I see this in the QRD8x26. Can you reproduce this as well?
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 3•10 years ago
|
||
Hmm, I tested on nexus-4 and nexus-5. But I can not reproducible the symptom. It might be similar to Bug 962152, not clear yet. By the way, I've heard that I am going to receive QRD8x26 soon.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 4•10 years ago
|
||
tiling(Bug 963073) might fix the problem.
Assignee | ||
Comment 5•10 years ago
|
||
From the analysis in Bug 977975, Compositor take too long time for OpenGL rendering. And tiling seems to mitigate the problem.
Diego, it is not related to Bug 977975. Instead, it is related to https://bugzilla.mozilla.org/show_bug.cgi?id=974152.
Reporter | ||
Comment 7•10 years ago
|
||
I can't reproduce this anymore. Looks like there was a mystery fix sometime in the past 4 days.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 8•10 years ago
|
||
It's back! I can see this issue even with the patch from bug 974152. The video flicker is gone so it it was probably never related to bug 974152. Sotaro, Can you reproduce this on v1.4 KK?
Status: RESOLVED → REOPENED
Flags: needinfo?(sotaro.ikeda.g)
Resolution: WORKSFORME → ---
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Diego Wilson [:diego] from comment #8) > It's back! I can see this issue even with the patch from bug 974152. The > video flicker is gone so it it was probably never related to bug 974152. > > Sotaro, > > Can you reproduce this on v1.4 KK? I can not reproduce the problem on nexus-5. I still did not receive QRD device... Last week, I was in Taipei, and heard that QC sent incorrect HW and needed to ask to send correct one.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 10•10 years ago
|
||
Diego, can you confirm that if the problem happens when tiling is disabled.
Flags: needinfo?(dwilson)
Reporter | ||
Comment 11•10 years ago
|
||
How do I disable tiling?
Flags: needinfo?(dwilson) → needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 12•10 years ago
|
||
Change "Layers:Enable Tiles" setting by Settings app and reboot the phone. Within "settings->Developer", there is "Layers:Enable Tiles".
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 13•10 years ago
|
||
Current tiling thebes layer does not care about fence. The layer is enabled on a scrollable layer.
Assignee | ||
Comment 14•10 years ago
|
||
I borrowed QRD8x26 from mikeh. The rom seems old. I am going to try to create a recent rom.
Reporter | ||
Comment 15•10 years ago
|
||
> Current tiling thebes layer does not care about fence. The layer is enabled on a scrollable layer.
You got it! The issue goes away if I disable tiling. I'm guessing there's still some fence work to be done there?
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Diego Wilson [:diego] from comment #15) > > Current tiling thebes layer does not care about fence. The layer is enabled on a scrollable layer. > > You got it! The issue goes away if I disable tiling. I'm guessing there's > still some fence work to be done there? Yea, fences delivery for tiling is necessary. I am going to work for it.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Updated•10 years ago
|
Keywords: regression
Assignee | ||
Updated•10 years ago
|
Blocks: b2g-tiling
Assignee | ||
Updated•10 years ago
|
Component: Graphics → Graphics: Layers
Comment 18•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #16) > (In reply to Diego Wilson [:diego] from comment #15) > > > Current tiling thebes layer does not care about fence. The layer is enabled on a scrollable layer. > > > > You got it! The issue goes away if I disable tiling. I'm guessing there's > > still some fence work to be done there? > > Yea, fences delivery for tiling is necessary. I am going to work for it. Do we need tiling to use hwcomposer for 1.4? It doesn't sound easy. IIRC, non-tiled thebes layers only use hwcomposer when unrotated, which doesn't happen often while scrolling.
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #18) > > Do we need tiling to use hwcomposer for 1.4? It doesn't sound easy. IIRC, > non-tiled thebes layers only use hwcomposer when unrotated, which doesn't > happen often while scrolling. HwComposer is used even for OpenGL composition case in JB/KK. OpenGL Composition result is composed to FrameBuffer layer. The layer is rendered by HwComposer. The layer's AcquireFence is used to prevent a tearing problem happen on OpenGL composition. It is done in Bug 974152.
Comment 20•10 years ago
|
||
Sotaro, can you give a brief overview on what happens when "Layers: Enable Tiles" is enabled. Does it mean, instead of having a single layer buffer, there are multiple tiles (what size?) of buffers for a layer. I believed it should affect only the Rendering phase. But now the sync fences are being used, so the acquire and release fence of a layer needs to take care of all tiles assigned to it (by dup?) in framework. How does it impact Composition phase ?
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Sushil from comment #20) > Sotaro, can you give a brief overview on what happens when "Layers: Enable > Tiles" is enabled. Does it mean, instead of having a single layer buffer, > there are multiple tiles (what size?) of buffers for a layer. Yes. Multiple tiles are used for a scrollable thebes layer. Therefore one layer has multiples of tiles. HwcComposer2D and fence handling implementation expects one buffer for one layer. > I believed it > should affect only the Rendering phase. But now the sync fences are being > used, so the acquire and release fence of a layer needs to take care of all > tiles assigned to it (by dup?) in framework. How does it impact Composition > phase ? About fence, need to extend CompositorOGL::SetFBAcquireFence() to handle tiles. And TiledContentHost(tiled thebes layer) uses different way of buffer swapping than ContentHost(thebes layer). Need to change TiledContentHost as to return fence to client side.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 23•10 years ago
|
||
I created the following diagrams. It might help to understand the difference between thebes layer and tiled thebes layer. - ClientThebesLayer (Firefox OS v1.4 gonk) https://github.com/sotaroikeda/firefox-diagrams/blob/master/gfx/gfx_ClientThebesLayer_FirefoxOS_1_4.pdf?raw=true - ClientTiledThebesLayer (Firefox OS v1.4 gonk) https://github.com/sotaroikeda/firefox-diagrams/blob/master/gfx/gfx_ClientTiledThebesLayer_FirefoxOS_1_4.pdf?raw=true - SimpleClientTiledThebesLayer (Firefox OS v1.4 gonk) https://github.com/sotaroikeda/firefox-diagrams/blob/master/gfx/gfx_SimpleClientTiledThebesLayer_FirefoxOS_1_4.pdf?raw=true - HwcComposer2D (Firefox OS v1.4 gonk JB) https://github.com/sotaroikeda/firefox-diagrams/blob/master/widget/widget_HwcComposer2D__FirefoxOS_1_4.pdf?raw=true https://github.com/sotaroikeda/firefox-diagrams/wiki/Firefox-Diagrams
Assignee | ||
Comment 24•10 years ago
|
||
Confirmed that release fences are delivered to child side. A way of tile recycle is same as default.
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8396118 [details] [diff] [review] wip patch - Handle ReleaseFence on tiled thebes layer nical, can I have a feedback to the patch?
Attachment #8396118 -
Flags: feedback?(nical.bugzilla)
Updated•10 years ago
|
Attachment #8396118 -
Flags: feedback?(nical.bugzilla) → feedback+
Updated•10 years ago
|
Attachment #8396118 -
Flags: feedback?(chrislord.net)
Comment 26•10 years ago
|
||
Comment on attachment 8396118 [details] [diff] [review] wip patch - Handle ReleaseFence on tiled thebes layer Review of attachment 8396118 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! All my comments are basically just 'document it better'. ::: gfx/layers/TiledLayerBuffer.h @@ +18,5 @@ > #include "nsRect.h" // for nsIntRect > #include "nsRegion.h" // for nsIntRegion > #include "nsTArray.h" // for nsTArray > > +#if MOZ_WIDGET_GONK && ANDROID_VERSION >= 17 Elsewhere, we would write the first bit as defined(MOZ_WIDGET_GONK). Not sure that it matters. @@ +211,5 @@ > */ > virtual const nsIntRegion& GetValidLowPrecisionRegion() const = 0; > + > +#if MOZ_WIDGET_GONK && ANDROID_VERSION >= 17 > + virtual void SetReleaseFence(const android::sp<android::Fence>& aReleaseFence) = 0; Documentation needed. ::: gfx/layers/client/TextureClient.h @@ +339,5 @@ > { > return mReleaseFenceHandle; > } > > + virtual void WaitReleaseFence() {} Documentation. ::: gfx/layers/client/TextureClientPool.cpp @@ +43,5 @@ > // Try to fetch a client from the pool > RefPtr<TextureClient> textureClient; > if (mTextureClients.size()) { > textureClient = mTextureClients.top(); > + textureClient->WaitReleaseFence(); I'll just mention so that it's on record (but I'm sure you're aware) - this will only stop us returning a TextureClient that has a fence at this point in time, but another user could easily use it after this point. We still need to make sure that we don't return clients that are in use (which we can currently tell by looking at the associated read-lock). ::: gfx/layers/composite/TextureHost.cpp @@ +261,5 @@ > } > > void > +TextureHost::CompositorRecycle() > +{ Trailing whitespace. ::: gfx/layers/composite/TextureHost.h @@ +286,5 @@ > static TemporaryRef<TextureHost> Create(const SurfaceDescriptor& aDesc, > ISurfaceAllocator* aDeallocator, > TextureFlags aFlags); > > + void CompositorRecycle(); Documentation. ::: gfx/layers/composite/ThebesLayerComposite.cpp @@ +92,5 @@ > ThebesLayerComposite::GetTiledLayerComposer() > { > + //MOZ_ASSERT(mBuffer && mBuffer->IsAttached()); > + if (!mBuffer || !mBuffer->IsAttached()) { > + return nullptr; Is this related? Do we want to NS_NOTREACHED here, or is it reasonable? If the latter, documentation. ::: gfx/layers/opengl/CompositorOGL.cpp @@ +1223,5 @@ > } > return; > } > > + // Set FBAcquireFence to tiles Although the pre-existing documentation is as bad, it'd be nice if this explained a bit better what's happening here... ::: gfx/layers/opengl/TextureHostOGL.h @@ +169,5 @@ > * Return a releaseFence's Fence and clear a reference to the Fence. > */ > virtual android::sp<android::Fence> GetAndResetReleaseFence(); > + > + virtual android::sp<android::Fence> GetReleaseFence(); Documentation.
Attachment #8396118 -
Flags: feedback?(chrislord.net) → feedback+
Assignee | ||
Comment 27•10 years ago
|
||
Thanks for the quick feedbacks! I am going to update the patch soon.
Assignee | ||
Comment 28•10 years ago
|
||
> ::: gfx/layers/composite/ThebesLayerComposite.cpp
> @@ +92,5 @@
> > ThebesLayerComposite::GetTiledLayerComposer()
> > {
> > + //MOZ_ASSERT(mBuffer && mBuffer->IsAttached());
> > + if (!mBuffer || !mBuffer->IsAttached()) {
> > + return nullptr;
>
> Is this related? Do we want to NS_NOTREACHED here, or is it reasonable? If
> the latter, documentation.
Want to return nullptr when ThebesLayerComposite is not ready for composition. I changed the code because GetTiledLayerComposer() is called from CompositorOGL::SetFBAcquireFence() even when mBuffer is nullptr.
Comment 29•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #28) > > ::: gfx/layers/composite/ThebesLayerComposite.cpp > > @@ +92,5 @@ > > > ThebesLayerComposite::GetTiledLayerComposer() > > > { > > > + //MOZ_ASSERT(mBuffer && mBuffer->IsAttached()); > > > + if (!mBuffer || !mBuffer->IsAttached()) { > > > + return nullptr; > > > > Is this related? Do we want to NS_NOTREACHED here, or is it reasonable? If > > the latter, documentation. > > Want to return nullptr when ThebesLayerComposite is not ready for > composition. I changed the code because GetTiledLayerComposer() is called > from CompositorOGL::SetFBAcquireFence() even when mBuffer is nullptr. Sounds fine - do we want to do something like: if (!mBuffer) { return; } MOZ_ASSERT(mBuffer->IsAttached()); though? Or can this be called with an unattached buffer?
Assignee | ||
Comment 30•10 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #29) > (In reply to Sotaro Ikeda [:sotaro] from comment #28) > > > ::: gfx/layers/composite/ThebesLayerComposite.cpp > > > @@ +92,5 @@ > > > > ThebesLayerComposite::GetTiledLayerComposer() > > > > { > > > > + //MOZ_ASSERT(mBuffer && mBuffer->IsAttached()); > > > > + if (!mBuffer || !mBuffer->IsAttached()) { > > > > + return nullptr; > > > > > > Is this related? Do we want to NS_NOTREACHED here, or is it reasonable? If > > > the latter, documentation. > > > > Want to return nullptr when ThebesLayerComposite is not ready for > > composition. I changed the code because GetTiledLayerComposer() is called > > from CompositorOGL::SetFBAcquireFence() even when mBuffer is nullptr. > > Sounds fine - do we want to do something like: > > if (!mBuffer) { > return; > } > MOZ_ASSERT(mBuffer->IsAttached()); > > though? Or can this be called with an unattached buffer? Thanks. The above seems fine. It should not be called with an unattached buffer.
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8396118 -
Attachment is obsolete: true
Assignee | ||
Comment 34•10 years ago
|
||
Hmm, by applying attachment 8396515 [details] [diff] [review] on nexus-5, video playback seems to have flickering. The symptom is same as Bug 981532 :-( On default nexus-5 ROM, I do not see the flicker.
Assignee | ||
Comment 35•10 years ago
|
||
After applying attachment 8396515 [details] [diff] [review], valid fence did not delivered to OMXCodec. In CompositorOGL::SetFBAcquireFence(), I put "return" in wrong place :-( By fixing it. Video regression is fixed. The following is a fixed version. ----------------------- TiledLayerComposer* composer = nullptr; LayerComposite* shadow = aLayer->AsLayerComposite(); if (shadow) { composer = shadow->GetTiledLayerComposer(); if (composer) { composer->SetReleaseFence(new android::Fence(GetGonkDisplay()->GetPrevFBAcquireFd())); return; } }
Assignee | ||
Comment 36•10 years ago
|
||
Fix video playback's flickering regression.
Attachment #8396515 -
Attachment is obsolete: true
Assignee | ||
Comment 37•10 years ago
|
||
Diego, can you check if attachment 8396597 [details] [diff] [review] fix the problem?
Flags: needinfo?(dwilson)
Assignee | ||
Updated•10 years ago
|
Attachment #8396597 -
Flags: review?(nical.bugzilla)
Attachment #8396597 -
Flags: review?(chrislord.net)
Comment 38•10 years ago
|
||
Comment on attachment 8396597 [details] [diff] [review] patch v4 - Handle ReleaseFence on tiled thebes layer Review of attachment 8396597 [details] [diff] [review]: ----------------------------------------------------------------- Good stuff, r+ with the comments addressed. None of the comments are critical imo. I'm unsure what the recycling bit was about, so I hope nical can give that more scrutiny, but it looked benign. Appreciate the additional documentation :) ::: gfx/layers/TiledLayerBuffer.h @@ +215,5 @@ > + /** > + * Store a fence that will signal when the current buffer is no longer being read. > + * Similar to android's GLConsumer::setReleaseFence() > + */ > + virtual void SetReleaseFence(const android::sp<android::Fence>& aReleaseFence) = 0; Because we're touching TiledLayerBuffer, you might want to get BenWa's approval for this - is there a way this could be on the implementation rather than on this interface? (i.e. on ClientTiledLayerBuffer) ::: gfx/layers/client/TextureClientPool.cpp @@ +43,5 @@ > // Try to fetch a client from the pool > RefPtr<TextureClient> textureClient; > if (mTextureClients.size()) { > textureClient = mTextureClients.top(); > + textureClient->WaitReleaseFence(); Note for self, this will require a change to my patch in bug 984618 if we decide to commit that. ::: gfx/layers/composite/TextureHost.h @@ +287,5 @@ > ISurfaceAllocator* aDeallocator, > TextureFlags aFlags); > > /** > + * Tell to TextureChild that TextureHost is recycled. I think this comment could be a bit better - I'm not really sure what recycled means in this context, or when it ought to be called. ::: gfx/layers/composite/TiledContentHost.cpp @@ +31,5 @@ > , mHasDoubleBufferedTiles(false) > , mUninitialized(true) > {} > > +static void RecycleCallback(TextureHost* textureHost, void* aClosure) { nits, static void should be on a separate line and the opening function brace should be on a separate line. ::: gfx/layers/opengl/CompositorOGL.cpp @@ +1218,5 @@ > if (visibleRegion.IsEmpty()) { > return; > } > > + // Set FBAcquireFence to ContainerLayer's childs s/to/on/ @@ +1227,5 @@ > } > return; > } > > + // Set FBAcquireFence as tiles' ReleaseFence to TiledLayerComposer. s/to/on/ ::: gfx/layers/opengl/TextureHostOGL.h @@ +175,5 @@ > + > + /** > + * Hold previous ReleaseFence to prevent Fence delivery failure via gecko IPC. > + * Fence is a kernel object and it's lifetime is managed by a reference count. > + * Until the Fence is delivered to client side, need to hold Fence on host side. nits, trailing space and s/it's/its/
Attachment #8396597 -
Flags: review?(chrislord.net) → review+
Reporter | ||
Comment 39•10 years ago
|
||
I can't reproduce the issue on the tip of v1.4. But then again, it has disappeared and reappeared before.
Flags: needinfo?(dwilson)
Assignee | ||
Comment 40•10 years ago
|
||
Apply the comments. Carry "r=cwiiis".
Attachment #8396597 -
Attachment is obsolete: true
Attachment #8396597 -
Flags: review?(nical.bugzilla)
Attachment #8396888 -
Flags: review?(nical.bugzilla)
Attachment #8396888 -
Flags: review?(bgirard)
Assignee | ||
Comment 41•10 years ago
|
||
Fix crash problem by changing TiledLayerBufferComposite::SetReleaseFence().
Attachment #8396903 -
Flags: review?(nical.bugzilla)
Attachment #8396903 -
Flags: review?(bgirard)
Assignee | ||
Updated•10 years ago
|
Attachment #8396888 -
Attachment is obsolete: true
Attachment #8396888 -
Flags: review?(nical.bugzilla)
Attachment #8396888 -
Flags: review?(bgirard)
Assignee | ||
Comment 42•10 years ago
|
||
Fix build failures on some platforms.
Attachment #8396903 -
Attachment is obsolete: true
Attachment #8396903 -
Flags: review?(nical.bugzilla)
Attachment #8396903 -
Flags: review?(bgirard)
Attachment #8397050 -
Flags: review?(nical.bugzilla)
Attachment #8397050 -
Flags: review?(bgirard)
Assignee | ||
Comment 43•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1ac39b093c22
Comment 44•10 years ago
|
||
This is not reproducible on Buri with/without Hardware Composer Enabled on master now Gaia 80af23f8c74d9d2e9388d8ed3c204040b5c528ec Gecko https://hg.mozilla.org/mozilla-central/rev/c69c55582faa BuildID 20140326040202 Version 31.0a1
Assignee | ||
Comment 45•10 years ago
|
||
On ICS device (like buri), this problem does not happen. genlock protect from the problem. The problem could happen only JB or newer gonk.
Assignee | ||
Comment 46•10 years ago
|
||
But even on nenxu-4, nexus-5, QRD devices, I could not reproduce the problem by myself.
Updated•10 years ago
|
Attachment #8397050 -
Flags: review?(nical.bugzilla) → review+
Comment 47•10 years ago
|
||
Comment on attachment 8397050 [details] [diff] [review] patch v7 - Handle ReleaseFence on tiled thebes layer (r=cwiiis) Review of attachment 8397050 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/SimpleTextureClientPool.cpp @@ +63,5 @@ > // Try to fetch a client from the pool > RefPtr<TextureClient> textureClient; > if (mAvailableTextureClients.size()) { > textureClient = mAvailableTextureClients.top(); > + textureClient->WaitReleaseFence(); Shouldn't we look for a texture client that doesn't require waiting for a fence and maybe going as far as allocating for a new object instead of waiting for a fence or get content we're not going to use re-use anyways? (Where just going to reuse the storage). If so we should have a comment on why we wait for the fence instead of doing something else. ::: gfx/layers/client/TextureClientPool.cpp @@ +43,5 @@ > // Try to fetch a client from the pool > RefPtr<TextureClient> textureClient; > if (mTextureClients.size()) { > textureClient = mTextureClients.top(); > + textureClient->WaitReleaseFence(); Similar to the other pool ::: gfx/layers/composite/TextureHost.cpp @@ +760,5 @@ > +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17 > + if (mTextureHost) { > + TextureHostOGL* hostOGL = mTextureHost->AsHostOGL(); > + android::sp<android::Fence> fence = hostOGL->GetAndResetReleaseFence(); > + if (fence.get() && fence->isValid()) { What happens if this fails? It looks from the code that we need to fence for Android >= 17 but if we don't get a valid fence we should pass a null fence handle. ::: gfx/layers/composite/TiledContentHost.h @@ +140,5 @@ > +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17 > + virtual void SetReleaseFence(const android::sp<android::Fence>& aReleaseFence); > +#endif > + > + // Recycle callback fro TextureHost. for ::: gfx/layers/opengl/CompositorOGL.cpp @@ +1206,5 @@ > void > CompositorOGL::SetFBAcquireFence(Layer* aLayer) > { > + // OpenGL does not provide ReleaseFence for rendering. > + // Instead use FBAcquireFence as layer buffer's ReleaseFence I have trouble understanding this code because FBAcquireFence isn't documented. ::: gfx/layers/opengl/GrallocTextureClient.cpp @@ +193,5 @@ > +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17 > + if (mReleaseFenceHandle.IsValid()) { > + android::sp<Fence> fence = mReleaseFenceHandle.mFence; > +#if ANDROID_VERSION == 17 > + fence->waitForever(1000, "GrallocTextureClientOGL::Lock"); Why do we only wait 1000 for Android v17 and not 18+?
Attachment #8397050 -
Flags: review?(bgirard) → review-
Comment 48•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #47) > Comment on attachment 8397050 [details] [diff] [review] > patch v7 - Handle ReleaseFence on tiled thebes layer (r=cwiiis) > > Review of attachment 8397050 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/client/SimpleTextureClientPool.cpp > @@ +63,5 @@ > > // Try to fetch a client from the pool > > RefPtr<TextureClient> textureClient; > > if (mAvailableTextureClients.size()) { > > textureClient = mAvailableTextureClients.top(); > > + textureClient->WaitReleaseFence(); > > Shouldn't we look for a texture client that doesn't require waiting for a > fence and maybe going as far as allocating for a new object instead of > waiting for a fence or get content we're not going to use re-use anyways? > (Where just going to reuse the storage). If so we should have a comment on > why we wait for the fence instead of doing something else. > > ::: gfx/layers/client/TextureClientPool.cpp > @@ +43,5 @@ > > // Try to fetch a client from the pool > > RefPtr<TextureClient> textureClient; > > if (mTextureClients.size()) { > > textureClient = mTextureClients.top(); > > + textureClient->WaitReleaseFence(); > > Similar to the other pool We only return clients to the pool that are imminently going to be released - this is rarely hit, and when it is, the cost of waiting is usually less than the cost of a new allocation (which involves a compositor round-trip). If we had other clients to reuse, we could reuse another instead of waiting though, there's certainly an argument to be made for that.
Assignee | ||
Comment 49•10 years ago
|
||
About "textureClient->WaitReleaseFence()", I, BenWa and jrmuizel talked about it. Even in ICS, genlock do the same thing in ICS(hamachi) implicitly. The genlock's wait happens when drawing. WaitReleaseFence() is a little bit sooner than genlock but. It is almost same timing. So, it should not make more harm than current ICS situation. We are agreed not to touch this code. As a separate bug, we are going to investigate, Which could get better performance between std::stack std::queue as BufferPool implementation. Current implementation uses stack.
Assignee | ||
Comment 50•10 years ago
|
||
Apply comments.
Attachment #8397050 -
Attachment is obsolete: true
Attachment #8397421 -
Flags: review?(bgirard)
Assignee | ||
Updated•10 years ago
|
Attachment #8397421 -
Flags: review?(bgirard)
Assignee | ||
Comment 51•10 years ago
|
||
Add one comment.
Attachment #8397421 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8397425 -
Flags: review?(bgirard)
Comment 52•10 years ago
|
||
Comment on attachment 8397425 [details] [diff] [review] patch v9 - Handle ReleaseFence on tiled thebes layer (r=cwiiis, nical) Review of attachment 8397425 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/TextureClient.cpp @@ +102,5 @@ > + if (aFence.type() != aFence.Tnull_t) { > + FenceHandle fence = aFence.get_FenceHandle(); > + if (fence.IsValid() && mTextureClient) { > + mTextureClient->SetReleaseFenceHandle(aFence); > + } Lets also write why failure handling isn't required here.
Attachment #8397425 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 53•10 years ago
|
||
Apply the comment.
Attachment #8397425 -
Attachment is obsolete: true
Attachment #8397488 -
Flags: review+
Assignee | ||
Comment 54•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/19bcfceeb477
Comment 55•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/19bcfceeb477
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 56•10 years ago
|
||
Has this made it to the "second" B2G build today? If so, and it can be tested and verified, let's uplift to aurora today.
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(npark)
Keywords: checkin-needed
Whiteboard: [CR 623404] → [CR 623404][checkin-needed-aurora]
Assignee | ||
Comment 57•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #56) > Has this made it to the "second" B2G build today? If so, and it can be > tested and verified, let's uplift to aurora today. What is second B2G build? There is no mozilla build ROM that can check this problem. The problem is only confirmed by diego's QRD device.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Updated•10 years ago
|
status-b2g-v1.4:
--- → affected
Updated•10 years ago
|
Flags: needinfo?(npark)
Comment 58•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b96db54cfaa8
status-b2g-v2.0:
--- → fixed
status-firefox29:
--- → wontfix
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Keywords: checkin-needed
Whiteboard: [CR 623404][checkin-needed-aurora] → [CR 623404]
Updated•10 years ago
|
Flags: in-moztrap?
Comment 59•10 years ago
|
||
Found Test Case: https://moztrap.mozilla.org/manage/case/3450/ STR needs to added to verify this bug to the existing test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
Flags: in-moztrap? → in-moztrap?(ychung)
Comment 60•10 years ago
|
||
Added an extra step and expected result to cover this issue in this moztrap test case https://moztrap.mozilla.org/manage/cases/?filter-id=3450
Flags: needinfo?(ktucker)
Flags: in-moztrap?(ychung)
Flags: in-moztrap+
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
You need to log in
before you can comment on or make changes to this bug.
Description
•