Currently we're moving from mProducer -> mStaging -> mConsumer in the same frame. This means that from the time we go from the last gl command to the time we want to present we can have a little as 0ms before waiting on the GPU. I'm suggesting that we instead keep 3 buffers but introduce a one frame delay before moving things to mConsumer. This gives the GPU the time to work while we prepare the next frame on the GPU letting the CPU+GPU work together.

This fixes the stuck frame problem.

Comment on attachment 8428931 [details] [diff] [review] hiben.patch Review of attachment 8428931 [details] [diff] [review]: ----------------------------------------------------------------- Sorry this patch needs cleaning up, got it to work just before leaving work. Just ignore the printf and give me feedback. Wanted to avoid having another day of delay.

Comment on attachment 8429362 [details] [diff] [review] patch Review of attachment 8429362 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/SurfaceStream.cpp @@ +54,5 @@ > break; > + } > + case SurfaceStreamType::TripleBuffer: { > + bool delayFrame = false; > +#ifdef MOZ_WIDGET_GONK The better way to do this is with a bool ShouldDelayFrame() func, and put our (ifdef) logic in there. @@ +479,4 @@ > { > MonitorAutoLock lock(mMonitor); > if (mStaging) { > + if (!mConsumer || !mSwapDelay) { This func definitely needs more comments. The mental load here is pretty high. Invert this if/else order. It's easier to understand if we do: if (mConsumer && mSwapDelay) else // !(mConsumer && mSwapDelay) => (!mConsumer || !mSwapDelay), right? I would also recommend trying keeping mSwapDelay and !mSwapDelay as separate, instead of interleaving this extra variable in our swap logic. That way, it should be more obvious what we do in each case in isolation. @@ -464,1 @@ > Scrap(mConsumer); Why were we scrapping the already-null mConsumer? ::: gfx/gl/SurfaceStream.h @@ +56,5 @@ > + /** > + * If we have a pending frame in the queue then well need > + * another SwapConsumer. > + */ > + bool DelayedFrame() { HasDelayedFrame @@ +202,4 @@ > SharedSurface* mStaging; > SharedSurface* mConsumer; > SharedSurface* mDelay; > + bool mSwapDelay; Shouldn't this be const? Also probably 'mHas/UseSwapDelay'.

(In reply to Jeff Gilbert [:jgilbert] from comment #6) > Invert this if/else order. It's easier to understand if we do: > if (mConsumer && mSwapDelay) > else // !(mConsumer && mSwapDelay) => (!mConsumer || !mSwapDelay), right? Good suggestion. And it is true via De Morgan' laws, which is worth memorizing IMO for this sort of simplification: http://en.wikipedia.org/wiki/De_Morgan%27s_laws > > I would also recommend trying keeping mSwapDelay and !mSwapDelay as > separate, instead of interleaving this extra variable in our swap logic. > That way, it should be more obvious what we do in each case in isolation. > > @@ -464,1 @@ > > Scrap(mConsumer); > > Why were we scrapping the already-null mConsumer? I'm not sure but now it needs to happen to be safe.

Comment on attachment 8434337 [details] [diff] [review] patch v2 Review of attachment 8434337 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/SurfaceStream.cpp @@ +36,5 @@ > +{ > +#ifdef MOZ_WIDGET_GONK > + return true; > +#else > + return true; This should be false, this was meant for try where I wanted to make sure this configuration didn't regress anywhere.

(In reply to Benoit Girard (:BenWa) from comment #7) > (In reply to Jeff Gilbert [:jgilbert] from comment #6) > > Invert this if/else order. It's easier to understand if we do: > > if (mConsumer && mSwapDelay) > > else // !(mConsumer && mSwapDelay) => (!mConsumer || !mSwapDelay), right? > > Good suggestion. And it is true via De Morgan' laws, which is worth > memorizing IMO for this sort of simplification: > http://en.wikipedia.org/wiki/De_Morgan%27s_laws I remember these, but couldn't remember the name. Regardless, it's in our best interest to write code that's most obvious, and not expect people to be fluent in boolean logical transformations. :) > > > > > I would also recommend trying keeping mSwapDelay and !mSwapDelay as > > separate, instead of interleaving this extra variable in our swap logic. > > That way, it should be more obvious what we do in each case in isolation. > > > > @@ -464,1 @@ > > > Scrap(mConsumer); > > > > Why were we scrapping the already-null mConsumer? > > I'm not sure but now it needs to happen to be safe. I would like to try to remove this, since it's obviously wrong.

Comment on attachment 8434337 [details] [diff] [review] patch v2 Review of attachment 8434337 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/SurfaceStream.h @@ +56,5 @@ > + /** > + * If we have a pending frame in the queue then well need > + * another SwapConsumer. > + */ > + bool HasDelayedFrame() const { This would be better as a virtual, which we could then implement in a subclass as: virtual bool HasDelayedFrame() const MOZ_OVERRIDE { return !!mDelay; }

Backed out for B2G reftest failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/33ff7ab025d1 https://tbpl.mozilla.org/php/getParsedLog.php?id=41257257&tree=Mozilla-Inbound

We're thinking of taking this bug on 1.4 instead of bug 1001417

But I just realized this has reftest failures. So maybe bug 1001417 is better.

2 rAF: https://tbpl.mozilla.org/?tree=Try&rev=5a81349bbf3a 2 rAF + Timeout (just in case since our tests are so slow): https://tbpl.mozilla.org/?tree=Try&rev=de013b7c0c6d

BenWa, I have one question. attachment 8437184 [details] [diff] [review] delay one frame. How does the patch ensure the last rendered frame is always rendered to the screen? SurfaceStream is used also for SkiaGL. And I just thought that there might be a case that last rendered frame is not rendered to a display.

(In reply to Benoit Girard (:BenWa) from comment #0) > I'm suggesting that we instead keep 3 buffers but introduce a one frame > delay before moving things to mConsumer. Does this mean that there will be one added vsync worth of latency (16ms) incurred to graphics rendering?

(In reply to Sotaro Ikeda [:sotaro] from comment #20) This line here will make sure we display the next frame: + if (context->GL()->Screen()->HasDelayedFrame()) { + context->Invalidate(); + } (In reply to Jukka Jylänki from comment #21) > (In reply to Benoit Girard (:BenWa) from comment #0) > > I'm suggesting that we instead keep 3 buffers but introduce a one frame > > delay before moving things to mConsumer. > > Does this mean that there will be one added vsync worth of latency (16ms) > incurred to graphics rendering? Yes.

I can't say I'd understand the internal details of the compositor at all, but that sounds suboptimal. Is there any other way?

Yes, bug 1001417 another option. The tradeoff are much more complicated then one has delay, one does not however.

Note that we're probably moving towards the reality where everything is delayed by a frame as we do bug 987532. If we're going to miss the frames often enough, the though is that "missing" all of them gives you better uniformity and better overall experience.

I talked with Jeff and Sotaro today, for now we're considering bug 1001417 because we think it as a lower chance of regression. With the frame delay we risk regressions with DrawWindow, invalidation and webgl updates no longer syncing up with the same transaction as content. We will come back to do if there's excessive waiting in the compositor for the fence.

For now we're officially going with bug 1001417 instead of this approach. It performs much better then I anticipated. We should re-open this if it causes too much blocking on the compositor thread.