cc: Plumb more details about pixel refs to tile manager.

cc: Plumb more details about pixel refs to tile manager.

This patch plumbs the full pixel ref information (including matrix and
rect) to tile manager. Currently, we still just use the pixel ref
itself, but this will change.

Also modifies unittests to test the rect to be in the expected area.
Changing the unittests also revealed a few bugs that this patch also
addresses.

R=danakj, enne, weiliangc
BUG=516751
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/66d74d497c608b8b3349597d2c82dea030c0c8be
Cr-Commit-Position: refs/heads/master@{#343285}

[vmpstr, 2015-08-06 20:20:30]

Please take a look.

[ericrk, 2015-08-07 00:33:55]

ericrk@chromium.org changed reviewers:
+ ericrk@chromium.org

[ericrk, 2015-08-07 00:33:56]

https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.cc
File cc/playback/pixel_ref_map.cc (right):

https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.c...
cc/playback/pixel_ref_map.cc:64: // positions here
nit: This second comment is a bit hard to follow. From your first comment it
seems pretty clear that we want to store positions relative to layer, not sure
we need this additional comment.

[weiliangc, 2015-08-07 22:56:00]

https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.cc
File cc/playback/pixel_ref_map.cc (right):

https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.c...
cc/playback/pixel_ref_map.cc:49: // We recorded the picture as if it was at (0,
0) by translating by layer
Do you mean in skia::PixelRefUtils::GatherDiscardablePixelRefs? Isn't the line
that translates picture by layer rect origin removed?

https://codereview.chromium.org/1279843004/diff/1/skia/ext/pixel_ref_utils.cc
File skia/ext/pixel_ref_utils.cc (right):

https://codereview.chromium.org/1279843004/diff/1/skia/ext/pixel_ref_utils.cc...
skia/ext/pixel_ref_utils.cc:372:
empty_bitmap.setInfo(SkImageInfo::MakeUnknown(picture_ibounds.right(),
Just to double check, this won't need more memory right?

[vmpstr, 2015-08-10 17:36:04]

vmpstr@chromium.org changed reviewers:
+ reed@google.com

[vmpstr, 2015-08-10 17:36:05]

PTAL

https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.cc
File cc/playback/pixel_ref_map.cc (right):

https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.c...
cc/playback/pixel_ref_map.cc:49: // We recorded the picture as if it was at (0,
0) by translating by layer
On 2015/08/07 22:56:00, weiliangc wrote:
> Do you mean in skia::PixelRefUtils::GatherDiscardablePixelRefs? Isn't the line
> that translates picture by layer rect origin removed?

No I mean here at record time when we record the picture we offset it by the
layer rect origin. Here's kind of a picture:

a---------------a
|.b-----------b.|
|.|.....c---c.|.|
|.|.....|...|.|.|
|.|.....c---c.|.|
|.b-----------b.|
|...............|
a---------------a

"a" is layer rect here, and within it we have a picture "b". When we record, we
offset by -(b's origin) so internally (in the picture) b's origin is (0, 0).
Then inside the picture we also have a cull rect "c", which is the bounds of any
useful information within the picture. The line that I removed in pixel ref
utils was the offset by -(c's origin), since we want to get the pixel_ref_rect
relative to b's origin. Here, however, the final rect that we want is relative
to a's origin, so I add the layer rect's origin back in. 

Does that make sense? Or is that more confusing? :P

https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.c...
cc/playback/pixel_ref_map.cc:64: // positions here
On 2015/08/07 00:33:56, ericrk wrote:
> nit: This second comment is a bit hard to follow. From your first comment it
> seems pretty clear that we want to store positions relative to layer, not sure
> we need this additional comment.

Ya sorry this was an unfinished comment that evolved into the above comment.
Removed this one.

https://codereview.chromium.org/1279843004/diff/1/skia/ext/pixel_ref_utils.cc
File skia/ext/pixel_ref_utils.cc (right):

https://codereview.chromium.org/1279843004/diff/1/skia/ext/pixel_ref_utils.cc...
skia/ext/pixel_ref_utils.cc:372:
empty_bitmap.setInfo(SkImageInfo::MakeUnknown(picture_ibounds.right(),
On 2015/08/07 22:56:00, weiliangc wrote:
> Just to double check, this won't need more memory right?

It shouldn't, since we don't allocate any memory for this. +reed to ensure my
understanding is correct.

[weiliangc, 2015-08-10 17:51:32]

On 2015/08/10 at 17:36:05, vmpstr wrote:
> PTAL
> 
> https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.cc
> File cc/playback/pixel_ref_map.cc (right):
> 
>
https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.c...
> cc/playback/pixel_ref_map.cc:49: // We recorded the picture as if it was at
(0, 0) by translating by layer
> On 2015/08/07 22:56:00, weiliangc wrote:
> > Do you mean in skia::PixelRefUtils::GatherDiscardablePixelRefs? Isn't the
line
> > that translates picture by layer rect origin removed?
> 
> No I mean here at record time when we record the picture we offset it by the
layer rect origin. Here's kind of a picture:
> 
> a---------------a
> |.b-----------b.|
> |.|.....c---c.|.|
> |.|.....|...|.|.|
> |.|.....c---c.|.|
> |.b-----------b.|
> |...............|
> a---------------a
> 
> "a" is layer rect here, and within it we have a picture "b". When we record,
we offset by -(b's origin) so internally (in the picture) b's origin is (0, 0).
Then inside the picture we also have a cull rect "c", which is the bounds of any
useful information within the picture. The line that I removed in pixel ref
utils was the offset by -(c's origin), since we want to get the pixel_ref_rect
relative to b's origin. Here, however, the final rect that we want is relative
to a's origin, so I add the layer rect's origin back in. 
> 
> Does that make sense? Or is that more confusing? :P
> 

So I think I know what you meant, but my mental model is still a bit different?
I thought picture's size is same with layer rect's size?
So say a is layer rect (a_x, a_y, a_width by a_height), won't picture b be (0,
0, a_width, a_height), and c is changed from (0,0, c_width by c_height) to
(c_x_wrt_b, c_y_wrt_b, c_width by c_height), but we still need to add (a_x, a_y)
to b?

Thanks for explaining this with a ascii art. :)
>
https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.c...
> cc/playback/pixel_ref_map.cc:64: // positions here
> On 2015/08/07 00:33:56, ericrk wrote:
> > nit: This second comment is a bit hard to follow. From your first comment it
> > seems pretty clear that we want to store positions relative to layer, not
sure
> > we need this additional comment.
> 
> Ya sorry this was an unfinished comment that evolved into the above comment.
Removed this one.
> 
> https://codereview.chromium.org/1279843004/diff/1/skia/ext/pixel_ref_utils.cc
> File skia/ext/pixel_ref_utils.cc (right):
> 
>
https://codereview.chromium.org/1279843004/diff/1/skia/ext/pixel_ref_utils.cc...
> skia/ext/pixel_ref_utils.cc:372:
empty_bitmap.setInfo(SkImageInfo::MakeUnknown(picture_ibounds.right(),
> On 2015/08/07 22:56:00, weiliangc wrote:
> > Just to double check, this won't need more memory right?
> 
> It shouldn't, since we don't allocate any memory for this. +reed to ensure my
understanding is correct.

Cool, thanks!

[vmpstr, 2015-08-10 17:58:10]

On 2015/08/10 17:51:32, weiliangc wrote:
> On 2015/08/10 at 17:36:05, vmpstr wrote:
> > PTAL
> > 
> >
https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.cc
> > File cc/playback/pixel_ref_map.cc (right):
> > 
> >
>
https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.c...
> > cc/playback/pixel_ref_map.cc:49: // We recorded the picture as if it was at
> (0, 0) by translating by layer
> > On 2015/08/07 22:56:00, weiliangc wrote:
> > > Do you mean in skia::PixelRefUtils::GatherDiscardablePixelRefs? Isn't the
> line
> > > that translates picture by layer rect origin removed?
> > 
> > No I mean here at record time when we record the picture we offset it by the
> layer rect origin. Here's kind of a picture:
> > 
> > a---------------a
> > |.b-----------b.|
> > |.|.....c---c.|.|
> > |.|.....|...|.|.|
> > |.|.....c---c.|.|
> > |.b-----------b.|
> > |...............|
> > a---------------a
> > 
> > "a" is layer rect here, and within it we have a picture "b". When we record,
> we offset by -(b's origin) so internally (in the picture) b's origin is (0,
0).
> Then inside the picture we also have a cull rect "c", which is the bounds of
any
> useful information within the picture. The line that I removed in pixel ref
> utils was the offset by -(c's origin), since we want to get the pixel_ref_rect
> relative to b's origin. Here, however, the final rect that we want is relative
> to a's origin, so I add the layer rect's origin back in. 
> > 
> > Does that make sense? Or is that more confusing? :P
> > 
> 
> So I think I know what you meant, but my mental model is still a bit
different?
> I thought picture's size is same with layer rect's size?
> So say a is layer rect (a_x, a_y, a_width by a_height), won't picture b be (0,
> 0, a_width, a_height), and c is changed from (0,0, c_width by c_height) to
> (c_x_wrt_b, c_y_wrt_b, c_width by c_height), but we still need to add (a_x,
a_y)
> to b?

Ya that makes more sense :P You're right, I thought we removed some padding or
something but a and b do have the same size, but b is at (0, 0) whereas a is at
wherever we actually record. So pixel_ref_rect here is relative to b, but it
needs to be relative to a. 

Does the comment still make sense or did you want me to update it?

> 
> Thanks for explaining this with a ascii art. :)
> >
>
https://codereview.chromium.org/1279843004/diff/1/cc/playback/pixel_ref_map.c...
> > cc/playback/pixel_ref_map.cc:64: // positions here
> > On 2015/08/07 00:33:56, ericrk wrote:
> > > nit: This second comment is a bit hard to follow. From your first comment
it
> > > seems pretty clear that we want to store positions relative to layer, not
> sure
> > > we need this additional comment.
> > 
> > Ya sorry this was an unfinished comment that evolved into the above comment.
> Removed this one.
> > 
> >
https://codereview.chromium.org/1279843004/diff/1/skia/ext/pixel_ref_utils.cc
> > File skia/ext/pixel_ref_utils.cc (right):
> > 
> >
>
https://codereview.chromium.org/1279843004/diff/1/skia/ext/pixel_ref_utils.cc...
> > skia/ext/pixel_ref_utils.cc:372:
> empty_bitmap.setInfo(SkImageInfo::MakeUnknown(picture_ibounds.right(),
> > On 2015/08/07 22:56:00, weiliangc wrote:
> > > Just to double check, this won't need more memory right?
> > 
> > It shouldn't, since we don't allocate any memory for this. +reed to ensure
my
> understanding is correct.
> 
> Cool, thanks!

[reed1, 2015-08-10 17:58:38]

https://codereview.chromium.org/1279843004/diff/20001/cc/playback/pixel_ref_m...
File cc/playback/pixel_ref_map.cc (right):

https://codereview.chromium.org/1279843004/diff/20001/cc/playback/pixel_ref_m...
cc/playback/pixel_ref_map.cc:49: // We recorded the picture as if it was at (0,
0) by translating by layer
"We recorded the picture as if it was at (0, 0) by translating by layer rect
origin."

I like the comment, but it feels like there is implicit knowledge here, and that
if blink changed its behavior somehow, this assumption might break. Perhaps not,
and I am misunderstanding the larger picture.

On the topic of larger picture, is this an equivalent way to express the flow:?

- GatherPixelRefs job is to return a set of rectangles in global/device
coordinates, so they can be compared against tile boundaries
- To do this in a single picture, we can either transform the local bounds by
the CTM (per-call) or we can record the CTM per image.
- For nested pictures, that themselves might be transformed, we need to account
for the CTM at the time the sub-pictures was instantiated.

If this is approximately correct, perhaps a more obvious CL would be to pass the
CTM as a parameter to GatherPixelsRefs. Then on sub-pictures, it could update
that matrix before it does the recursive call on the sub-picture.

https://codereview.chromium.org/1279843004/diff/20001/skia/ext/pixel_ref_util...
File skia/ext/pixel_ref_utils.cc (right):

https://codereview.chromium.org/1279843004/diff/20001/skia/ext/pixel_ref_util...
skia/ext/pixel_ref_utils.cc:386:
empty_bitmap.setInfo(SkImageInfo::MakeUnknown(picture_ibounds.right(),
it might justify a comment here, why we're using right/bottom instead of
width/height. Its clear in this CL, but that's because I can see (on the left)
that we used to performa a translate.

[weiliangc, 2015-08-10 18:09:45]

Overall LGTM, with comment about comment. :P

https://codereview.chromium.org/1279843004/diff/20001/cc/playback/pixel_ref_m...
File cc/playback/pixel_ref_map.cc (right):

https://codereview.chromium.org/1279843004/diff/20001/cc/playback/pixel_ref_m...
cc/playback/pixel_ref_map.cc:49: // We recorded the picture as if it was at (0,
0) by translating by layer
On 2015/08/10 at 17:58:38, reed1 wrote:
> "We recorded the picture as if it was at (0, 0) by translating by layer rect
origin."
> 
> I like the comment, but it feels like there is implicit knowledge here, and
that if blink changed its behavior somehow, this assumption might break. Perhaps
not, and I am misunderstanding the larger picture.
> 
> On the topic of larger picture, is this an equivalent way to express the
flow:?
> 
> - GatherPixelRefs job is to return a set of rectangles in global/device
coordinates, so they can be compared against tile boundaries
> - To do this in a single picture, we can either transform the local bounds by
the CTM (per-call) or we can record the CTM per image.
> - For nested pictures, that themselves might be transformed, we need to
account for the CTM at the time the sub-pictures was instantiated.
> 
> If this is approximately correct, perhaps a more obvious CL would be to pass
the CTM as a parameter to GatherPixelsRefs. Then on sub-pictures, it could
update that matrix before it does the recursive call on the sub-picture.

My understanding is that Pixel Refs are in layer/picture space, and we gather
them per SkPicture?

https://codereview.chromium.org/1279843004/diff/20001/cc/playback/pixel_ref_m...
cc/playback/pixel_ref_map.cc:50: // rect origin. However, now the matrix stores
this offset, and the rect is
Overall comment is good. I was mostly confused by "However, now the matrix
stores this offset, and the rect is also offset." What is "the matrix" and "the
rect" here? Maybe just remove this sentence is ok?

[vmpstr, 2015-08-11 21:00:49]

PTAL

https://codereview.chromium.org/1279843004/diff/20001/cc/playback/pixel_ref_m...
File cc/playback/pixel_ref_map.cc (right):

https://codereview.chromium.org/1279843004/diff/20001/cc/playback/pixel_ref_m...
cc/playback/pixel_ref_map.cc:49: // We recorded the picture as if it was at (0,
0) by translating by layer
On 2015/08/10 17:58:38, reed1 wrote:
> "We recorded the picture as if it was at (0, 0) by translating by layer rect
> origin."
> 
> I like the comment, but it feels like there is implicit knowledge here, and
that
> if blink changed its behavior somehow, this assumption might break. Perhaps
not,
> and I am misunderstanding the larger picture.

The translation happens during record time in the compositor, so it's not really
blink that's responsible for the offset. I think this part of the code is
sufficiently tied to the recording side of this that this should be OK (ie, if
the recording code changes it should be obvious that other parts of the same
system might need updating). 

> 
> On the topic of larger picture, is this an equivalent way to express the
flow:?
> 
> - GatherPixelRefs job is to return a set of rectangles in global/device
> coordinates, so they can be compared against tile boundaries
> - To do this in a single picture, we can either transform the local bounds by
> the CTM (per-call) or we can record the CTM per image.
> - For nested pictures, that themselves might be transformed, we need to
account
> for the CTM at the time the sub-pictures was instantiated.
> 
> If this is approximately correct, perhaps a more obvious CL would be to pass
the
> CTM as a parameter to GatherPixelsRefs. Then on sub-pictures, it could update
> that matrix before it does the recursive call on the sub-picture.

GatherPixelRefs should return information relative to the given picture (ie as
if the picture was at (0, 0)). It does return a CTM for each of the pixel refs
in position_pixel_ref, as well as the rect. The rect is properly returned as
being at (0, 0)... I think it's nicer though to make it be relative to the
layer, since picture pile's calling code isn't aware of picture pile's
implementation details. I think technically CTM could be updated as well, but
I'm a bit hesitant to mess with it, since we really only need scale out of it
(for decoding), and the actual position doesn't really affect the decode.

The subpicture process (if this picture contains another picture) should also be
handled naturally by this, since the returning values are going to be relative
to the top level picture.

https://codereview.chromium.org/1279843004/diff/20001/cc/playback/pixel_ref_m...
cc/playback/pixel_ref_map.cc:50: // rect origin. However, now the matrix stores
this offset, and the rect is
On 2015/08/10 18:09:45, weiliangc wrote:
> Overall comment is good. I was mostly confused by "However, now the matrix
> stores this offset, and the rect is also offset." What is "the matrix" and
"the
> rect" here? Maybe just remove this sentence is ok?

Done.

https://codereview.chromium.org/1279843004/diff/20001/skia/ext/pixel_ref_util...
File skia/ext/pixel_ref_utils.cc (right):

https://codereview.chromium.org/1279843004/diff/20001/skia/ext/pixel_ref_util...
skia/ext/pixel_ref_utils.cc:386:
empty_bitmap.setInfo(SkImageInfo::MakeUnknown(picture_ibounds.right(),
On 2015/08/10 17:58:38, reed1 wrote:
> it might justify a comment here, why we're using right/bottom instead of
> width/height. Its clear in this CL, but that's because I can see (on the left)
> that we used to performa a translate.

Done.

[vmpstr, 2015-08-12 20:36:32]

friendly ping

[reed1, 2015-08-13 14:02:23]

skia lgtm

[vmpstr, 2015-08-13 20:29:11]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-08-13 20:29:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
weiliangc@chromium.org
Link to the patchset: https://codereview.chromium.org/1279843004/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-08-13 20:29:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1279843004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1279843004/60001

[commit-bot: I haz the power, 2015-08-13 22:08:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-13 22:08:46]

Patchset 4 (id:??) landed as
https://crrev.com/66d74d497c608b8b3349597d2c82dea030c0c8be
Cr-Commit-Position: refs/heads/master@{#343285}