The key for SkBitmapCache can now be genID+SkIRect

The key for SkBitmapCache can now be genID+SkIRect

BUG=skia:2894
Committed: https://skia.googlesource.com/skia/+/42b0dfeb29e993b7fd247dcecff705d3dd4cf191

[Rémi Piotaix, 2014-08-29 19:09:55]

piotaixr@chromium.org changed reviewers:
+ junov@chromium.org, reed@google.com

[Rémi Piotaix, 2014-08-29 19:11:06]

PTAL

[Justin Novosad, 2014-08-29 20:07:44]

Reitveld chose the most confusing way to highlight the changes. LOL

A test would be nice.

[reed1, 2014-09-02 14:21:12]

API looks good. Please augment the unitests to exercise this.

[reed1, 2014-09-02 14:21:37]

On 2014/09/02 14:21:12, reed1 wrote:
> <span id="abbrex-spnAbbr_14537" class="abbrex-Abbr abbrex-Abbr_API
abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span> looks good. Please
augment the unitests to exercise this.

tests/ScaledImageCache.cpp

[Rémi Piotaix, 2014-09-02 15:15:01]

On 2014/09/02 14:21:37, reed1 wrote:
> On 2014/09/02 14:21:12, reed1 wrote:
> > <span id="abbrex-spnAbbr_14537" class="abbrex-Abbr abbrex-Abbr_API
> abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span> looks good. Please
> augment the unitests to exercise this.
> 
> tests/ScaledImageCache.cpp

Done

[reed1, 2014-09-02 15:23:09]

What do we think should be done with ambiguous arguments?

e.g.
- what if the subset rect is not contained inside the bitmap's bounds?
- what if the subset rect is empty?

[Rémi Piotaix, 2014-09-02 15:33:43]

On 2014/09/02 15:23:09, reed1 wrote:
> What do we think should be done with ambiguous arguments?
> 
> e.g.
> - what if the subset rect is not contained inside the bitmap's bounds?
The way we are going to use it, it will be possible that the rect will not be in
the bitmap's bound:
We will reference the cached subset of an other bitmap so, the rect will be the
position of the cached bitmap in the original one.
Maybe the test that we can do here is to check that the dimensions of the bitmap
and the rect are the same.

> - what if the subset rect is empty?
For that, I don't know... return false? What do we do in Add(_, width, height,
_) if width or height = 0?

[Justin Novosad, 2014-09-02 17:02:41]

On 2014/09/02 15:33:43, Rémi Piotaix wrote:
> On 2014/09/02 15:23:09, reed1 wrote:
> > What do we think should be done with ambiguous arguments?
> > 
> > e.g.
> > - what if the subset rect is not contained inside the bitmap's bounds?
> The way we are going to use it, it will be possible that the rect will not be
in
> the bitmap's bound:
> We will reference the cached subset of an other bitmap so, the rect will be
the
> position of the cached bitmap in the original one.
> Maybe the test that we can do here is to check that the dimensions of the
bitmap
> and the rect are the same.

BTW "not contained" also means that the bitmap could intersect the bitmap bounds

> 
> > - what if the subset rect is empty?
> For that, I don't know... return false? What do we do in Add(_, width, height,
> _) if width or height = 0?

I personally think we should aim to be consistent with the existing behavior of
SkBitmap::extractSubset, which intersects the subset rect with the bitmap
bounds, and uses that. If the intersection is empty. it returns false.

[Rémi Piotaix, 2014-09-02 18:08:57]

On 2014/09/02 17:02:41, junov wrote:
> On 2014/09/02 15:33:43, Rémi Piotaix wrote:
> > On 2014/09/02 15:23:09, reed1 wrote:
> > > What do we think should be done with ambiguous arguments?
> > > 
> > > e.g.
> > > - what if the subset rect is not contained inside the bitmap's bounds?
> > The way we are going to use it, it will be possible that the rect will not
be
> in
> > the bitmap's bound:
> > We will reference the cached subset of an other bitmap so, the rect will be
> the
> > position of the cached bitmap in the original one.
> > Maybe the test that we can do here is to check that the dimensions of the
> bitmap
> > and the rect are the same.
> 
> BTW "not contained" also means that the bitmap could intersect the bitmap
bounds
> 
> > 
> > > - what if the subset rect is empty?
> > For that, I don't know... return false? What do we do in Add(_, width,
height,
> > _) if width or height = 0?
> 
> I personally think we should aim to be consistent with the existing behavior
of
> SkBitmap::extractSubset, which intersects the subset rect with the bitmap
> bounds, and uses that. If the intersection is empty. it returns false.

Ok, so we can check if the size of the rect used for the key is the same size of
the bitmap we want to put in the cache and fail if not. For that, maybe change
the return type of the Add method? (void -> bool)

[reed1, 2014-09-02 18:13:45]

I think we can formalize the idea of "make a subset rect canonical or fail".
Meaning given a bitmap and a "requested" subset rect, we can compute a canonical
subset rect (i.e. intersect it with the bitmap bounds). That is the "right" rect
to use for keys, or the rect is empty, in which case we reject it.

1. What to do with Find? We can canonicalize the caller's rect and then search,
or we can just pass it in as is, and if they have not "canonicalized" it
themselves, they will always get a miss.

2. What to do with Add? We already have the case where Add (in the general
sense) can be a no-opt (e.g. the specified key already exists in the cache, in
which case we might take the new value, or we might ignore it). In the case of a
non-canonical key passed to Add, we can canonicalize it first and then proceed
(my slight preference), or we can see that it is not canonical, and just do
nothing.

[Rémi Piotaix, 2014-09-02 19:20:58]

On 2014/09/02 18:13:45, reed1 wrote:
> I think we can formalize the idea of "make a subset rect canonical or fail".
> Meaning given a bitmap and a "requested" subset rect, we can compute a
canonical
> subset rect (i.e. intersect it with the bitmap bounds). That is the "right"
rect
> to use for keys, or the rect is empty, in which case we reject it.
> 
> 1. What to do with Find? We can canonicalize the caller's rect and then
search,
> or we can just pass it in as is, and if they have not "canonicalized" it
> themselves, they will always get a miss.

We can't canonicalize it in the Find method because we don't have the source
bitmap (we only have its getID)

> 
> 2. What to do with Add? We already have the case where Add (in the general
> sense) can be a no-opt (e.g. the specified key already exists in the cache, in
> which case we might take the new value, or we might ignore it). In the case of
a
> non-canonical key passed to Add, we can canonicalize it first and then proceed
> (my slight preference), or we can see that it is not canonical, and just do
> nothing.

I Implemented a solution where, if the rect don't have the same size as the
bitmap, the Add method return false.
But maybe the correct solution would be to just resize it...

https://codereview.chromium.org/518983002/diff/60001/src/core/SkBitmapCache.cpp
File src/core/SkBitmapCache.cpp (right):

https://codereview.chromium.org/518983002/diff/60001/src/core/SkBitmapCache.c...
src/core/SkBitmapCache.cpp:108: return Add(genID, SkIRect::MakeWH(width,
height), result);
Because width and height must be the ones of 'result' maybe we can just change
the sig. to Add(uint32_t, const SkBitmap&) ?

https://codereview.chromium.org/518983002/diff/60001/src/core/SkBitmapCache.c...
src/core/SkBitmapCache.cpp:112: BitmapKey key(genID, SK_Scalar1, SK_Scalar1,
subset);
Do we need to add the check on the Find methods? (if the subset is not correct,
it will just return false)

[Justin Novosad, 2014-09-02 19:30:55]

On 2014/09/02 19:20:58, Rémi Piotaix wrote:
> 
> I Implemented a solution where, if the rect don't have the same size as the
> bitmap, the Add method return false.
> But maybe the correct solution would be to just resize it...
> 

If the calling code is expected to cannonicalize, then perhaps Add() should be
more aggressive and assert that the subset rect is contained by the bitmap
bounds.

[reed1, 2014-09-02 19:57:47]

Find -- good point that we can't verify that the subset intersects the src. That
is probably fine, but it makes the case for definitely canonicalizing the subset
in Add all the more.

Add -- not 100% sure about returning a bool (since underlying Add doesn't yet).
I think I vote to keep it void for now.

To compute the canonical subset, you need to do the following

bitmap_bounds = 0, 0, bm.width, bm.height
if (local_subset.intersect(bitmap_bounds, user_subset)) {
    ... go ahead and Add
}

[Rémi Piotaix, 2014-09-02 20:27:01]

On 2014/09/02 19:57:47, reed1 wrote:
> Find -- good point that we can't verify that the subset intersects the src.
That
> is probably fine, but it makes the case for definitely canonicalizing the
subset
> in Add all the more.
> 
> Add -- not 100% sure about returning a bool (since underlying Add doesn't
yet).
> I think I vote to keep it void for now.
> 
> To compute the canonical subset, you need to do the following
> 
> bitmap_bounds = 0, 0, bm.width, bm.height
> if (local_subset.intersect(bitmap_bounds, user_subset)) {
>     ... go ahead and Add
> }

Done

[reed1, 2014-09-02 20:38:45]

https://codereview.chromium.org/518983002/diff/100001/src/core/SkBitmapCache.cpp
File src/core/SkBitmapCache.cpp (right):

https://codereview.chromium.org/518983002/diff/100001/src/core/SkBitmapCache....
src/core/SkBitmapCache.cpp:119: // We need to take only the width and height of
'subset' to compute the intersection
Actually the subset rect is very interesting because it can have non-zero
left/top (in the general case). Why are you explicitly zapping those?

[Rémi Piotaix, 2014-09-02 20:48:44]

https://codereview.chromium.org/518983002/diff/100001/src/core/SkBitmapCache.cpp
File src/core/SkBitmapCache.cpp (right):

https://codereview.chromium.org/518983002/diff/100001/src/core/SkBitmapCache....
src/core/SkBitmapCache.cpp:119: // We need to take only the width and height of
'subset' to compute the intersection
On 2014/09/02 20:38:45, reed1 wrote:
> Actually the subset rect is very interesting because it can have non-zero
> left/top (in the general case). Why are you explicitly zapping those?

Because the given subset is relative to the original bitmap from which the
'result' has been extracted.
If we extract, from a 100x100 bitmap, a subset at 90,90 of width 10x10, we will
have:
intersect(rect(0, 0, 10, 10), rect(90, 90, 10, 10)) which will be always empty.

[reed1, 2014-09-02 21:09:06]

Ah, I finally get it! Since we don't have the bloody original dimensions, we
can't perform an intersect test (either in Find or Add). Crud.

OK, I think perhaps an earlier version might have been right. I think the best
we can do to "legalize" the subset is:

1. ensure that top/left are not negative
2. ensure that subset and result have the same width/height.

Sorry for thrashing on this.

[Rémi Piotaix, 2014-09-02 21:16:56]

On 2014/09/02 21:09:06, reed1 wrote:
> Ah, I finally get it! Since we don't have the bloody original dimensions, we
> can't perform an intersect test (either in Find or Add). Crud.
> 
> OK, I think perhaps an earlier version might have been right. I think the best
> we can do to "legalize" the subset is:
> 
> 1. ensure that top/left are not negative
> 2. ensure that subset and result have the same width/height.
> 
> Sorry for thrashing on this.

So if we ensure that subset and result have the same width/height, maybe we can
omit the with and height parameters in Add(genID, width, height, result) ?

[reed1, 2014-09-02 21:21:06]

On 2014/09/02 21:16:56, Rémi Piotaix wrote:
> On 2014/09/02 21:09:06, reed1 wrote:
> > Ah, I finally get it! Since we don't have the bloody original dimensions, we
> > can't perform an intersect test (either in Find or Add). Crud.
> > 
> > OK, I think perhaps an earlier version might have been right. I think the
best
> > we can do to "legalize" the subset is:
> > 
> > 1. ensure that top/left are not negative
> > 2. ensure that subset and result have the same width/height.
> > 
> > Sorry for thrashing on this.
> 
> So if we ensure that subset and result have the same width/height, maybe we
can
> omit the with and height parameters in Add(genID, width, height, result) ?

Hmm, or remove that Find/Add, and always require an explicit rect (just to make
it clear what the intent is).

[Rémi Piotaix, 2014-09-03 13:53:10]

On 2014/09/02 21:21:06, reed1 wrote:
> On 2014/09/02 21:16:56, Rémi Piotaix wrote:
> > On 2014/09/02 21:09:06, reed1 wrote:
> > > Ah, I finally get it! Since we don't have the bloody original dimensions,
we
> > > can't perform an intersect test (either in Find or Add). Crud.
> > > 
> > > OK, I think perhaps an earlier version might have been right. I think the
> best
> > > we can do to "legalize" the subset is:
> > > 
> > > 1. ensure that top/left are not negative
> > > 2. ensure that subset and result have the same width/height.
> > > 
> > > Sorry for thrashing on this.
> > 
> > So if we ensure that subset and result have the same width/height, maybe we
> can
> > omit the with and height parameters in Add(genID, width, height, result) ?
> 
> Hmm, or remove that Find/Add, and always require an explicit rect (just to
make
> it clear what the intent is).

New patch, I'll remove Find/Add(_, width, height, _) in an other CL

[reed1, 2014-09-03 18:24:56]

lgtm

[Rémi Piotaix, 2014-09-03 18:25:43]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-09-03 18:26:25]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/518983002/120001

[commit-bot: I haz the power, 2014-09-03 18:33:18]

Committed patchset #7 (id:120001) as 42b0dfeb29e993b7fd247dcecff705d3dd4cf191