1011460 - [Flame] Wallpaper picker flickering

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

Looks like bug 972442.

I've been reproducing this for a while on a bunch of devices: Nexus S, Desire Z, and recently on Flame. 100% reproductibility.

In my case, it flickers one and then all wallpapers are just not visible.

Comment 1

[Julien Wajsberg [:julienw]]

This works fine on my Buri master but I reproduce on my Peak v1.4.

Could it be happening on HD devices only?

Comment 2

[Stephanie Ouillon [:arroway] (needinfo me)]

I can reproduce it on a flame (2.0.0-prerelease), but it works fine on my keon (v1.4).

Comment 3

[:gerard-majax]

If anyone could try with an Open C, with v1.3 and v1.4.

Comment 4

[Joshua Mitchell (Inactive)]

This issue DOES reproduce on the 1.4 Open C
Environmental Variables:
Device: Open_C
BuildID: 20140515183003
Gaia: 32fca83da31b9a0f9a5a88f96c913a25accdc14b
Gecko: a1e455367fa6
Version: 30.0
Firmware Version: P821A10V1.0.0B06_LOG_DL

This issue DOES NOT reproduce on the 1.3 Open C
Environmental Variables:
Device: Open_C 
BuildID: 20140410144939
Gaia: acc6a85492e730133fbd79c6529882734bba3e02
Gecko: Unknown
Version: 28.0
Firmware Version: P821A10V1.0.0B06_LOG_DL

Comment 5

[Joshua Mitchell (Inactive)]

B2G Inbound Regression Window:

Last Working:
Environmental Variables:
Device: Flame 
BuildID: 20140515113003
Gaia: 7973e06dc278f67b4109ac3c33020ed086f0d042
Gecko: 15af5b93c0ea
Version: 32.0a1
Firmware Version: v10F-

First Broken:
Environmental Variables:
Device: Flame 
BuildID: 20140515143028
Gaia: c0d92f9e1c0580208ee83a7f2db01e56c2c21490
Gecko: 4b08c59bde94
Version: 32.0a1
Firmware Version: v10F-3

Last Working Gaia First Broken Gecko: Issue does NOT reproduce
Gaia: 7973e06dc278f67b4109ac3c33020ed086f0d042
Gecko: 4b08c59bde94

First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: c0d92f9e1c0580208ee83a7f2db01e56c2c21490
Gecko: 15af5b93c0ea

Gaia pushlog:https://github.com/mozilla-b2g/gaia/compare/7973e06dc278f67b4109ac3c33020ed086f0d042...c0d92f9e1c0580208ee83a7f2db01e56c2c21490

Comment 6

[Jason Smith [:jsmith]]

The range seems off here - the commit in the range seems unrelated to the problem here.

Can we get a video here?

Comment 7

[Jason Smith [:jsmith]]

Switching to regression range wanted to double check the range as well.

Comment 8

[Duane Dixon [:ddixon]]

Confirming the same test results were found Comment 5. Last working build, first broken build had the same pushlog and it is confirmed to be a Gaia issue. 

Gaia pushlog:https://github.com/mozilla-b2g/gaia/compare/7973e06dc278f67b4109ac3c33020ed086f0d042...c0d92f9e1c0580208ee83a7f2db01e56c2c21490


1.4 Regression Window 

Last working: 

Environmental Variables:
Device: Flame 1.4
BuildID: 20140515123001
Gaia: 8806eebe8f1555b1f924166f921ab831a5e3560c
Gecko: 366af94890cf
Version: 30.0
Firmware Version: v10F-3

First Broken: 

Environmental Variables:
Device: Flame 1.4
BuildID: 20140515183003
Gaia: 32fca83da31b9a0f9a5a88f96c913a25accdc14b
Gecko: a1e455367fa6
Version: 30.0
Firmware Version: v10F-3

Last Working Gecko First broken Gaia: 
Issue DOES occur here. 
Gaia: 32fca83da31b9a0f9a5a88f96c913a25accdc14b
Gecko: 366af94890cf

Last Working Gaia First broken Gecko: 
Issues DOES NOT occur here. 
Gaia: 8806eebe8f1555b1f924166f921ab831a5e3560c
Gecko: a1e455367fa6

Gaia Pushlog: 

https://github.com/mozilla-b2g/gaia/compare/8806eebe8f1555b1f924166f921ab831a5e3560c...32fca83da31b9a0f9a5a88f96c913a25accdc14b

Comment 9

[Jason Smith [:jsmith]]

This is similar to the problem seen in bug 1008586, which regressed due to switching Flame over to 1.5 pixels to px in bug 1010156.

Hema - Can you find someone to look into this?

Comment 10

[Hema Koka [:hema]]

Punam, 

Could you please take a look and see if it is a problem with wallpaper. 

thanks
hema

Comment 11

[Punam Dahiya [:pdahiya]]

Here are my findings on below environment:

Device: Flame
Platform Version:30.0
OS Version - 1.4
BuildId:20140515160201 , is the first build that shows this issue. 

Flashing my device using make reset-gaia resolves the issue. 

I tried to flash gaia from 15th may build on my machine and than running

> git reset -- hard 8eca5c19f4923432d55e676475236473252962a4
> make reset-gaia

and this fixes the random flashing in wallpaper picker. This is not a gaia issue. May be someone who is familiar with build process can take it up and see what's missing in flame builds since may 15th that gets fixed by flashing device using make reset-gaia. Thanks

Comment 12

[Jason Smith [:jsmith]]

(In reply to Punam Dahiya from comment #11)
> Here are my findings on below environment:
> 
> Device: Flame
> Platform Version:30.0
> OS Version - 1.4
> BuildId:20140515160201 , is the first build that shows this issue. 
> 
> Flashing my device using make reset-gaia resolves the issue. 
> 
> I tried to flash gaia from 15th may build on my machine and than running
> 
> > git reset -- hard 8eca5c19f4923432d55e676475236473252962a4
> > make reset-gaia
> 
> and this fixes the random flashing in wallpaper picker. This is not a gaia
> issue. May be someone who is familiar with build process can take it up and
> see what's missing in flame builds since may 15th that gets fixed by
> flashing device using make reset-gaia. Thanks

I'm not sure this analysis is entirely right. Gaia also has a build process as well for configuring areas such as wallpapers. Why wouldn't this be a problem with HiDPI wallpapers?

Comment 13

[:gerard-majax]

Did I mentionned I've been seeing this issue for a long time ; and specifically on HIDPI devices ?

Comment 14

[David Flanagan [:djf]]

The fact that this bug occurs in nightly builds but not when a developer does make reset-gaia or APP=wallpaper make install-gaia says to me that the nightly builds are being created with more (or incorrect) wallpaper images than those that are used by default when we just use the Gaia makefile.

Who can tell us whether and how the nightly builds are customizing the wallpaper? Jason: do you know anything about that?  Alternatively, I wonder if there have recently been changes in the build system and something is going wrong in the handling of the @2x and other variant sizes in nightly builds. Could it be that we're using extra large image sizes in nightly builds or something? The fact that Alexandre says this is specific to high DPI devices makes me suspect those images, somehow.

Punam: if you add more wallpapers (just duplicates of the existing ones) to the list.json file can you duplicate this bug with a local build?  What if you take the biggest images images and rename them as @1.5x images so they get used as the wallpapers?  Does that reproduce the bug?

The flickering says to me that gecko is trying to conserve image memory and is swapping the wallpaper images in and out.  We're using CSS background-image on 24+ screen-size images, so that is using a fair bit of image memory. (Though the fact that this happens on a nexus 4 (with plenty of available memory) suprises me.

We should probably modify the app to use <img> elements instead of background-image. That will allow gecko to avoid decoding the images that are offscreen.  I think it cannot currently do that for background image images.  It seems like that should prevent the bug from manifesting.

There is still presumably some kind of underlying graphics bug here, but I'm unqualified to comment on that.

Comment 15

[Punam Dahiya [:pdahiya]]

> 
> Punam: if you add more wallpapers (just duplicates of the existing ones) to
> the list.json file can you duplicate this bug with a local build?  What if
> you take the biggest images images and rename them as @1.5x images so they
> get used as the wallpapers?  Does that reproduce the bug?
> 
>
Thanks David for input. Renaming all 2.25x images (e.g. [e-mail] to FX_Bolt.jpg) and flashing using APP=wallpaper make install-gaia reproduces this issue in local build. Looks like flame build after 15th may are using 2.25x images as wallpaper thumbnails, making gecko swap wallpaper thumbnail images in and out.

Comment 16

[:gerard-majax]

(In reply to Punam Dahiya from comment #15)
> > 
> > Punam: if you add more wallpapers (just duplicates of the existing ones) to
> > the list.json file can you duplicate this bug with a local build?  What if
> > you take the biggest images images and rename them as @1.5x images so they
> > get used as the wallpapers?  Does that reproduce the bug?
> > 
> >
> Thanks David for input. Renaming all 2.25x images (e.g. [e-mail] to
> FX_Bolt.jpg) and flashing using APP=wallpaper make install-gaia reproduces
> this issue in local build. Looks like flame build after 15th may are using
> 2.25x images as wallpaper thumbnails, making gecko swap wallpaper thumbnail
> images in and out.

Gaia built for my Flame device shows that the icons are properly using @1.5x. And opening them, they are correctly sized (480x854).

Comment 17

[Jason Smith [:jsmith]]

Not really, although I do know that the Flame build is being built with a 1.5 DPI setting.

Comment 18

[Punam Dahiya [:pdahiya]]

Renaming all @F1.5x.jpg images to .jpg replicates the issue when flashed using 
APP=wallpaper make install-gaia. It is possible https://bugzilla.mozilla.org/show_bug.cgi?id=1010516#c4 commit exposed this bug.

Michael,
Is there any way to build locally with GAIA_DEV_PIXELS_PER_PX := 1.5 setting, i am assuming when we build local using make install-gaia GAIA_DEV_PIXELS_PER_PX := 1.5 setting is not applied. Is that correct?
Thanks

Comment 19

[Michael Wu [:mwu]]

Just build with 

make install-gaia GAIA_DEV_PIXELS_PER_PX=1.5

Comment 20

[Punam Dahiya [:pdahiya]]

(In reply to Michael Wu [:mwu] from comment #19)
> Just build with 
> 
> make install-gaia GAIA_DEV_PIXELS_PER_PX=1.5

Thanks Michael, APP=wallpaper make install-gaia GAIA_DEV_PIXELS_PER_PX=1.5 reproduces this issue in local build, indicating with the GAIA_DEV_PIXELS_PER_PX=1.5 turned on, wallpaper app is using lot more image memory due to @1.5x images in wallpaper list resulting in flicker.

Comment 21

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of Bug 1011460

Hi David

Please review attached pull request with the proposed fix for wallpaper picker flickering for high resolution devices. 

Fix involves separating thumbnail images shown in wallpaper list in a separate folder 'resources/thumbnails/'. As of now using high resolution images as thumbnails is memory intensive and causing wallpaper images to swap in and out.

The PR is using 320x480 images in resources/thumbnails. If we go with this approach, we can request UX to provide images in smaller sizes for thumbnails.

I did try to swap background images to <img> element to conserve image memory https://github.com/mozilla-b2g/gaia/pull/19489. However i see a lag and black tiles for ~6 sec  while loading the images with this approach.

Thanks

Comment 22

[David Flanagan [:djf]]

Now that we have the #-moz-samplesize media fragment, it would be easier to just decode the wallpapers smaller rather than trying to maintain a directory or separate images.

Add #-mozsample-size=2 to decode at 1/2 or #-moz-samplesize=3 to decode at 3/8ths size (in each dimension). Or use shared/js/media/downsample.js for a more portable solution.  That only works for jpegs, but the wallpapers are mostly jpegs.

If you agree that this is a better approach, then treat this as an r- on your patch (which I have not looked at yet.) Or, convince me that the thumbnails approach is better, and I'll actually do a review.

Also: there are issues with the wallpaper files that should probably have separate bugs filed.

1) The FX_*.jpg files are actually pngs, not jpegs, so they should at least have their filenames changed.  Since they are line art they might not look right as jpegs, but it means that we won't get the memory savings with #-moz-samplesize with them.  If visdev allows us to convert them we should just make them jpegs. If not, they should be renamed to match their actual file type.

2) Some or all of the Illus_* wallpapers are square rather than rectangular, so we're probably using about 50% more memory on displaying those than we need to. These should be cropped to match the actual form factors that we are targeting.  We'll probably want visdev to do the cropping to decide where to cut the pattern.  (Note that these are also line art, not photos, but are real jpegs, not pngs).

Needinfo Punam for the implementation suggestion, and needinfo Patryk becasue the wallpaper files are kind of busted.

Comment 23

[David Flanagan [:djf]]

Comment on attachment 8426616 [details] [review]
PR with fix of Bug 1011460

Clearing the review request.

Comment 24

[Patryk Adamczyk [:patryk] UX]

Hey David, I'd rather make the changes myself. I am traveling today, but will attached the fixed files by my EOD May 23rd.
- Not sure why the file names are incorrect in the FX files
- The Illus files are square for ease home screen rotation (ie for tablets), but it feels like that has been de-scoped for a while, so I'll crop them.

Comment 25

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #22)
> Now that we have the #-moz-samplesize media fragment, it would be easier to
> just decode the wallpapers smaller rather than trying to maintain a
> directory or separate images.
> 
> Add #-mozsample-size=2 to decode at 1/2 or #-moz-samplesize=3 to decode at
> 3/8ths size (in each dimension). Or use shared/js/media/downsample.js for a
> more portable solution.  That only works for jpegs, but the wallpapers are
> mostly jpegs.
> 

I agree this is good place to use #-moz-samplesize to downsample while decoding so that we can display a small images without using lots of memory. I will try that and update patch. Thanks

Comment 26

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of Bug 1011460

Hi David
Please review attached PR, the fix is using #moz-sample-size to downsample and create smaller images to show in wallpaper picker list. Thanks

Comment 27

[:gerard-majax]

(In reply to Punam Dahiya from comment #26)
> Created attachment 8427326 [details] [review]
> PR with fix of Bug 1011460
> 
> Hi David
> Please review attached PR, the fix is using #moz-sample-size to downsample
> and create smaller images to show in wallpaper picker list. Thanks

That does a great job on my Flame.

Comment 28

[Patryk Adamczyk [:patryk] UX]

Resized illustrated wallpapers, I also ran a JPG compressor on them:
https://mozilla.box.com/s/ue0x8qq76seen8bfi83g

For the FX_ set,please rename them to .PNG, because they have low colours they should be smaller as PNGs.

Comment 29

[Punam Dahiya [:pdahiya]]

As suggested in comment#22, wallpaper file fixes should be tracked as separate bug. Created bug https://bugzilla.mozilla.org/show_bug.cgi?id=1015448 for tracking wallpaper file type and size fixes.

Comment 33

[David Flanagan [:djf]]

Comment on attachment 8427326 [details] [review]
PR with fix of Bug 1011460

This looks great. Sorry the review took so long.

I was assuming that the downsample.js file would have already been ported to master an 1.4, and wouldn't be needed here, but I'm guessing you are going to land this before that happens.

See also bug 1008026 because it looks like this bug is also affecting high-resolution devices in 1.3. This samplesize-based fix won't work on 1.3, but I still think it is the right thing to use for 1.4 and later.

Comment 34

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #33)
> Comment on attachment 8427326 [details] [review]
> PR with fix of Bug 1011460
> 
> This looks great. Sorry the review took so long.
> 
> I was assuming that the downsample.js file would have already been ported to
> master an 1.4, and wouldn't be needed here, but I'm guessing you are going
> to land this before that happens.
> 

Thanks David for the review. I will land PR on master once travis is green.

Because of bug 971704 fixes in 2.0, this patch will not be a straight uplift to 1.4. I will create a separate PR for fixing this bug in 1.4

> See also bug 1008026 because it looks like this bug is also affecting
> high-resolution devices in 1.3. This samplesize-based fix won't work on 1.3,
> but I still think it is the right thing to use for 1.4 and later.

I will update bug 1008026 with requested info.

Comment 35

[Punam Dahiya [:pdahiya]]

PR merged with master
https://github.com/mozilla-b2g/gaia/commit/d2dbde967830767643f7e833662633bbec8280db

Comment 36

[Punam Dahiya [:pdahiya]]

Here's the link to 1.4 version of the fix
https://github.com/mozilla-b2g/gaia/pull/20025

Comment 37

[:gerard-majax]

This is still present with a Flatfish. Just updated mine to current gecko/gaia, and I'm reproducing.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/ff17b951c1ebfee1f43bf15541164f16b701c88d

Comment 39

[Punam Dahiya [:pdahiya]]

(In reply to Alexandre LISSY :gerard-majax from comment #37)
> This is still present with a Flatfish. Just updated mine to current
> gecko/gaia, and I'm reproducing.

If it's v1.4  you were testing on, please try again now that the patch is uplifted. Thanks

Comment 40

[:gerard-majax]

current gecko/gaia meant master.

Comment 41

[Punam Dahiya [:pdahiya]]

Hi Alexandre
The patch is in master, i don't have a flatfish device with me to debug the issue. Verifying if the patch has made to flatfish current build can be first step to debug. Can you pl. provide the link to current flatfish build and i can help verify that? If the fix is there and not working for flatfish, i will suggest creating a new bug to track fix for flatfish. Thanks

Comment 42

[:gerard-majax]

(In reply to Punam Dahiya from comment #41)
> Hi Alexandre
> The patch is in master, i don't have a flatfish device with me to debug the
> issue. Verifying if the patch has made to flatfish current build can be
> first step to debug. Can you pl. provide the link to current flatfish build
> and i can help verify that? If the fix is there and not working for
> flatfish, i will suggest creating a new bug to track fix for flatfish. Thanks

Okay, it's fixed. Turned out my Gaia on my Flatfish was busted, with one install in /data/local/webapps and one in /system/b2g/webapps/ ; the first one taking the lead and being obsolete.