1167336 - [Video] Missing thumbnails will be present in the Video app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Description:
With over 200 videos loaded on the dut, the user notice that several missing thumbnails were present. If the user taps on a video to play it and then goes back to the main view, the thumbnail will be present.


Repro Steps:
1) Update a Flame to 20150521010203
2) Load 200+ videos on the dut.
3) Open the video app and allow all the videos to load.
4) Scroll through all the available videos.
5) Tap on a video that has a missing thumbnail and then tap the back button to go back to the main video view.

Actual:
Thumbnails will be randomly missing on the dut in the Video app. Once the video has been played, the thumbnail will appear.

Expected:
All thumbnails appear in the Video app without issue.

Environmental Variables:
Device: Flame 3.0 (Full Flash)(KK)(319mb)
Build ID: 20150521010203
Gaia: 5a7f87b1505ba89b586372cbbbe9507d1016c40c
Gecko: b9424d63fe35
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Repro frequency: 5/5 100%
See attached: Video, logcat

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue also occurs on the Flame 2.2

The user will notice missing thumbnails in the Video app.

Device: Flame 2.2(Full Flash)(KK)(319mb)
BuildID: 20150521002508
Gaia: bc42fbc12d622bffd7e8afcb8d56f8a1d9773c60
Gecko: 6e4eaf59efda
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

---------------------

This issue does not occur on the Flame 2.1

All thumbnails appear without issue in the Video app.

Device: Flame 2.1 (Full Flash)(KK)(319mb)
BuildID: 20150521001201
Gaia: c80865cb0bf73f1b97defbc646083b404feb3ac4
Gecko: ddb03c402946
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: MissingThumbnailsLogcat.txt

Comment 3

[KTucker [:KTucker][Inactive 3/4/2016]]

[Blocking Requested - why for this release]:

This is a regression from 2.1 so nominating this 2.2?

Comment 4

[David Flanagan [:djf]]

I think this is the issue discussed here: https://bugzilla.mozilla.org/show_bug.cgi?id=1161734#c39

That is: we need to modify the video app to use <img> element instead of background-image for displaying thumbnails.

I'm taking this and will try to get it fixed tomorrow.

Note that the equivalent bug for the Gallery app (bug 1161734) is 2.2+. This one is a less critical app and the bug is harder to trigger, I think, but this does seems like something we might want to block on.

Comment 5

[Pi Wei Cheng [:piwei] (inactive)]

Unable to find a regression window for this bug. I'm seeing different issues on older builds where some of the thumbnails would not appear at first, but as I scroll through the page the thumbnails start to appear; at times the thumbnails just start flashing after excessive scrolling, and eventually some of them display thumbnails, and some of them don't. Builds like this I'm not sure if I should consider them reproducing or not, and I'm not confident finding a window for this bug.

Comment 6

[David Flanagan [:djf]]

Pi-Wei: thanks for trying. I don't think we really need a regression window here, since this is a lot like 1161734 and I assume that Seth actually knows what's going on here.  The Gaia Video app does not make the most efficient use of image memory, and sometime realatively recently, Gecko changes landed that added new restrictions on the total amount of image memory an app could use. (This was, I assume, in order to improve memory management for the overall system, at the expense of some indidiual apps.)

Seth: is that an accurate description of what is going on? Do you have the bug number for the change that added the new image memory restrictions?

Comment 7

[Seth Fowler [:seth] [:s2h]]

There are two things that have changed.

(1) We now store all image textures in the SurfaceCache, which as you mention is much harsher about not going over the memory usage limits we've set via prefs than the old code was. This was necessary to prevent OOM issues, which we had *lots* of in the past. This change happened over many bugs but the majority of image textures moved into the SurfaceCache in bug 1060869.

(2) We're now locking image textures on B2G, as we do on all other platforms. That means that image textures that the system thinks are currently visible won't be discarded, even if we're low on memory. This was necessary to fix flickering issues in the Gallery and in other B2G apps. The change was made in bug 1148696.

Both of these changes are present in 2.2 and on master.

These changes are for the better, overall, but they do force us to actually fix bugs in how we manage image memory at the platform level, and while I know less about Gaia I'm sure they have exposed some issues on the Gaia side as well.

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1167336 > mozilla-b2g:master

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8609642 [details] [review]
[gaia] davidflanagan:bug1167336 > mozilla-b2g:master

Russ: this is a patch that is going to need to be uplifted to 2.2, so I need a review ASAP, please.

Note that Kevin's template patch means that I'll have to prepare a different PR for 2.2, and I might ask you to review them both.

This patch seems easier than I expected it to be. Am I missing anything here?

(Actually I probably broke some tests with the change... I'll fix anything busted before landing, but it is basically ready for review now.)

Comment 10

[David Flanagan [:djf]]

Sure enough. I broke a unit test. The PR has been updated to fix that. I'll wait for the automated test results before messing with the integration tests, though.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1167336-v2.2 > mozilla-b2g:v2.2

Comment 12

[David Flanagan [:djf]]

Comment on attachment 8609656 [details] [review]
[gaia] davidflanagan:bug1167336-v2.2 > mozilla-b2g:v2.2

Russ: this is the 2.2 version of the patch. It is almost the same as the other, but just different enough that it should probably have a separate review.

If it is not clear from the context of this bug, the point here is to use <img> instead of background-image because gecko can manage image memory better when we use <img>. And (I think) gecko has gotten more strict about not using too much image memory.

Comment 13

[David Flanagan [:djf]]

Comment on attachment 8609656 [details] [review]
[gaia] davidflanagan:bug1167336-v2.2 > mozilla-b2g:v2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): caused by a gecko patch. I think Seth probably knows which one, but I'm not sure.

[User impact] if declined: user with more than 100 videos on low-memory devices will not always be able to see the video thumbnails.

[Testing completed]: locally

[Risk to taking this patch] (and alternatives if risky): not very risky. We're just changing one way of displaying an image to another way.

[String changes made]: none

Comment 14

[David Flanagan [:djf]]

Jim,

Does the Music app use background-image to display album art? If so, then this bug is probably present for Music as well. If you've got 100+ albums on the device, then scrolling through the tiles view might show some missing album art.  If that is the case, we should switch to using <img> there as well.  Note that this affects 2.2

Comment 15

[David Flanagan [:djf]]

Seth: even with this patch applied, I noticed (in 2.2; haven't tested in 3.0) that if I just let it sit and scan 200 videos, then I'll end up with transiently missing thumbnails on the screen. They're not permanently black like I get in the gallery, but they are missing. This is one of those situations where there is no scrolling, but a lot of thumbnails that are created on screen and then pushed off screen when new thumbnails are added.  I mention this because we were discussing the possibility that gecko does not realize it can release the image memory in that case.

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to David Flanagan [:djf] from comment #15)
> Seth: even with this patch applied, I noticed (in 2.2; haven't tested in
> 3.0) that if I just let it sit and scan 200 videos, then I'll end up with
> transiently missing thumbnails on the screen. They're not permanently black
> like I get in the gallery, but they are missing. This is one of those
> situations where there is no scrolling, but a lot of thumbnails that are
> created on screen and then pushed off screen when new thumbnails are added. 
> I mention this because we were discussing the possibility that gecko does
> not realize it can release the image memory in that case.

Probably the same issue we have in the Gallery with too many images being locked, as discussed in bug 1161734.

I am still trying to narrow down the cause of that.

Comment 17

[Seth Fowler [:seth] [:s2h]]

That will be fixed in bug 1166136, by the way, so if you file a new bug about that, please make it depend on bug 1166136.

Comment 18

[Jim Porter (:squib)]

The music app uses background-image. I'm loathe to change that though, since we need it in order to automatically crop the images so they're square. Your patch doesn't seem to account for this case, so I'm assuming you're relying on everything having the right aspect ratio to begin with. We can't make that assumption in the music app, though.

Comment 19

[Josh Cheng [:josh]]

2.2+,
waiting for patch reviewed and land on master first.

Comment 20

[Russ Nicoletti [:russn]]

Comment on attachment 8609642 [details] [review]
[gaia] davidflanagan:bug1167336 > mozilla-b2g:master

The changes look good in terms of changing from using background image to <img> element. I have not tested the changes so I'm relying on David's (and Seth's) testing to determine if the changes produced the desired result.

Comment 21

[GitHub Autolander Bot]

Pull request has landed in v2.2: https://github.com/mozilla-b2g/gaia/commit/6a8d171d00efe8b27cba91bf1d789ab824579664

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Autolander doesn't care about approval flags, so please avoid using it on uplifts. Also, this should have landed on master first.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/820301a53c0dae52e7a77bba7a9bdee3e707199c

Comment 24

[Josh Cheng [:josh]]

Comment on attachment 8609656 [details] [review]
[gaia] davidflanagan:bug1167336-v2.2 > mozilla-b2g:v2.2

Approving and requesting QA verifyme.

Comment 25

[David Flanagan [:djf]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> Autolander doesn't care about approval flags, so please avoid using it on
> uplifts. Also, this should have landed on master first.

Ryan: thanks for cleaning this up. Looks like auto-lander doesn't know what to do if there is more than one attachment on a bug. I was wondering what it was going to do in this case. checkin-needed was set yesterday, presumably because I was out for memorial day and it was a 2.2+ bug.

Comment 26

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 3.0 and 2.2. No missing thumbnails appear in Video app. Also tried with excessive scrolling with no reproduction of bug.

However I found that on 3.0 it displays much fewer number of videos in the App. Using the same SD card, v2.2 lists 64 videos, and v30 only lists 42 videos. I'll look into bugging this separately. (I wish there's a Select All functionality in Video app so I can quickly count how many videos are listed)

Verified on:

Device: Flame (KK, full flashed, 319MB)
BuildID: 20150527010201
Gaia: 8ca93673869a64e09ed6153c5402896822dfb253
Gecko: ff2e07228041
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Device: Flame (KK, full flashed, 319MB)
BuildID: 20150527002504
Gaia: 8084264c4d1e28bc33220bc7443c7425bb76dbcc
Gecko: 19fcc06fb7ab
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 27

[Pi Wei Cheng [:piwei] (inactive)]

FYI, filed bug 1169024 for the issue observed while verifying.