Scrolling http://www.puppygames.net/blog/?p=1282 with either the trackpack or mousewhell but not the scrollbar. There's heavy flickering.

The GIF is recorded at a low framerate but the flickering I see is even more jarring. Notice how it's not the whole layer that's fickering as well. The part on the right doesn't flicker.

I don't see flickering on my machine, just checkerboarding. But I have the velocity bias set to zero. Can you try that and see whether it makes a difference?

I see the background lagging behind (due to a main-thread parallax effect), but no flickering.

I tried with 'apz.velocity_bias;0' and it still reproduces. I'll try to run a bisection.

Caused by OSX Basic.

I confirmed the problem on my mac.

(In reply to Sotaro Ikeda [:sotaro] from comment #7) > I confirmed the problem on my mac. The problem happened when e10s was enabled.

I also see the problem with BasicCompositor on win8, when tiling and e10s is enabled.

During scrolling I sometimes saw different tile's drawing. And when nglayout.debug.paint_flashing is set, I saw very frequent flashing.

(In reply to Sotaro Ikeda [:sotaro] from comment #10) > During scrolling I sometimes saw different tile's drawing. Tile's front buffer's recycling seems now work correctly.

attachment 8744152 [details] [diff] [review] addressed the problem on my Win8 and mac.

(In reply to Sotaro Ikeda [:sotaro] from comment #11) > (In reply to Sotaro Ikeda [:sotaro] from comment #10) > > During scrolling I sometimes saw different tile's drawing. > > Tile's front buffer's recycling seems now work correctly. In current gecko, front buffer is recycled at the end of Transaction. It does not wait end of the buffer's compositing on compositor side. There is a risk that client side does recycle and paint to compositing buffer. On BasicCompositor, it seems to happen easily, because compositing is very heavy.

With CompositorOGL, we copy the tile contents when we process the transaction, by uploading it to a GPU texture. With BasicCompositor, apparently we don't copy. So do we need to keep the texture locked, until compositing is done? It sounds like we have very similar requirements here to what we need in bug 1265824.

Tiling is supposed to handle this already, for TextureHosts that return false for HasIntermediateBuffer. All tiles passed into TiledLayerBufferComposite::UseTiles should have a read lock on them, and if HasIntermediateBuffer() is false, then we only unlock the tiles the composite *after* a new transaction arrives (with a new set of tiles). Tiles that exist in both the new set and the old set will have been read locked a second time by content, so this unlock still leaves them in a locked state. Any tiles that are no longer in the tiled buffer will have their final read lock released (once we've finished the next composite and are sure no access will be made to them by deferred GPU work) and will be available to the tile pool again. On the client side it looks like ComputeHasIntermediateBuffer should be determining that we don't have an intermediate buffer when using basic compositor.

Yea, on TileClient point of view, TextureClient seems to be handled correctly with gfxSharedReadLock. But this bug was out of TileClient management :(

Update nits.

Hmm, TransactionId is not always updated incrementally from ClientLayerManager point of view. Sometimes, it is reset to 1 and increment again. It needs to be always incremental.

It seems better to pick FwdTransactionId from Bug 1252835.

Add FwdTransacitonId.

Update nits.

(In reply to Sotaro Ikeda [:sotaro] from comment #17) > Yea, on TileClient point of view, TextureClient seems to be handled > correctly with gfxSharedReadLock. But this bug was out of TileClient > management :( Can you provide more details about what's wrong here? The tiling code is supposed to handle this through the copy-on-write lock mechanism. If that logic has bugs, I would prefer that we fix it rather than add new stuff to work around the issue. (In reply to Sotaro Ikeda [:sotaro] from comment #14) > In current gecko, front buffer is recycled at the end of Transaction. It > does not wait end of the buffer's compositing on compositor side. There is a > risk that client side does recycle and paint to compositing buffer. On > BasicCompositor, it seems to happen easily, because compositing is very > heavy. Shouldn't this take the state of the Copy-on-write lock into account? If the compositor hasn't unlocked it, we shouldn't try to recycle the texture (yet). My recollection of the details involved is a bit fuzzy...

(In reply to Nicolas Silva [:nical] from comment #24) > (In reply to Sotaro Ikeda [:sotaro] from comment #17) > > Yea, on TileClient point of view, TextureClient seems to be handled > > correctly with gfxSharedReadLock. But this bug was out of TileClient > > management :( > > Can you provide more details about what's wrong here? The tiling code is > supposed to handle this through the copy-on-write lock mechanism. If that > logic has bugs, I would prefer that we fix it rather than add new stuff to > work around the issue. When TileClient calls ReturnTextureClientDeferred(), it returned TextureClient to TextureClientPool and gfxSharedReadLock is unlocked and cleared. https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp#674 The gfxSharedReadLock is not referenced anymore on client side. Only Host side continues to hold ref of gfxSharedReadLock. Then TextureClientPool reuse the TextureClient after TextureClientPool::ReturnDeferredClients(). It does not care about gfxSharedReadLock to recycle TextureClient. ReturnDeferredClients() is called in ClientLayerManager::EndTransaction(). If compositor side delayed its task, the TextureClient could be still in use in Host side. > (In reply to Sotaro Ikeda [:sotaro] from comment #14) > > In current gecko, front buffer is recycled at the end of Transaction. It > > does not wait end of the buffer's compositing on compositor side. There is a > > risk that client side does recycle and paint to compositing buffer. On > > BasicCompositor, it seems to happen easily, because compositing is very > > heavy. > > Shouldn't this take the state of the Copy-on-write lock into account? If the > compositor hasn't unlocked it, we shouldn't try to recycle the texture > (yet). My recollection of the details involved is a bit fuzzy... As the above, the lock is not applied to TextureClient that ReturnTextureClientDeferred() is called.

attachment 8744326 [details] [diff] [review] did not use gfxSharedReadLock to address ReturnTextureClientDeferred(), since it might be better to limit the lock only to TileClient.

Ok, this makes sense to me now. When we're recycling buffers within a tile, the read lock system work. But sometimes we discard tile buffers and return those buffers to the pool while they should still be locked. The code in DiscardBackBuffer only returns the tile if we're already unlocked, and discards the buffer entirely if not. The simple solution would be to do the same for DiscardFrontBuffer. Ultimately it seems like it would be preferable to keep the buffers and locks together at all times, teach the pool about locking, and only recycle buffers that are unlocked.

(In reply to Matt Woodrow (:mattwoodrow) from comment #27) > > Ultimately it seems like it would be preferable to keep the buffers and > locks together at all times, teach the pool about locking, and only recycle > buffers that are unlocked. I am creating a patch to do it.

Fix build failures on android.

Need to address ShmemSection lifetime problem.

(In reply to Sotaro Ikeda [:sotaro ouf of office 29/Apr - 8/May] from comment #33) > Need to address ShmemSection lifetime problem. If you are talking about the mUsedShmems.empty() assertion, I am okay with r+ing a patch that just removes it. That bogus assertion is getting in the way of too much stuff and causes intermittent failures in cases where things are actually correct.

(In reply to Nicolas Silva [:nical] from comment #34) > If you are talking about the mUsedShmems.empty() assertion, I am okay with > r+ing a patch that just removes it. I put a patch on bug 1267246 that removes the assertion.

(In reply to Nicolas Silva [:nical] from comment #34) > (In reply to Sotaro Ikeda [:sotaro ouf of office 29/Apr - 8/May] from > comment #33) > > Need to address ShmemSection lifetime problem. > > If you are talking about the mUsedShmems.empty() assertion. test failures happened on mac. Address of mShmemSection seemed to become invalid when ReadCount became 0. gfxShmSharedReadLock::ReadUnlock() dealloc ShmemSection when the ReadCount became 0. It seemed to be affected to the failures. https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp#422

Fix ASSERT.

Comment on attachment 8745832 [details] [diff] [review] patch part 2 - Update ReadLock handling Forgot to update one asserts in TextureClientPool.

Fix assert.

I see a 3% talos improvement on osx for the main_rss measurement- thought I would mention it: https://treeherder.mozilla.org/perf.html#/alerts?id=1043

Hi Florin, can we have QA's help to verify this?

Setting verification flag.

Hi Sotaro, Since this patch also affects 48, are you also considering to uplift this patch to 48?

By Bug 1263053 comment 15, it does not affect 48. Then it is not necessary to uplift the patch to 48.

I can't reproduce this using 48 Nightly under Mac OS X 10.9.5. Are there any prefs that I need to set first or is this specific to some OS's? If that's the case, can you please confirm the fix using Firefox 49 RC build? Thank you!

According to the tracking flags it says that 48 is unaffected. Make sure 'layers.acceleration.disabled' is set to false and to restart the build on an affected build to reproduce the problem.

I was testing the Nightly from the date this bug was filled (2016-04-19) - this was 48 (as stated for Version in the top description). 48 was set as unaffected after a backout. 'layers.acceleration.disabled' preference is set to false by default. Even though I couldn't reproduce the flickering, I can see that on Firefox 49 RC the scrolling is improved (eg: on OSX the background image doesn't scroll anymore). Considering this, I'll remove the qe-verify+ flag and leave the status flag as it is.