1041710 - Resize wallpaper images to device dimensions before saving to settings

Status: RESOLVED FIXED

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

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

In bug 1038988, we can create a fast path to the GPU for the wallpaper images if we don't have to resize using CSS (ie background-size: cover). First we must make sure we always have the properly sized image blob in the settings DB.

Comment 1

[David Flanagan [:djf]]

This blocks a 2.0+ blocker, so setting the flag here, too.

Comment 2

[Michael Henretty [:mikehenrty][:mhenretty]]

Just as an update, David and I talked about this on IRC today, and he is working on on plan to do the resizing when we pull the image out of settings rather than when we save the image to settings. This avoids many edge cases where we could erroneously save an improperly sized image to the DB, which could cause display issues when we land bug 1038988.

Comment 3

[David Flanagan [:djf]]

The nice thing about putting the wallpaper resizing code in the system app is that we only have to do it once, rather than in the homescreen, settings and FL apps.

The bad thing about putting it in the system app is that it is the system app and we want to avoid adding features to it when we have to.

The reason that I feel we have to put this resizing code in the system app is that our build system is not aware of the screen size that it is building for. Builds do know the devicePixelRatio, and we use this to select regular wallpaper or 1.5x or 2.0x wallpaper.  But our build system cannot distinguish between a 1.5x WVGA (480x800) display (like the QRD device that CAF is testing on) and a 1.5X FWVGA (480x854) display. So, when we build gaia for a WVGA display it ships with 1.5x wallpapers that are sized to 480x854.  So even the default wallpaper that we ship with does not have the right size.  Fixing the build system is too big a job for a 2.0+ bug, so I think we have to modify the system app to resize the wallpaper before displaying it.

Given that the point of this bug is to save memory, I want to be careful about memory. Decoding images unnecessarily is a quick way to spike memory usage, so I don't want to just decode the image and check its size. We have utilities in shared/js/media/ for determining the size of an image without decoding it. Unfortunately, to use the existing getImageSize() utility requires loading three separate modules (blobview.js, jpeg_metadata_parser.js, and image_size.js). This seems heavy for the system app, especially loading the a full JPEG exif parser, so I think that to do this right, I should create a lightweight standalone version of getImageSize().

For wallpaper images that are large jpegs (we might get these from 3rd party wallpaper apps or from the FL app) it is probably worth using the #-moz-samplesize media fragment to downsample while decoding, if the images are more than twice as big as the screen, and I'll probably use my shared/js/media/downsample.js utility to handle this case.

I think that I should save the resized version back into the settings db so that I don't need to resize it each time the phone starts up. Assuming I do this, there are two things I'm uncertain about:

1) Should I try to set some other settings property that asserts that the wallpaper size has been checked, so that I can skip the imagesize check next time?  (If so, all the image sizing and resizing code could be lazily loaded and would only be read into the system app if the user changes wallpaper).  I'm leaning toward yes: this seems like a valuable optimization.

2) Should I save the resized wallpaper as PNG to avoid image quality reductions caused by re-encoding as JPEG?  Or, should I keep it as JPEG so that the lockscreen code can be modified to use #-moz-samplesize when it generates its masked background color.  I'm leaning toward PNG here, since it seems like premature optimization to worry about the lockscreen now.

Setting needinfo for Tim so that this pending change to the system app is on his radar. Please let me know if you have any objections to the plan described here.  Also setting needinfo for Etienne since Tim is probably not reading his bugmail because of a typhoon!

Comment 4

[David Flanagan [:djf]]

I'm planning to put my image sizing and resizing module in shared/js/ because it will be useful for other apps, too, and am designing a blob-based API.

But then I remembered that the system app wallpaper code users urls becasue we initialize the database with a base64-encoded data: URL.  That is something else that should be fixed in the build system. But it means that the first time we run, I'll need to take that data URI and convert it to a blob.  And if I do that, then I might as well write that blob back into the database so we don't have to deal with base64 strings anymore.

Comment 5

[David Flanagan [:djf]]

Another idea: for the case of wallpapers that are just a little too large (like the 480x854 wallpapers when we want 480x800) we could use the #xywh= media fragment to do the cropping when the image was decoded. I thought that was working, but I can't get it to work in desktop Aurora, and I can't find a pref to enable it, so maybe it is not enabled.

Comment 6

[David Flanagan [:djf]]

Attachment: link to patch on github

This isn't a full patch yet, but this particular image size utility is ready for review.

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8461165 [details] [review]
link to patch on github

I left a few comments on github, but overall I defer to your expertise on image manipulation. I can't really give this an r+ since there is a missing function, but I can give a fb+ because the test cases seem thorough, and this looks like reworked battle tested code.

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to David Flanagan [:djf] from comment #3)
> The nice thing about putting the wallpaper resizing code in the system app
> is that we only have to do it once, rather than in the homescreen, settings
> and FL apps.
> 
> The bad thing about putting it in the system app is that it is the system
> app and we want to avoid adding features to it when we have to.
> 
> The reason that I feel we have to put this resizing code in the system app
> is that our build system is not aware of the screen size that it is building
> for. Builds do know the devicePixelRatio, and we use this to select regular
> wallpaper or 1.5x or 2.0x wallpaper.  But our build system cannot
> distinguish between a 1.5x WVGA (480x800) display (like the QRD device that
> CAF is testing on) and a 1.5X FWVGA (480x854) display. So, when we build
> gaia for a WVGA display it ships with 1.5x wallpapers that are sized to
> 480x854.  So even the default wallpaper that we ship with does not have the
> right size.  Fixing the build system is too big a job for a 2.0+ bug, so I
> think we have to modify the system app to resize the wallpaper before
> displaying it.
> 
> Given that the point of this bug is to save memory, I want to be careful
> about memory. Decoding images unnecessarily is a quick way to spike memory
> usage, so I don't want to just decode the image and check its size. We have
> utilities in shared/js/media/ for determining the size of an image without
> decoding it. Unfortunately, to use the existing getImageSize() utility
> requires loading three separate modules (blobview.js,
> jpeg_metadata_parser.js, and image_size.js). This seems heavy for the system
> app, especially loading the a full JPEG exif parser, so I think that to do
> this right, I should create a lightweight standalone version of
> getImageSize().
> 
> For wallpaper images that are large jpegs (we might get these from 3rd party
> wallpaper apps or from the FL app) it is probably worth using the
> #-moz-samplesize media fragment to downsample while decoding, if the images
> are more than twice as big as the screen, and I'll probably use my
> shared/js/media/downsample.js utility to handle this case.
> 
> I think that I should save the resized version back into the settings db so
> that I don't need to resize it each time the phone starts up. Assuming I do
> this, there are two things I'm uncertain about:
> 
> 1) Should I try to set some other settings property that asserts that the
> wallpaper size has been checked, so that I can skip the imagesize check next
> time?  (If so, all the image sizing and resizing code could be lazily loaded
> and would only be read into the system app if the user changes wallpaper). 
> I'm leaning toward yes: this seems like a valuable optimization.

The only problem I see with this is when to invalidate this assert in the DB. For instance, we would probably need to invalidate the setting on upgrade, or whenever anyone writes to the wallpaper setting in general. My preference would be to leave this out for now (since we only run the resize logic at boot, and when changing wallpaper), and we can always optimize in a follow-up.


> 
> 2) Should I save the resized wallpaper as PNG to avoid image quality
> reductions caused by re-encoding as JPEG?  Or, should I keep it as JPEG so
> that the lockscreen code can be modified to use #-moz-samplesize when it
> generates its masked background color.  I'm leaning toward PNG here, since
> it seems like premature optimization to worry about the lockscreen now.

Is it visibly obvious when we resize/re-encode a JPEG versus PNG? If so, I don't think we have any other option than to do PNG.


(In reply to David Flanagan [:djf] from comment #4)
> But then I remembered that the system app wallpaper code users urls becasue
> we initialize the database with a base64-encoded data: URL.  That is
> something else that should be fixed in the build system. But it means that
> the first time we run, I'll need to take that data URI and convert it to a
> blob.  And if I do that, then I might as well write that blob back into the
> database so we don't have to deal with base64 strings anymore.

Good catch.


(In reply to David Flanagan [:djf] from comment #5)
> Another idea: for the case of wallpapers that are just a little too large
> (like the 480x854 wallpapers when we want 480x800) we could use the #xywh=
> media fragment to do the cropping when the image was decoded. I thought that
> was working, but I can't get it to work in desktop Aurora, and I can't find
> a pref to enable it, so maybe it is not enabled.

Does this save us memory on the gecko side? If so, we should investigate, but I wouldn't think this would save us enough to block 2.0.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I think I agree with you on the division of responsibilities: whoever writes the wallpaper to settings database should be responsible of cropping that to the right size. However, that also make this bug touches multiple apps + build script so we might not want to consider all of these works are blockers for the time being.

Build script work is non-trivial but nonetheless doable. However, instead of attempt to crop the image on build-time with, say, a GAIA_SCREEN_DIMENSION variable, we might want to ask visual designers to provide cropped images of all shipping screen sizes and have the build script pick the images between them. The script can always fall back to a slightly bigger one if the image of exact size is not found.

Comment 10

[David Flanagan [:djf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #9)
> I think I agree with you on the division of responsibilities: whoever writes
> the wallpaper to settings database should be responsible of cropping that to
> the right size. However, that also make this bug touches multiple apps +
> build script so we might not want to consider all of these works are
> blockers for the time being.

So I think we agree that doing the cropping in the various apps would be better, but that for now we have to do it in the system app. Since wallpaper changes should be infrequent, I will lazy-load the required code. 

> 
> Build script work is non-trivial but nonetheless doable. However, instead of
> attempt to crop the image on build-time with, say, a GAIA_SCREEN_DIMENSION
> variable, we might want to ask visual designers to provide cropped images of
> all shipping screen sizes and have the build script pick the images between
> them. The script can always fall back to a slightly bigger one if the image
> of exact size is not found.

Yes, if the build system changes are made, then we would just add properly-sized wallpapers to the build so that this is not an issue for system wallpapers.

Comment 11

[David Flanagan [:djf]]

I've updated my WIP pull request to include an image resizing and a #-moz-samplesize utility in shared/js/image_utils.js.  A The ImageUtils.Downsample utility is a direct copy of shared/js/media/downsample.js.  And the test code for the new ImageUtils.resizeAndCropToCover() function is based heavily on the existing tests for the shared/js/media/crop_resize_rotate.js module.  So even though it looks like a lot of new code, much of it is already production code that I've just moved around to be easier to use outside of the media apps.

With these utilities defined, I'll start working on apps/system/js/wallpaper_manager.js

Comment 12

[David Flanagan [:djf]]

Comment on attachment 8461165 [details] [review]
link to patch on github

Mike,

The image_utils.js file is ready for review now.  You've already looked at the getSizeAndType code.  The ImageUtils.Downsample code is just copied from another module and probably does not need to be reviewed again. So what is new is the ImageUtils.resizeAndCropToCover() function.  That is what I'll be using in the system app.

Comment 13

[David Flanagan [:djf]]

Comment on attachment 8461165 [details] [review]
link to patch on github

Tim,

I'm setting feedback? for you on this pull request which includes a first (untested) pass at the changes I'm making to wallpaper_manager.js for this patch.

Would you like to be the reviewer for the system app changes, or can you suggest someone else?

Comment 14

[David Flanagan [:djf]]

A related issue: the gallery pick activity seems to be returning an 853x480 image instead of a 854x480 image on flame. Looks like a rounding error in Gallery, and it is causing an extra resize pass in the system app.

Comment 15

[David Flanagan [:djf]]

Another possible issue: if the settings app is not running and I change the wallpaper from the homescreen, and then open the settings app, the settings:display screen does not display the current wallpaper correctly.  Have I caused this regression, or is it an existing bug?

Comment 16

[David Flanagan [:djf]]

The issue in comment 14 was caused because on a FWVGA display like Flame, screen.height * window.devicePixelRatio is 853.5 instead of the exact 854 value. I've added Math.round() calls in the code that invokes the wallpaper pick activity, so we should get 854px wallpaper from the gallery now.

Comment 17

[David Flanagan [:djf]]

Actually, I updated the old homescreen. Now I've fixed the activity in the new homescreen as well.

Comment 18

[David Flanagan [:djf]]

I've edited the description of the pull request.  This patch is ready to go except for unit tests for the system app changes.  The wallpaper_manager.js module in the system app does not have any existing tests, so I'll need to start from scratch.

Comment 19

[David Flanagan [:djf]]

The settings issue described in comment 15 is unrelated to this patch. I've filed bug 1044166 for it.

Comment 20

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8461165 [details] [review]
link to patch on github

r=me on the ImageUtils getSizeAndType and resizeAndCropToCover functionality and tests. I left a few comments on error handling but overall looks good. I also learned a lot just reading this code, so thanks :)

Comment 21

[David Flanagan [:djf]]

Comment on attachment 8461165 [details] [review]
link to patch on github

Clearing the feedback request for Tim and converting it into a review request for either Tim or Etienne.... Could one of look at the system app changes in this bug?  Mike has already reviewed the shared/js/image_utils.js changes, so its only the system app changes that need review.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8461165 [details] [review]
link to patch on github

Since this is a 2.0+ bug, I am just going to lower the review standard and r+ this patch with only a few nitpicks.

Upon reviewing, I found the WallpaperManager try to do a series of successive async action, with each action being a method in the instance and named non-descriptively. For example, it's hard to understand what |convertToBlob| will call next without actually reading the code. Also, with these rather large list of methods, it take some time to realize what's the actual useful API of the module (none actually, except |start()| and |stop()|).

I would recommend we spend some time to rewrite this module after the patch is checked in. To be more specific:

1. All non-public methods should begin with underline. Whether or not to test them individually as a "unit" in the unit test is of personal preference, but I usually call the public API and ensure the eventual outcome.
2. Chain these async actions into Promises so it's there is a overlook available for people at the starting functions.

For example, let's rename |newWallpaper| to |_handleWallpaperChange| since what this function really do is handling the incoming wallpaper change from settings db observer. Then, in this function, we could do

var p = Promise.resolve(value);

if (typeof value === 'string') {
  p = p.then(this._convertToSizedBlob.bind(this));
}
if (!valid) {
  p = p.then(this._checkAndConvertWallpaperSize.bind(this));
}
p = p.then(this._displayWallpaper.bind(this));

return p;

Obviously this is just an example and obviously, for example, |_checkAndConvertWallpaperSize| may result more async actions in ImageUtils. The good thing about Promise is that they all can be chained, and your ImageUtils is already returning promises. If the code is written this way in less than 10 lines reader of |_handleWallpaperChange| will know the eventual outcome of this functions is |_displayWallpaper|. That's a benefit I don't see from the current code, and it's a benefit we would need if this piece of code needs to be maintainable by someone else in the future.

Thanks.

Comment 23

[David Flanagan [:djf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #22)
> Comment on attachment 8461165 [details] [review]
> link to patch on github
> 
> Since this is a 2.0+ bug, I am just going to lower the review standard and
> r+ this patch with only a few nitpicks.

Thank you for the review, Tim. I don't suppose you meant it this way, but note that this first sentence sounds very condescending.

> 
> Upon reviewing, I found the WallpaperManager try to do a series of
> successive async action, with each action being a method in the instance and
> named non-descriptively. For example, it's hard to understand what
> |convertToBlob| will call next without actually reading the code. Also, with
> these rather large list of methods, it take some time to realize what's the
> actual useful API of the module (none actually, except |start()| and
> |stop()|).
> 
> I would recommend we spend some time to rewrite this module after the patch
> is checked in. To be more specific:
> 
> 1. All non-public methods should begin with underline. Whether or not to
> test them individually as a "unit" in the unit test is of personal
> preference, but I usually call the public API and ensure the eventual
> outcome.

I dislike the proliferation of underscores in code. In this case, however I agree that they are probably appropriate. Normally, I try to hide internal methods as nested functions and avoid underscores that way. For this patch I felt it was going to be helpful to have the internal methods exposed to facilitate unit testing...

Adding underscores is easier to do than making the switch from self to bind() that you requested, so I'll go ahead and make this change now, or will at least document clearly that only start() and stop() are public. 

> 2. Chain these async actions into Promises so it's there is a overlook
> available for people at the starting functions.
>

I think it would be better to provide that overview of the code with comments than with a Promise-based architecture. I find Promises confusing and difficult to reason about. It will presumably become easier as I gain more experience using them. But for now I can say that for a reader like me adding promises to this code would make it harder to understand, not easier. Also, I'm not convinced that I could make the error handling cases (like passing the default URL to convertToBlob if the image in the setting db is corrupt) work with a promise-based solution. Perhaps that would all work out as a chain of promises...

Comment 24

[David Flanagan [:djf]]

Attachment: github pr for the 2.0 branch

The PR does not uplift cleanly to 2.0 because the settings app has been restructured and the wallpaper.js is in a different directory in 2.0 and master. That was the only merge conflict.

Comment 25

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/e058cf96d19b0eac12759a500599fb5380dd5a9d

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to David Flanagan [:djf] from comment #23)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #22)
> > Comment on attachment 8461165 [details] [review]
> > link to patch on github
> > 
> > Since this is a 2.0+ bug, I am just going to lower the review standard and
> > r+ this patch with only a few nitpicks.
> 
> Thank you for the review, Tim. I don't suppose you meant it this way, but
> note that this first sentence sounds very condescending.
> 

Sorry, I wasn't meant to make you feel this way and I certainly doesn't mean that. What I meant was that I should unblock technical discussion like what we are having right now from shipping the feature, and we can always file the next bug to reflect the outcome of the technical discussion.

> > 
> > 1. All non-public methods should begin with underline. Whether or not to
> > test them individually as a "unit" in the unit test is of personal
> > preference, but I usually call the public API and ensure the eventual
> > outcome.
> 
> I dislike the proliferation of underscores in code. In this case, however I
> agree that they are probably appropriate. Normally, I try to hide internal
> methods as nested functions and avoid underscores that way. For this patch I
> felt it was going to be helpful to have the internal methods exposed to
> facilitate unit testing...
> 
> Adding underscores is easier to do than making the switch from self to
> bind() that you requested, so I'll go ahead and make this change now, or
> will at least document clearly that only start() and stop() are public. 
> 

I dislike the exact opposite (hiding the internal methods inside |function(exports) { .. }(window)|). These functions looks like free floating functions w/o any connection to the exported function, and it's hard to find out who is using that function if the file exports more than one function. I agree that hides internal detail well than underscore though.

> > 2. Chain these async actions into Promises so it's there is a overlook
> > available for people at the starting functions.
> >
> 
> I think it would be better to provide that overview of the code with
> comments than with a Promise-based architecture. I find Promises confusing
> and difficult to reason about. It will presumably become easier as I gain
> more experience using them. But for now I can say that for a reader like me
> adding promises to this code would make it harder to understand, not easier.
> Also, I'm not convinced that I could make the error handling cases (like
> passing the default URL to convertToBlob if the image in the setting db is
> corrupt) work with a promise-based solution. Perhaps that would all work out
> as a chain of promises...

It would, promise by default give you the opportunity to "then()" a rejected promise, error-handling it in the onReject callback, and return a resolved promise.

It's fine if we don't convert this module to anything else for the time being, as long as knowledge is entirely kept in the git repo (as code or comment).

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/8a37e7f08ae1282d4615454fb35b0da7e20e9ca9