I'm increasingly feeling that not enabling image locking on B2G is causing more problems than it solves. Things like bug 1145284 occur because we don't have image locking. In this bug I'd like to enable image locking on B2G. We obviously introduced image locking for a reason, so it may be that this isn't possible right away. Downscale-during-decode should reduce the impact of enabling image locking, so if we can't get away with it now, we should try again once DDD is on by default.

Here's the patch; just a pref flip. Let's see how this looks on try before worrying about review.

Comment on attachment 8584939 [details] [diff] [review] Enable image locking on B2G. OK, try looks good, so I think we should give this a shot. Going ahead and requesting review.

Comment on attachment 8584939 [details] [diff] [review] Enable image locking on B2G. You'll probably wasn't to change the min expiration time too similar to desktop, one day is a long time.

(In reply to Timothy Nikkel (:tn) from comment #4) > You'll probably wasn't to change the min expiration time too similar to > desktop, one day is a long time. Yeah, we probably want to change that. But I think it'd be a good idea to do it in a separate bug. Both changes might cause regressions, so it'd be useful to land them separately.

Comment on attachment 8584939 [details] [diff] [review] Enable image locking on B2G. If we don't decrease the expiration then the only way images will get removed from memory is when we get a memory pressure event basically. So this change on it's own can only serve to increase memory use. If you want to land this alone to get data and then land the expiration change shortly after that that's okay. But without changing the expiration you basically cannot evaluation image locking.

(In reply to Timothy Nikkel (:tn) from comment #6) > Comment on attachment 8584939 [details] [diff] [review] > Enable image locking on B2G. > > If we don't decrease the expiration then the only way images will get > removed from memory is when we get a memory pressure event basically. Yeah. Nothing has changed there. > So > this change on it's own can only serve to increase memory use. If you want > to land this alone to get data and then land the expiration change shortly > after that that's okay. But without changing the expiration you basically > cannot evaluation image locking. Well, there are two things I want to evaluate right now: 1. Does enabling image locking cause OOMs? I don't think we need to drop the expiration time to evaluate this, since we rely on memory pressure to free unlocked image data on B2G right now. All we're doing is increasing the amount of image data that's locked. If that amount ends up being too much, landing this change will let us know. If it's not, then things should work fine, because we will continue to free unlocked image data due to memory pressure. 2. Does image locking fix bug 1145284? (It should.) When we land the change to decrease the expiration time, it will definitely increase the amount of free memory we have on B2G. But it may cause performance regressions, since it will greatly increase the frequency with which images have to be redecoded. That's why I want to evaluate that change separately. I'm planning to land this, and if it sticks for 2 or 3 days, land the patch to decrease the expiration time.

Comment on attachment 8584939 [details] [diff] [review] Enable image locking on B2G. NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Hard to choose a specific bug number. User impact if declined: Frequent image flashing in the B2G Gallery app, and in other situations where many images may be on the screen at once. This pref change makes us manage image memory on B2G using the same algorithm we use on Firefox Desktop and Firefox for Android, which is much smarter in situations involving memory pressure. Testing completed: On m-c for a month with no issues. Risk to taking this patch (and alternatives if risky): At this point, low risk. We really haven't seen any fallout from making this change. This is also just a pref flip, so backout is trivial if we do change our minds later. String or UUID changes made by this patch: None.

(In reply to Seth Fowler [:seth] from comment #10) > Comment on attachment 8584939 [details] [diff] [review] > Enable image locking on B2G. > > NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to > better understand the B2G approval process and landings. > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): Hard to choose a specific bug > number. > User impact if declined: Frequent image flashing in the B2G Gallery app, and > in other situations where many images may be on the screen at once. Can you give some more example's or solid STR or existing bug #'s to reference the user impact. Often seen that fallouts are easily to happen late in the game and want to be comfortable with the risk/reward here before taking it or will let it ride the train. > This > pref change makes us manage image memory on B2G using the same algorithm we > use on Firefox Desktop and Firefox for Android, which is much smarter in > situations involving memory pressure. > Testing completed: On m-c for a month with no issues. > Risk to taking this patch (and alternatives if risky): At this point, low > risk. We really haven't seen any fallout from making this change. This is > also just a pref flip, so backout is trivial if we do change our minds later. > String or UUID changes made by this patch: None.

(In reply to bhavana bajaj [:bajaj] from comment #11) > Can you give some more example's or solid STR or existing bug #'s to > reference the user impact. Please see bug 1145284 for an example with STR. (Please note that that bug *does* reproduce on 2.2, even though the early comments say it doesn't - see e.g. comment 9 and comment 16. There was some confusion while we were trying to find the regression range.) > Often seen that fallouts are easily to happen > late in the game and want to be comfortable with the risk/reward here before > taking it or will let it ride the train. Totally agreed. This has been on trunk for a month without causing a regression, and it's just a pref flip that gives B2G the same behavior as all our other products, so IMO it's relatively low risk.

Another app that we've seen flashing images in in the past is Wallpaper. I can't get this to happen with today's nightly 2.2 build, but if partners add more wallpapers and we don't uplift this bug, I wouldn't be surprised to see image flashing when the wallpaper app is invoked by activity from the settings app. That would be another low memory, high image situation. So I support Seth's request to uplift this pref setting to 2.2

(In reply to Seth Fowler [:seth] from comment #12) > (In reply to bhavana bajaj [:bajaj] from comment #11) > > Can you give some more example's or solid STR or existing bug #'s to > > reference the user impact. > > Please see bug 1145284 for an example with STR. (Please note that that bug > *does* reproduce on 2.2, even though the early comments say it doesn't - see > e.g. comment 9 and comment 16. There was some confusion while we were trying > to find the regression range.) > > > Often seen that fallouts are easily to happen > > late in the game and want to be comfortable with the risk/reward here before > > taking it or will let it ride the train. > > Totally agreed. This has been on trunk for a month without causing a > regression, and it's just a pref flip that gives B2G the same behavior as > all our other products, so IMO it's relatively low risk. Understood, thanks for clearing it up. If you need any help in QA verification please flag njpark for additional testing here.

I used B2G/edit_prefs.sh to set pref("image.mem.allow_locking_in_content_processes", true); which I think is the same as uplifting this patch. It seems to fix the flashing part of bug 1161734, but it does not fix the blank thumbnails part of that bug. And, unfortunately, it also makes bug 1163102 (which I just filed) manifest on 2.2. If we uplift, then I I think that 1163102 will have to become a 2.2+ blocker as well. This may still need to be uplifted, but note that it will take us temporarily from one blocker to two blockers.

(In reply to David Flanagan [:djf] from comment #15) > It seems to fix the flashing part of bug 1161734, but it does not fix the > blank thumbnails part of that bug. Yes, we still don't know what's causing the blank thumbnails. That's present on 2.2 and master. > And, unfortunately, it also makes bug 1163102 (which I just filed) manifest > on 2.2. If we uplift, then I I think that 1163102 will have to become a > 2.2+ blocker as well. I'm pretty sure that's already fixed as bug 1162282. > This may still need to be uplifted, but note that it will take us > temporarily from one blocker to two blockers. If we don't uplift this, then nothing can be done for flashing in the Gallery. =\ There simply is no other fix.