Loading/Creating images for mutiple scale factors on the fly

Loading/Creating images for mutiple scale factors on the fly
This allow us not to load all (1x and 2x) resources at startup time, and also unload resources when they become unnecessary.

* Converted Menu's radio button to use ImageSkiaSource
* Converted Network related icons to use ImageSkiaSource

I'll create separate CL to load resources in the ResourceBundle on the fly.


BUG=122992
TEST=ImageSkiaTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145251

[oshima, 2012-07-01 19:01:57]

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
File ui/gfx/image/image_skia_unittest.cc (right):

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
ui/gfx/image/image_skia_unittest.cc:62: EXPECT_FALSE(image_skia.isNull());  //
Is this expected?
peter, is this correct?

[pkotwicz, 2012-07-01 22:22:23]

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
File ui/gfx/image/image_skia_unittest.cc (right):

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
ui/gfx/image/image_skia_unittest.cc:62: EXPECT_FALSE(image_skia.isNull());  //
Is this expected?
This is correct. When an ImageSkia has an ImageSkiaSource, there is no way for
the ImageSkia to know if the ImageSkiaSource is capable of generating image
reps, other than trying to upon a call to isNull() or empty() which is
suboptimal. Thus, an ImageSkia with an ImageSkiaSource MUST be able to generate
a bitmap for at least one scale factor.
You will have to change GetRepresentation accordingly in order to guarantee this

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:116: }
This code is ok. However, I would rather that if the image representation at the
desired scale factor is not available that it would be generated via a low
quality scale as I did here: http://codereview.chromium.org/10575039/

When a user does something like this
gfx::Canvas canvas(image.size(), ui::SCALE_FACTOR_200P, false);
SkBitmapOperations(c.ExtractImageRep(),
image1.GetRepresentation(ui::SCALE_FACTOR_200P))
It is much simpler if this call always succeeds.
It can fail if image1 only has a representation for ui::SCALE_FACTOR_100P.
The reason why I think it makes sense to do a low quality scale in
GetRepresentation is that we are just moving the time at which the scale is
applied from Canvas::DrawImageInt to ImageSkia::GetRepresentation.
The benefit is a cleaner API
I am ok if you do this in a separate CL though

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:274: return storage_->image_reps();
What happens here when you have an ImageSkiaSource() ?
I think you should rename this to cached_image_reps() to make it clear as to
what this method does.
I think you should either
1) Remove / change the use of image_reps() in the code base as this code will
not do what is expected when when being called on an ImageSkia with an
ImageSkiaSource (I think this is doable)
2) Create an image_reps() method which generates image reps for all possible
scale factors

[oshima, 2012-07-02 16:43:34]

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
File ui/gfx/image/image_skia_unittest.cc (right):

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
ui/gfx/image/image_skia_unittest.cc:62: EXPECT_FALSE(image_skia.isNull());  //
Is this expected?
On 2012/07/01 22:22:23, pkotwicz wrote:
> This is correct. When an ImageSkia has an ImageSkiaSource, there is no way for
> the ImageSkia to know if the ImageSkiaSource is capable of generating image
> reps, other than trying to upon a call to isNull() or empty() which is
> suboptimal. Thus, an ImageSkia with an ImageSkiaSource MUST be able to
generate
> a bitmap for at least one scale factor.
> You will have to change GetRepresentation accordingly in order to guarantee
this

Actually, the soruce should never be NULL (there is no use case to pass NULL),
so I removed this test. Please disregard this question.

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:116: }
On 2012/07/01 22:22:23, pkotwicz wrote:
> This code is ok. However, I would rather that if the image representation at
the
> desired scale factor is not available that it would be generated via a low
> quality scale as I did here: http://codereview.chromium.org/10575039/
> 
> When a user does something like this
> gfx::Canvas canvas(image.size(), ui::SCALE_FACTOR_200P, false);
> SkBitmapOperations(c.ExtractImageRep(),
> image1.GetRepresentation(ui::SCALE_FACTOR_200P))
> It is much simpler if this call always succeeds.
> It can fail if image1 only has a representation for ui::SCALE_FACTOR_100P.
> The reason why I think it makes sense to do a low quality scale in
> GetRepresentation is that we are just moving the time at which the scale is
> applied from Canvas::DrawImageInt to ImageSkia::GetRepresentation.
> The benefit is a cleaner API
> I am ok if you do this in a separate CL though

ImageSkia::GetRepresentation doesn't guarantee that it will
return the image rep for given scale factor, and the caller must either 
do CHECK/DCHECK or handle it gracefully.

If scaling is what a user wants, we can provide such option as an utility
source, and
daisy chain them. Here is pseudo code:

class AutoScaleSoruce : public gfx::ImagSkiaSource {
 public:
  ImageSkiaRep GetImageForScale(ui::ScaleFactor scale_factor) {
    ImageSkiaRep rep = source_->GetImageForScale(scale_factor);
    if (rep.scale_factor() != scale_factor) {
      rep = ScaleImageSkiaRep(rep, cale_factor);
    return rep;
  }
  scoped_ptr<ImageSkiaSource> source_;
}

new ImageSkia(new AutoScaleSoruce(new YourImageSkiaSource());

Otherwise, it should fail early (either programmatically, or visually) so that
we can detect such issue sooner than later.

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:274: return storage_->image_reps();
On 2012/07/01 22:22:23, pkotwicz wrote:
> What happens here when you have an ImageSkiaSource() ?
> I think you should rename this to cached_image_reps() to make it clear as to
> what this method does.

The semantics of this api is and has been "return the image reps that ImageSkia
currently have".
It's never been "returns all images for available scale factors". I understand
that there are
such use cases, and the goal of this change is to eliminate such use case. Once
we convert
them, the use of this api should be rare, and such code must know what they're
doing.

> I think you should either
> 1) Remove / change the use of image_reps() in the code base as this code will
> not do what is expected when when being called on an ImageSkia with an
> ImageSkiaSource (I think this is doable)
> 2) Create an image_reps() method which generates image reps for all possible
> scale factors

Do you have valid use case of 2)?

[pkotwicz, 2012-07-02 17:13:53]

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
File ui/gfx/image/image_skia_unittest.cc (right):

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
ui/gfx/image/image_skia_unittest.cc:62: EXPECT_FALSE(image_skia.isNull());  //
Is this expected?
You are correct that the source should never be NULL. Sorry I didn't read the
test well enough.
But I think you are still missing handling for code which does something like
this.
ImageSkia image_skia(new FixedSource(...));
if (image_skia.isNull())
   SetImage(... some default image ...)
else
   SetImage(image_skia)

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:116: }
I think there are two aspects.
1) We currently sometimes only have 1x images. (This is a bug because we are
missing assets). It might be useful to scale the images directly in the resource
bundle.
2) I don't know of any situation where the user would not want scaling. From
implementing the tab strip, I ran into a number of places where we were missing
2x assets. So I had to be really careful about which scale factor I requested
assets which went through SkBitmapOperations. However, I am fine if you want to
tackle this problem in a separate patch.
Btw, we currently already support HDA. (Though I should write some code to make
this slightly more explicit) So we already are not scaling for HDA code.

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:274: return storage_->image_reps();
A couple places where we might want to use 2
- Converting between image types (though it might be possible to use 1)
- When we want to save the contents of an ImageSkia to a file. One example is in
web_app_mac

[pkotwicz, 2012-07-02 17:26:21]

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:116: }
Btw, it might be sufficient to scale the assets which do not have 2x versions to
2x. (Only do 1)

[oshima, 2012-07-02 17:26:47]

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
File ui/gfx/image/image_skia_unittest.cc (right):

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
ui/gfx/image/image_skia_unittest.cc:62: EXPECT_FALSE(image_skia.isNull());  //
Is this expected?
On 2012/07/02 17:13:53, pkotwicz wrote:
> You are correct that the source should never be NULL. Sorry I didn't read the
> test well enough.
> But I think you are still missing handling for code which does something like
> this.
> ImageSkia image_skia(new FixedSource(...));
> if (image_skia.isNull())
>    SetImage(... some default image ...)
> else
>    SetImage(image_skia)

Do you have real example you want to do this? That is, you set source, but it
may return
null?

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:116: }
On 2012/07/02 17:13:53, pkotwicz wrote:
> I think there are two aspects.
> 1) We currently sometimes only have 1x images. (This is a bug because we are
> missing assets). It might be useful to scale the images directly in the
resource
> bundle.

grit will scale if necessary. Even if it doesn't 1x image will be scaled in
canvas
appropriately, right?

> 2) I don't know of any situation where the user would not want scaling.

We don't want to scale in most case and want to use correct 2x images instead.

> From
> implementing the tab strip, I ran into a number of places where we were
missing
> 2x assets. So I had to be really careful about which scale factor I requested
> assets which went through SkBitmapOperations. However, I am fine if you want
to
> tackle this problem in a separate patch.

This is transition issue and we should fix it in right way. I will handle these
issues
when I convert them.

> Btw, we currently already support HDA. (Though I should write some code to
make
> this slightly more explicit) So we already are not scaling for HDA code.

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:274: return storage_->image_reps();
On 2012/07/02 17:13:53, pkotwicz wrote:
> A couple places where we might want to use 2
> - Converting between image types (though it might be possible to use 1)
> - When we want to save the contents of an ImageSkia to a file. One example is
in
> web_app_mac

Are both cases for mac? We have no plan to use ImageSkiaSource for mac as it has
its own way to handle multiple resources.

[oshima, 2012-07-02 17:48:43]

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
File ui/gfx/image/image_skia_unittest.cc (right):

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
ui/gfx/image/image_skia_unittest.cc:62: EXPECT_FALSE(image_skia.isNull());  //
Is this expected?
On 2012/07/02 17:26:47, oshima wrote:
> On 2012/07/02 17:13:53, pkotwicz wrote:
> > You are correct that the source should never be NULL. Sorry I didn't read
the
> > test well enough.
> > But I think you are still missing handling for code which does something
like
> > this.
> > ImageSkia image_skia(new FixedSource(...));
> > if (image_skia.isNull())
> >    SetImage(... some default image ...)
> > else
> >    SetImage(image_skia)
> 
> Do you have real example you want to do this? That is, you set source, but it
> may return
> null?

By the way, I'm thinking of introducing "primary scale factor" so that in case
you want to compute the layout from image's size, we can load it immediately.
I'll add it when I found good example that needs this.

[pkotwicz, 2012-07-02 18:05:12]

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
File ui/gfx/image/image_skia_unittest.cc (right):

http://codereview.chromium.org/10694045/diff/5004/ui/gfx/image/image_skia_uni...
ui/gfx/image/image_skia_unittest.cc:62: EXPECT_FALSE(image_skia.isNull());  //
Is this expected?
I don't know of any case where you would want a source which is unable of
generating bitmaps for any scale.

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:274: return storage_->image_reps();
We will most likely be using ImageSkiaSource for mac as mac uses shared code
which uses SkBitmaps. I don't know of any place where we would call image_reps()
from an ImageSkia which would have been constructed with an ImageSkiaSource.
However, it would be nice for ImageSkia to have a robust API.

[oshima, 2012-07-02 18:19:30]

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc
File ui/gfx/image/image_skia.cc (right):

http://codereview.chromium.org/10694045/diff/9002/ui/gfx/image/image_skia.cc#...
ui/gfx/image/image_skia.cc:274: return storage_->image_reps();
On 2012/07/02 18:05:12, pkotwicz wrote:
> We will most likely be using ImageSkiaSource for mac as mac uses shared code
> which uses SkBitmaps.
> I don't know of any place where we would call image_reps()
> from an ImageSkia which would have been constructed with an ImageSkiaSource.

Let's revisit if we find such case then.

> However, it would be nice for ImageSkia to have a robust API.

As I mentioned, there is no guarantee that image_reps() will return
images for possible scale factors (it's simply impossible), even now
without this change. Such code must know what they're doing.

Actually, I wonder if we really need this API. I looked at the current usage in
mac,
and looks like we can replace them with "get supported scale factors +
GetRepresentation"?

[pkotwicz, 2012-07-02 18:48:08]

We still need to use it for web_app_mac because we can have multiple
representations for the same scale factor.
You are right about image_skia_mac_util

[oshima, 2012-07-02 20:15:20]

On 2012/07/02 18:48:08, pkotwicz wrote:
> We still need to use it for web_app_mac because we can have multiple
> representations for the same scale factor.

Ok, so that's the only use case and it doesn't have to deal with ImageSkiaSource
because
it's favicon, right?

> You are right about image_skia_mac_util

[pkotwicz, 2012-07-02 21:16:27]

LGTM

Oshima, thanks for doing this.
However, please document the things which might not work the way a user expects.
(image_reps(), and extractSubset()).

[Ben Goodger (Google), 2012-07-02 21:22:50]

lgtm

http://codereview.chromium.org/10694045/diff/4013/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10694045/diff/4013/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:41: ImageSkia(ImageSkiaSource* soruce, const
gfx::Size& size);
source

[oshima, 2012-07-02 21:23:11]

Ben, could you please review this?

On 2012/07/02 21:16:27, pkotwicz wrote:
> LGTM
> 
> Oshima, thanks for doing this.
> However, please document the things which might not work the way a user
expects.
> (image_reps(), and extractSubset()).
done

[oshima, 2012-07-02 21:35:06]

http://codereview.chromium.org/10694045/diff/4013/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

http://codereview.chromium.org/10694045/diff/4013/ui/gfx/image/image_skia.h#n...
ui/gfx/image/image_skia.h:41: ImageSkia(ImageSkiaSource* soruce, const
gfx::Size& size);
On 2012/07/02 21:22:50, Ben Goodger (Google) wrote:
> source

Done.

[oshima, 2012-07-02 21:36:17]

stevenjb -> chrome/browser/chromeos/status

[pkotwicz, 2012-07-02 21:39:36]

A couple nits, still LGTM

http://codereview.chromium.org/10694045/diff/4013/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

http://codereview.chromium.org/10694045/diff/4013/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:219: gfx::Canvas
canvas(icon_rep.sk_bitmap(), false);
Nit: You can just pass in the ImageSkiaRep to the canvas constructor and skip
the next line.

http://codereview.chromium.org/10694045/diff/4013/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:298: // NeworkMenuIcons has
the same size in DIP
Nit: has->all have

http://codereview.chromium.org/10694045/diff/4013/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_source.h (right):

http://codereview.chromium.org/10694045/diff/4013/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_source.h:20: virtual gfx::ImageSkiaRep
GetImageForScale(ui::ScaleFactor scale_factor) = 0;
Nit: rename this method to GetImageRepForScale/GetRepresentation as it returns
an ImageSkiaRep* not an ImageSkia*

[oshima, 2012-07-02 22:24:58]

http://codereview.chromium.org/10694045/diff/4013/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

http://codereview.chromium.org/10694045/diff/4013/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:219: gfx::Canvas
canvas(icon_rep.sk_bitmap(), false);
On 2012/07/02 21:39:36, pkotwicz wrote:
> Nit: You can just pass in the ImageSkiaRep to the canvas constructor and skip
> the next line.

ah, nice. done.

http://codereview.chromium.org/10694045/diff/4013/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:298: // NeworkMenuIcons has
the same size in DIP
On 2012/07/02 21:39:36, pkotwicz wrote:
> Nit: has->all have

Done.

http://codereview.chromium.org/10694045/diff/4013/ui/gfx/image/image_skia_sou...
File ui/gfx/image/image_skia_source.h (right):

http://codereview.chromium.org/10694045/diff/4013/ui/gfx/image/image_skia_sou...
ui/gfx/image/image_skia_source.h:20: virtual gfx::ImageSkiaRep
GetImageForScale(ui::ScaleFactor scale_factor) = 0;
On 2012/07/02 21:39:36, pkotwicz wrote:
> Nit: rename this method to GetImageRepForScale/GetRepresentation as it returns
> an ImageSkiaRep* not an ImageSkia*

ImageRep isn't correct either unless we use ImageSkiaRep. I once used
GetImageSkaiRepForScale, but it looked excessive. I also thought
that it's not helpful because this is not something you call.
It's something you have to implement and it's pretty clear that you need to
create ImageSkiaRep.
 I'd just follow OWNERS's judgement call. Ben, let me know what you think and
I'll make change.

[stevenjb, 2012-07-02 22:31:34]

network_menu_icon.cc lgtm (just one question)

http://codereview.chromium.org/10694045/diff/1045/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

http://codereview.chromium.org/10694045/diff/1045/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:284: gfx::ImageSkiaRep
image_rep = images->GetRepresentation(scale_factor);
I assume that image_rep will always be scaled appropriately based on
scale_factor, even if a larger image isn't provided? (Otherwise it seems like
scaling height and width below could cause problems if the high res image were
missing).

[oshima, 2012-07-02 22:42:06]

http://codereview.chromium.org/10694045/diff/1045/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

http://codereview.chromium.org/10694045/diff/1045/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:284: gfx::ImageSkiaRep
image_rep = images->GetRepresentation(scale_factor);
On 2012/07/02 22:31:34, stevenjb (chromium) wrote:
> I assume that image_rep will always be scaled appropriately based on
> scale_factor, even if a larger image isn't provided? (Otherwise it seems like
> scaling height and width below could cause problems if the high res image were
> missing).

It may not, and that's why I'm using the image_rep.scale_factor() instead of
scale_factor passed to this function below. If 2x resource is not available,
imgage_rep.scale_factor() returns 1x, and will give the correct subset rect for
1x image. Note that width and height are in dip, so it needs to be multiplied
by scale factor in order for extractSubset to work.

[commit-bot: I haz the power, 2012-07-03 02:19:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/10694045/14007

[commit-bot: I haz the power, 2012-07-03 03:35:33]

Change committed as 145251

[stevenjb, 2012-07-03 16:38:38]

http://codereview.chromium.org/10694045/diff/1045/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

http://codereview.chromium.org/10694045/diff/1045/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:284: gfx::ImageSkiaRep
image_rep = images->GetRepresentation(scale_factor);
On 2012/07/02 22:42:06, oshima wrote:
> On 2012/07/02 22:31:34, stevenjb (chromium) wrote:
> > I assume that image_rep will always be scaled appropriately based on
> > scale_factor, even if a larger image isn't provided? (Otherwise it seems
like
> > scaling height and width below could cause problems if the high res image
were
> > missing).
> 
> It may not, and that's why I'm using the image_rep.scale_factor() instead of
> scale_factor passed to this function below. If 2x resource is not available,
> imgage_rep.scale_factor() returns 1x, and will give the correct subset rect
for
> 1x image. Note that width and height are in dip, so it needs to be multiplied
> by scale factor in order for extractSubset to work.

Ah, I see, I was confused by scale vs. scale_factor. Cool, looks good then.

[sky, 2012-07-16 16:41:01]

https://chromiumcodereview.appspot.com/10694045/diff/14007/ui/gfx/image/image...
File ui/gfx/image/image_skia.h (right):

https://chromiumcodereview.appspot.com/10694045/diff/14007/ui/gfx/image/image...
ui/gfx/image/image_skia.h:41: ImageSkia(ImageSkiaSource* source, const
gfx::Size& size);
This should document it takes ownership of source.

[sky, 2012-07-16 16:44:31]

https://chromiumcodereview.appspot.com/10694045/diff/14007/ui/gfx/image/image...
File ui/gfx/image/image_skia_source.h (right):

https://chromiumcodereview.appspot.com/10694045/diff/14007/ui/gfx/image/image...
ui/gfx/image/image_skia_source.h:20: virtual gfx::ImageSkiaRep
GetImageForScale(ui::ScaleFactor scale_factor) = 0;
This should document the source shouldn't cache the imagerep since ImageSkia
caches the returned value.