With APZ turned on the gallery visibility_monitor.js stuff creates rendering issues since the scroll position is not synced anymore with what is rendered on the screen. I also tend to think that because of bug 847223 and bug 915526 that we don't need visibility_monitor.js anymore. But it needs to be checked since I'm not sure what exactly we are doing in the JS code that may still cause us to consume some memory.

Timothy, I made a small Gaia patch today to remove our custom code for showing/hidden images on the fly in the gallery app (see attachment). Setting layout.imagevisibility.enabled to true/false does not change anything about memory in this case. So 2 questions: - The thumbnails in the gallery app are rendered using blobs. Could it be why we don't discard anything here ? - The list is a scrollable element inside the main page. When we are looking for images to be discarded, does images that are scrolled out of view inside a scrollable item are taken into account ?

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #1) > Created attachment 8339994 [details] [diff] [review] > use.native.image.discarding.patch > > Timothy, I made a small Gaia patch today to remove our custom code for > showing/hidden images on the fly in the gallery app (see attachment). > > Setting layout.imagevisibility.enabled to true/false does not change > anything about memory in this case. > > So 2 questions: > - The thumbnails in the gallery app are rendered using blobs. Could it be > why we don't discard anything here ? image visibility only deals with whether an image is decoded and kept in memory in decoded form. I don't think the fact that the images come from blobs would change anything here. > - The list is a scrollable element inside the main page. When we are > looking for images to be discarded, does images that are scrolled out of > view inside a scrollable item are taken into account ? image visibility should fine with scrollable elements like this. Have we broken down the memory into what it's being used for? It is definitely decoded image data? Or memory use from storing the blobs? (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #0) > With APZ turned on the gallery visibility_monitor.js stuff creates rendering > issues since the scroll position is not synced anymore with what is rendered > on the screen. image visibility happens on the main thread. So it gets updated when the scroll position gets updated, but it doesn't know about async scrolling until the async scrolling updates the scroll position to the main thread. image visibility isn't perfect, it could still use some tuning for b2g.

I'm going to wait a little bit to investigate this one more since with bug 942750 is it very hard to know if the visible lag comes from APZ or the app itself.

Blocking QC CS 1.4.

(In reply to Milan Sreckovic [:milan] from comment #4) > Blocking QC CS 1.4. No it is not - QC needs to nominate individual dependencies, not a giant meta bug, to determine if something is a FC or CS blocker for a release.

Vivien, since this is on Gaia side, let me know if gallery can hit the performance goals for 1.4 without this fixed.

(In reply to Milan Sreckovic [:milan] from comment #6) > Vivien, since this is on Gaia side, let me know if gallery can hit the > performance goals for 1.4 without this fixed. Hey Milan. I think it can't. That's basically sounds like a checkerboarding issue. I remove the visiblity_monitor.js relative calls to see what happens and there is still a lot of checkerboarding. I also dumped the layers tree to see if there is anything suspect here (see attachment) and most of the panels of the app are hidden with display: none.

Thanks - making 1.4+ based on comment 7. One more question :) - do you think this is on Gallery side, and there is some Gaia work that should be done, or is it a Gecko issue? If Gecko, we should make it Core/Graphics until we figure out where it belongs.

(In reply to Milan Sreckovic [:milan] from comment #9) > Thanks - making 1.4+ based on comment 7. One more question :) - do you > think this is on Gallery side, and there is some Gaia work that should be > done, or is it a Gecko issue? If Gecko, we should make it Core/Graphics > until we figure out where it belongs. I think this is Gecko. The app is pretty clear on the gaia side from my point of view.

Benoit, Mason, is profiling the next step?

If the problem is CPU load the profiler will/might catch it. But if there's logic causing the imagine decoding to throttle then we'll need to debug the delay. It certainly wouldn't hurt to start running through the items in this list -in order- first if we're CPU bound: https://wiki.mozilla.org/index.php?title=B2G/Performance/App_Performance_Validation

Vivien: I don't know whether this is relevant at all to this bug anymore, but I have filed bug 967089 for removing visibility monitor from gallery. I haven't done that yet, because Timothy says that the gecko image visibility stuff doesn't work for CSS background images, which is what gallery is using currently.

Quick note, panning here = scrolling. We don't overpaint here a lot and scrolling in the gallery app works pretty well, even on tarako for reference-workload-light. Profile: https://people.mozilla.org/~bgirard/cleopatra/#report=a746d2fe65958d22c742c3de021ab5e9f94151a4 Vivien notices lots of jank with his specific photo workload. Will retest with his workload.

(In reply to David Flanagan [:djf] from comment #13) > Vivien: I don't know whether this is relevant at all to this bug anymore, > but I have filed bug 967089 for removing visibility monitor from gallery. I > haven't done that yet, because Timothy says that the gecko image visibility > stuff doesn't work for CSS background images, which is what gallery is using > currently. I made a custom gallery that does not use visibility_monitor.js and I still checkerboarding. So I think the issue lives elsewhere. My understanding of the main issue between APZC and visibility_monitor.js is the way is it used in some other apps. Contacts for example change the dom on the fly, which creates huge reflows. But gallery only set/unset background image. I assume this is fine. BenWa ?

Profile up with Vivien's workload: https://people.mozilla.org/~bgirard/cleopatra/#report=1526257bc9e665b519cd09a147f789d011d6fb29 Checkerboards a lot more than the standard reference-workload light. A few interesting things: 1) We have a 39 ms layout::Flush which causes a large amount of checkerboarding. 2) We also spend some time in DrawBufferWithRotation. Last time I heard, we shouldn't be doing that. Is that still the case? Otherwise, we spend a lot of time in buffer allocations. We aren't over painting and we're never painting the camera / dotted square at the bottom of the app, which looks good.

(In reply to Mason Chang [:mchang] from comment #16) > Profile up with Vivien's workload: > > https://people.mozilla.org/~bgirard/cleopatra/ > #report=1526257bc9e665b519cd09a147f789d011d6fb29 > > Checkerboards a lot more than the standard reference-workload light. A few > interesting things: > > 1) We have a 39 ms layout::Flush which causes a large amount of > checkerboarding. > 2) We also spend some time in DrawBufferWithRotation. Last time I heard, we > shouldn't be doing that. Is that still the case? > > Otherwise, we spend a lot of time in buffer allocations. > > We aren't over painting and we're never painting the camera / dotted square > at the bottom of the app, which looks good. From this profile we're allocating gralloc every frame which is hurting performance but not a dominating bottleneck. We have code to prevent re-allocation for small changes in the buffer size. Looks like it's not working well. This should be investigated.

Yeah I was looking at that. I thought it was the display port resizing, so i tried a patch where we kept the displayport size the same, but that didn't help.

The grallocs are coming from bug 958727, where we try to reclaim some memory when the thebes textures shrink. Here's a profile with bug 958727 backed out on mozilla-central. https://people.mozilla.org/~bgirard/cleopatra/#report=827d0e1859ca1d97d32e8bcc25064f98d4385b27 We don't see the grallocs at every frame, so we waste some memory but it seems to help checkerboarding. Tiling should eliminate the need for bug 958727. Vivien, can you try with this patch in gecko and see if it helps you? If so, I'll back out 958727.

(In reply to Mason Chang [:mchang] from comment #20) > Created attachment 8382622 [details] [diff] [review] > gralloc.patch > > The grallocs are coming from bug 958727, where we try to reclaim some memory > when the thebes textures shrink. Here's a profile with bug 958727 backed out > on mozilla-central. > > https://people.mozilla.org/~bgirard/cleopatra/ > #report=827d0e1859ca1d97d32e8bcc25064f98d4385b27 > > We don't see the grallocs at every frame, so we waste some memory but it > seems to help checkerboarding. Tiling should eliminate the need for bug > 958727. Vivien, can you try with this patch in gecko and see if it helps > you? If so, I'll back out 958727. I update my Gecko this morning and the situation is way better. Actually not only for the gallery app but for all big lists for me (could be related to my dogfooding data again). It's still far from perfect as I can trigger checkerboarding pretty easily but I don't have big {{background-color}} screen that last for a few secs now. It could be some other Gecko changes as well that have made the situation better, but I want to think that grallocing less is the key here. Thanks for the quick backout Mason.

Just talked with Vivien. Not a lot left to do in the gallery app. Vivien is tweaking the image-rendering quality of the thumbnails to see if that helps. Otherwise, we'll leave this open for now as the general 'gallery is checkerboarding' bug, but nothing actionable at this point until bug 942750 is fixed.

I have a very hard time to checkerboard with the previous backout and this small one liner in the gallery app. It reduce the quality of thumbnails but that's probably not the most important for this app, as long as the normal image quality is preserved.

That's very exciting. Should we just land this css change?

(In reply to Milan Sreckovic [:milan] from comment #24) > That's very exciting. Should we just land this css change? I'm going to see if I can make a small lib to have the best of both world. Do not alter the quality while panning slowly, but do it while panning quickly. Then it may be reusable in other apps.

I'm also wondering, if we can't do it at the gaia level, maybe we can do it at the gecko level. If we're currently skating (scrolling quickly, see something like http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1338), we change the thumbnails to be worse quality for newly painted thumbnails and return it back to a high quality once the velocity slows down.

Comment on attachment 8383167 [details] [diff] [review] bug943845.patch It seems painful to do that in the platform for 1.4. I agree that when you scroll super fast and can't see the rendered content altering the quality sounds a good idea to me, but it won't change the rendering of the child, only how the layer is repainted in the parent as far as I understand. So this is still useful.

Comment on attachment 8383167 [details] [diff] [review] bug943845.patch Review of attachment 8383167 [details] [diff] [review]: ----------------------------------------------------------------- r+, but I don't understand the patch and have not tested it. Consider asking :patryk or someone else from visual design for a consult on the degraded thumbnail image quality if you are not able to create a library that restores the higher quality when not scrolling. (Here's the thing I don't understand. For this patch to work, I assume that -moz-crisp-edges must be a faster image resizing algorithm than the auto value. But actually preserving edges (which is what the spec says is required) when downsampling seems hard to do. So is this CSS value mis-named? Should there be a -moz-just-do-the-quickest-thing value?) Here's something else you could do to validate this fix. Gallery creates thumbnail images large enough to be displayed actual size in landscape mode. In portrait mode they have to be downsampled, but in landscape mode they should be displayed actual size. So if it is the downsample algorithm that is making a difference, I'd expect that you would find that scrolling works well in landscape even without this patch.

David here a small library that listens for scroll events and fire a custom event when scroll start/end. Applications can then alter content when scroll start or end. I have moved it into shared/ as I want to use it for music as well, but also I have some plan to freeze the statusbar icons from refreshing (like wifi search, telephony search, network activity, system download) while panning an application. The reasons for that is because those compete in the compositor for repainting and make checkerboarding easier.

Those are the relevant Gallery changes with this patch.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #29) > ... > plan to freeze the statusbar icons from refreshing (like wifi search, > telephony search, network activity, system download) while panning an > application. The reasons for that is because those compete in the compositor > for repainting and make checkerboarding easier. +1!

Comment on attachment 8384263 [details] scroll_listener.js Trying to turn the attachment into a patch so it is easier to review

Comment on attachment 8384263 [details] scroll_listener.js That doesn't work. Its not a patch.

Comment on attachment 8384263 [details] scroll_listener.js This code looks fine to me, but I have not actually attempted to run it. I have a couple of questions for you below and various suggestions. Some of the suggestions are about things like naming and API, and are only relevant if you land this file in shared/js/. I'm giving r+ but would like you to add a short comment at the top of the file explaining what it does. Also, I'm assuming that you won't land this until it has unit tests or an integration test for Gallery or another app that exercises it. >var ScrollListener = function() { If this file is going to create the ScrollListener, and is not going to define any API for it, consider just using an anonymous function so that there is no global variable defined. If you are going to define a global name for this, consider ScrollDetector (like GestureDetector) or ScrollMonitor (like VisibilityMonitor). "Listener" seems too passive for what this module does. > 'use strict'; > > var scrollTimeout = 0; Initializing this to 0 made me think that it was a number of milliseconds, so I was surprised to see this variable used to hold the setTimeout return value. > var scrollDelay = 400; This seems like something that might be tweaked. Define it as ScrollDetector.IDLE_TIME or something? Is 400ms related to any constant in the APZ code? If so, a comment would be good. > var isScrolling = false; > function handleScroll(e) { > if (isScrolling === false) { Why not just !isScrolling? > dispatchScrollStart(); > } > > window.clearTimeout(scrollTimeout); > scrollTimeout = window.setTimeout(dispatchScrollEnd, scrollDelay); > } > > function dispatchScrollStart() { > isScrolling = true; > dispatchCustomScrollEvent(); > } > > function dispatchScrollEnd() { > isScrolling = false; > dispatchCustomScrollEvent(); I'd set scrollTimeout to null here and test for non-null before clearing it. It probably works as is but seems like extra work to ask Gecko to clear a timeout that doesn't exist anymore. > } > > function dispatchCustomScrollEvent() { > window.dispatchEvent(new CustomEvent('scrollstatechanged', { Did you consider an scrollstart, scrollend event pair instead? We have mousedown/mouseup, keydown/keyup, touchstart/touchend. I think most developers would expect a pair of events here rather than a state changed event. This way, with a key parameter in event.detail, you can't bind this event to some function you already have written. You have to write a custom event handler to scrollstatechanged. window.addEventListener('scrollstatechanged', function(e) { handleScroll(e.detail); }); versus: window.addEventListener('scrollstart', handleScroll.bind(this, true)); window.addEventListener('scrollend', handleScroll.bind(this, false)); I like the latter, but don't think it is a big deal. > bubbles: true, Since you're dispatching on the window, I don't know what good bubbling does. > detail: isScrolling > })); > } > > window.addEventListener('mozbrowserasyncscroll', handleScroll, true); > window.addEventListener('scroll', handleScroll, true); I don't know anything about mozbrowserasyncscroll and when you would receive that event instead of a regular 'scroll' event, so I can't comment on that part of the code. Doesn't 'scroll' sometimes get fired on elements that have overflow:scroll set? Would this class be useful for containers other than the window? If so, consider passing an element parameter to the constructor. >}; > >new ScrollListener(); >

Comment on attachment 8384264 [details] [diff] [review] bug943845.v2.patch Review of attachment 8384264 [details] [diff] [review]: ----------------------------------------------------------------- r+, but see my comments below, and consider writing an integration test to cover this code and the scroll_listener.js module ::: apps/gallery/js/gallery.js @@ +1241,5 @@ > + > + > +// Change the thumbnails quality while a scroll is happening in order to > +// favor scroll performance. > +window.addEventListener('scrollstatechanged', function onScroll(e) { Please add a comment saying that the 'scrollstatechanged' event comes from shared/js/scroll_listener.js, since this won't be obvious to readers. @@ +1242,5 @@ > + > +// Change the thumbnails quality while a scroll is happening in order to > +// favor scroll performance. > +window.addEventListener('scrollstatechanged', function onScroll(e) { > + var thumbnails = document.getElementById('thumbnails'); There is already a global variable named thumbnails that you can use here. See line 48. No need to look it up again on each scroll. @@ +1243,5 @@ > +// Change the thumbnails quality while a scroll is happening in order to > +// favor scroll performance. > +window.addEventListener('scrollstatechanged', function onScroll(e) { > + var thumbnails = document.getElementById('thumbnails'); > + thumbnails.classList.toggle('scroll', e.detail); I'd prefer the classname 'scrolling' because it indicates that the class represents a transient UI state. A reader might think that the class 'scroll' applies to any scrollable element.

(In reply to David Flanagan [:djf] from comment #34) > Comment on attachment 8384263 [details] > scroll_listener.js > > This code looks fine to me, but I have not actually attempted to run it. I > have a couple of questions for you below and various suggestions. Some of > the suggestions are about things like naming and API, and are only relevant > if you land this file in shared/js/. Thanks for the suggestions. Much appreciated. > > I'm giving r+ but would like you to add a short comment at the top of the > file explaining what it does. Also, I'm assuming that you won't land this > until it has unit tests or an integration test for Gallery or another app > that exercises it. > Yep. I'm planning to write some tests for it obviously. I will address/answer the comment in a few.

Send a PR on github with nits addressed and tests: https://github.com/mozilla-b2g/gaia/pull/16834

(In reply to David Flanagan [:djf] from comment #34) > > Is 400ms related to any constant in the APZ code? If so, a comment would be > good. > The apz code is firing scroll events every 40ms (from http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#870), while mozbrowserasyncscroll events are fired every 100ms. I took a bigger value since there is often some small inactive time between 2 scrolls and I didn't want to fire the scrollend events too often. > > var isScrolling = false; > > function handleScroll(e) { > > if (isScrolling === false) { > > Why not just !isScrolling? > I usually prefer to write === false as it is easier to read imho. But I fixed it. > I like the latter, but don't think it is a big deal. > > > bubbles: true, > > Since you're dispatching on the window, I don't know what good bubbling does. > It allows events to be caught during the capture phase, and to be stopped from here. But I don't really need it so I removed it. > > detail: isScrolling > > })); > > } > > > > window.addEventListener('mozbrowserasyncscroll', handleScroll, true); > > window.addEventListener('scroll', handleScroll, true); > > I don't know anything about mozbrowserasyncscroll and when you would receive > that event instead of a regular 'scroll' event, so I can't comment on that > part of the code. > It's mostly some scroll events that are fired onto a <iframe mozbrowser> when it's content is scrolling. This is what I planned to use from the system app to listen for pan gestures on child processes. > Doesn't 'scroll' sometimes get fired on elements that have overflow:scroll > set? Would this class be useful for containers other than the window? If > so, consider passing an element parameter to the constructor. > It does, and they normally bubbles.

had to revert for suspicion of failing mochitest like https://tbpl.mozilla.org/php/getParsedLog.php?id=35657964&tree=B2g-Inbound

(In reply to Carsten Book [:Tomcat] from comment #40) > had to revert for suspicion of failing mochitest like > https://tbpl.mozilla.org/php/getParsedLog.php?id=35657964&tree=B2g-Inbound I will be pretty surprised if that related to those changes, but let's wait a bit and see.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #41) > (In reply to Carsten Book [:Tomcat] from comment #40) > > had to revert for suspicion of failing mochitest like > > https://tbpl.mozilla.org/php/getParsedLog.php?id=35657964&tree=B2g-Inbound > > I will be pretty surprised if that related to those changes, but let's wait > a bit and see. yeah seems you are right (sorry for the revert) and seems https://bugzilla.mozilla.org/show_bug.cgi?id=897727 is more likely for causing the test failure

(In reply to Carsten Book [:Tomcat] from comment #42) > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #41) > > (In reply to Carsten Book [:Tomcat] from comment #40) > > > had to revert for suspicion of failing mochitest like > > > https://tbpl.mozilla.org/php/getParsedLog.php?id=35657964&tree=B2g-Inbound > > > > I will be pretty surprised if that related to those changes, but let's wait > > a bit and see. > > yeah seems you are right (sorry for the revert) and seems > https://bugzilla.mozilla.org/show_bug.cgi?id=897727 is more likely for > causing the test failure No worries, and thanks for trying to keep the tree green! Relanded: https://github.com/vingtetun/gaia/commit/eebebd962bde48157ed545d668610e6bb525d13c