Closed Bug 1011460 Opened 6 years ago Closed 6 years ago

[Flame] Wallpaper picker flickering

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: gerard-majax, Assigned: pdahiya)

References

Details

(Keywords: regression, Whiteboard: [2.0-flame-test-run-1])

Attachments

(1 file, 1 obsolete file)

47 bytes, text/x-github-pull-request
djf
: review+
Details | Review
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.
This works fine on my Buri master but I reproduce on my Peak v1.4.

Could it be happening on HD devices only?
I can reproduce it on a flame (2.0.0-prerelease), but it works fine on my keon (v1.4).
If anyone could try with an Open C, with v1.3 and v1.4.
blocking-b2g: 2.0? → 1.4?
Keywords: qawanted
QA Contact: jmitchell
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
Keywords: qawanted
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
The range seems off here - the commit in the range seems unrelated to the problem here.

Can we get a video here?
Keywords: qawanted
Switching to regression range wanted to double check the range as well.
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
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?
Blocks: 1010256
No longer depends on: 972442
Flags: needinfo?(hkoka)
Blocks: 1010516
No longer blocks: 1010256
Punam, 

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

thanks
hema
Flags: needinfo?(hkoka) → needinfo?(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
Flags: needinfo?(pdahiya)
blocking-b2g: 1.4? → 1.4+
(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?
Did I mentionned I've been seeing this issue for a long time ; and specifically on HIDPI devices ?
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.
Flags: needinfo?(pdahiya)
Flags: needinfo?(jsmith)
> 
> 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. FX_Bolt@2.25x.jpg 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.
Flags: needinfo?(pdahiya)
(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. FX_Bolt@2.25x.jpg 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).
Not really, although I do know that the Flame build is being built with a 1.5 DPI setting.
Flags: needinfo?(jsmith)
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
Flags: needinfo?(mwu)
Just build with 

make install-gaia GAIA_DEV_PIXELS_PER_PX=1.5
Flags: needinfo?(mwu)
(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.
Assignee: nobody → pdahiya
Attached file PR with fix of Bug 1011460 (obsolete) —
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
Attachment #8426616 - Flags: review?(dflanagan)
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.
Flags: needinfo?(pdahiya)
Flags: needinfo?(padamczyk)
Comment on attachment 8426616 [details] [review]
PR with fix of Bug 1011460

Clearing the review request.
Attachment #8426616 - Flags: review?(dflanagan)
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.
(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
Flags: needinfo?(pdahiya)
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
Attachment #8426616 - Attachment is obsolete: true
Attachment #8427326 - Flags: review?(dflanagan)
(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.
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.
Flags: needinfo?(padamczyk)
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.
Duplicate of this bug: 1015454
Whiteboard: [2.0-flame-test-run-1]
Duplicate of this bug: 1016158
Duplicate of this bug: 1017429
Target Milestone: --- → 2.0 S3 (6june)
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.
Attachment #8427326 - Flags: review?(dflanagan) → review+
(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.
Here's the link to 1.4 version of the fix
https://github.com/mozilla-b2g/gaia/pull/20025
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
This is still present with a Flatfish. Just updated mine to current gecko/gaia, and I'm reproducing.
Flags: needinfo?(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
Flags: needinfo?(pdahiya)
current gecko/gaia meant master.
Flags: needinfo?(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
Flags: needinfo?(pdahiya)
(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.
You need to log in before you can comment on or make changes to this bug.