Don't use a low-quality filter when painting images while others are animating.

Don't use a low-quality filter when painting images while others are animating.

Previously, any image animating would trigger low-quality filters for all
images during the animation timer window. Now only do so for images that are
actually animating.

BUG=583238
Committed: https://crrev.com/066567c230db336e44f10cc1a6a2c94be2982d0a
Cr-Commit-Position: refs/heads/master@{#374738}

[chrishtr, 2016-02-09 22:50:57]

Description was changed from

==========
none


none


none


none


none


none

BUG=
==========

to

==========
Don't use a low-quality filter when painting images while others are animating.

Previously, any image animating would trigger low-quality filters for all
images during the animation timer window. Now only do so for images that are
actually animating.

BUG=583238
==========

[chrishtr, 2016-02-09 22:54:43]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2016-02-10 04:05:46]

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ImageQualityController.cpp (right):

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ImageQualityController.cpp:194:
set(object, innerMap, layer, layoutSize, false);
Should this 'false' be 'objectIsAnimating' to avoid resetting the animating bit?

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ImageQualityController.h (right):

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ImageQualityController.h:46: typedef
HashMap<const void*, LayoutSize> LayerSizeMap;
The purpose of LayerSizeMap and ObjectLayerSizeMap appears to be a mapping:
(Image*, Layer*) -> LayoutSize (size that the image is drawn at)
This means that an image which is resizing in one layer does not necessarily get
drawn at low quality in other layers. This patch is an improvement by not having
a global resize bit, but it will introduce subtle edge-case bugs if a single
image is drawn into two layers where only one is animating. WDYT about moving
the 'animating' bit to the LayerSizeMap instead of the ObjectLayerSizeMap?

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ImageQualityController.h:47: // Maps from
LayoutObject to <LayerSizeMap, LayoutObject is animating>
Do you mind updating this to use a struct so we can use better names at the
callsites? Also maybe 'resizing' instead of 'animating' to disambiguate
Image::maybeAnimated()?

The best I could come up with is a -Info suffix :/ I accept 10 minutes in the
shame cube as punishment.
struct ResizeInfo {
    LayerSizeMap layerSizeMap;
    bool objectIsResizing;
};
typedef HashMap<const LayoutObject*, ResizeInfo> ObjectResizeInfoMap;

[chrishtr, 2016-02-10 17:08:18]

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ImageQualityController.cpp (right):

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ImageQualityController.cpp:194:
set(object, innerMap, layer, layoutSize, false);
On 2016/02/10 at 04:05:46, pdr wrote:
> Should this 'false' be 'objectIsAnimating' to avoid resetting the animating
bit?

Done.

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ImageQualityController.h (right):

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ImageQualityController.h:46: typedef
HashMap<const void*, LayoutSize> LayerSizeMap;
On 2016/02/10 at 04:05:46, pdr wrote:
> The purpose of LayerSizeMap and ObjectLayerSizeMap appears to be a mapping:
> (Image*, Layer*) -> LayoutSize (size that the image is drawn at)
> This means that an image which is resizing in one layer does not necessarily
get drawn at low quality in other layers.

Before this patch, if any layer of any object was detected to be resizing, all
of them
would paint at low quality for a period.

 This patch is an improvement by not having a global resize bit, but it will
introduce subtle edge-case bugs if a single image is drawn into two layers where
only one is animating. WDYT about moving the 'animating' bit to the LayerSizeMap
instead of the ObjectLayerSizeMap?

I had written the patch that way in the first pass, but moved the bit to
ObjectLayerSizeMap. I concluded that it makes the code simpler to keep it
per-LayoutObject, and that the use case of animating one layer of painting of an
object but not another may not even be possible (I didn't think very hard about
it; maybe one backgrouund image layered on another painted by the same object?),
and also not so important.

https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ImageQualityController.h:47: // Maps from
LayoutObject to <LayerSizeMap, LayoutObject is animating>
On 2016/02/10 at 04:05:46, pdr wrote:
> Do you mind updating this to use a struct so we can use better names at the
callsites? Also maybe 'resizing' instead of 'animating' to disambiguate
Image::maybeAnimated()?
> 
> The best I could come up with is a -Info suffix :/ I accept 10 minutes in the
shame cube as punishment.
> struct ResizeInfo {
>     LayerSizeMap layerSizeMap;
>     bool objectIsResizing;
> };
> typedef HashMap<const LayoutObject*, ResizeInfo> ObjectResizeInfoMap;

Good idea, I can do that. Let's agree on the question you raised above and then
I will
implement the struct.

[pdr., 2016-02-10 18:28:21]

On 2016/02/10 at 17:08:18, chrishtr wrote:
>
https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ImageQualityController.cpp (right):
> 
>
https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ImageQualityController.cpp:194:
set(object, innerMap, layer, layoutSize, false);
> On 2016/02/10 at 04:05:46, pdr wrote:
> > Should this 'false' be 'objectIsAnimating' to avoid resetting the animating
bit?
> 
> Done.
> 
>
https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ImageQualityController.h (right):
> 
>
https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ImageQualityController.h:46: typedef
HashMap<const void*, LayoutSize> LayerSizeMap;
> On 2016/02/10 at 04:05:46, pdr wrote:
> > The purpose of LayerSizeMap and ObjectLayerSizeMap appears to be a mapping:
> > (Image*, Layer*) -> LayoutSize (size that the image is drawn at)
> > This means that an image which is resizing in one layer does not necessarily
get drawn at low quality in other layers.
> 
> Before this patch, if any layer of any object was detected to be resizing, all
of them
> would paint at low quality for a period.
> 
>  This patch is an improvement by not having a global resize bit, but it will
introduce subtle edge-case bugs if a single image is drawn into two layers where
only one is animating. WDYT about moving the 'animating' bit to the LayerSizeMap
instead of the ObjectLayerSizeMap?
> 
> I had written the patch that way in the first pass, but moved the bit to
ObjectLayerSizeMap. I concluded that it makes the code simpler to keep it
per-LayoutObject, and that the use case of animating one layer of painting of an
object but not another may not even be possible (I didn't think very hard about
it; maybe one backgrouund image layered on another painted by the same object?),
and also not so important.
> 
>
https://codereview.chromium.org/1685933002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ImageQualityController.h:47: // Maps
from LayoutObject to <LayerSizeMap, LayoutObject is animating>
> On 2016/02/10 at 04:05:46, pdr wrote:
> > Do you mind updating this to use a struct so we can use better names at the
callsites? Also maybe 'resizing' instead of 'animating' to disambiguate
Image::maybeAnimated()?
> > 
> > The best I could come up with is a -Info suffix :/ I accept 10 minutes in
the shame cube as punishment.
> > struct ResizeInfo {
> >     LayerSizeMap layerSizeMap;
> >     bool objectIsResizing;
> > };
> > typedef HashMap<const LayoutObject*, ResizeInfo> ObjectResizeInfoMap;
> 
> Good idea, I can do that. Let's agree on the question you raised above and
then I will
> implement the struct.

SGTM, lets have it on the ObjectLayerSizeMap after all.

[chrishtr, 2016-02-10 19:04:08]

Updated to use a struct.

[pdr., 2016-02-10 19:06:30]

LGTM

https://codereview.chromium.org/1685933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ImageQualityController.cpp (right):

https://codereview.chromium.org/1685933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ImageQualityController.cpp:195:
set(object, innerMap, layer, layoutSize, true);
Should this be objectIsResizing instead of true?

[chrishtr, 2016-02-10 19:16:33]

https://codereview.chromium.org/1685933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ImageQualityController.cpp (right):

https://codereview.chromium.org/1685933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ImageQualityController.cpp:195:
set(object, innerMap, layer, layoutSize, true);
On 2016/02/10 at 19:06:30, pdr wrote:
> Should this be objectIsResizing instead of true?

No, because otherwise the callback in
ImageQualityController::highQualityRepaintTimerFired won't necessarily
invalidate
the object at the end of the frame resize.

[pdr., 2016-02-10 19:22:37]

On 2016/02/10 at 19:16:33, chrishtr wrote:
>
https://codereview.chromium.org/1685933002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/ImageQualityController.cpp (right):
> 
>
https://codereview.chromium.org/1685933002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/ImageQualityController.cpp:195:
set(object, innerMap, layer, layoutSize, true);
> On 2016/02/10 at 19:06:30, pdr wrote:
> > Should this be objectIsResizing instead of true?
> 
> No, because otherwise the callback in
ImageQualityController::highQualityRepaintTimerFired won't necessarily
invalidate
> the object at the end of the frame resize.

LGTM

[chrishtr, 2016-02-10 19:24:17]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2016-02-10 19:24:41]

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

[commit-bot: I haz the power, 2016-02-10 21:57:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:31:26]

Description was changed from

==========
Don't use a low-quality filter when painting images while others are animating.

Previously, any image animating would trigger low-quality filters for all
images during the animation timer window. Now only do so for images that are
actually animating.

BUG=583238
==========

to

==========
Don't use a low-quality filter when painting images while others are animating.

Previously, any image animating would trigger low-quality filters for all
images during the animation timer window. Now only do so for images that are
actually animating.

BUG=583238

Committed: https://crrev.com/066567c230db336e44f10cc1a6a2c94be2982d0a
Cr-Commit-Position: refs/heads/master@{#374738}
==========

[commit-bot: I haz the power, 2016-02-16 22:31:27]

Patchset 6 (id:??) landed as
https://crrev.com/066567c230db336e44f10cc1a6a2c94be2982d0a
Cr-Commit-Position: refs/heads/master@{#374738}