Avoid Copying damage rect when using Overlays

Avoid Copying damage rect when using Overlays

When using Overlays, the damage rect can be completely covered by
the layer being composited by Overlay. We still copy damage rect
on to the current surface(Primary frame buffer) in BufferQueue. This
should be un-necessary.

BUG=370522, 548452
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2904998edff0cfcaab43eeb3b1db013d6169bd17
Cr-Commit-Position: refs/heads/master@{#356352}

Committed: https://crrev.com/5ee13b006abdf74d0f6b3fbdde2d936c00ad97e7
Cr-Commit-Position: refs/heads/master@{#356570}

Committed: https://crrev.com/050e8dff0fd427ab33d8adb9e9b1d12cd2682067
Cr-Commit-Position: refs/heads/master@{#358434}

[kalyank, 2015-09-03 07:30:07]

kalyan.kondapally@intel.com changed reviewers:
+ achaulk@chromium.org, alexst@chromium.org, danakj@chromium.org

[kalyank, 2015-09-03 08:00:53]

Looking for some early feedback, if this approach is correct. With these
changes, I see graphics artifacts when video is using Hardware Overlay and we
scroll the page.

[alexst (slow to review), 2015-09-03 14:08:23]

alexst@chromium.org changed reviewers:
+ ccameron@chromium.org

[alexst (slow to review), 2015-09-03 14:09:28]

On 2015/09/03 08:00:53, kalyank wrote:
> Looking for some early feedback, if this approach is correct. With these
> changes, I see graphics artifacts when video is using Hardware Overlay and we
> scroll the page.

This is a popular topic these days.

We started it here https://codereview.chromium.org/1044093005/, and now
ccameron@ is trying to do more of this for OSX. 

We really need a general solution for this and should combine everyone's
efforts.

[kalyank, 2015-09-03 17:34:31]

On 2015/09/03 14:09:28, alexst wrote:
> On 2015/09/03 08:00:53, kalyank wrote:
> > Looking for some early feedback, if this approach is correct. With these
> > changes, I see graphics artifacts when video is using Hardware Overlay and
we
> > scroll the page.
> 
> This is a popular topic these days.
> 
> We started it here https://codereview.chromium.org/1044093005/, and now

Thanks for sharing. Will go through the CL. 

> ccameron@ is trying to do more of this for OSX. 
> 
> We really need a general solution for this and should combine everyone's
> efforts.
Ya, that would be good.

[alexst (slow to review), 2015-09-03 18:35:47]

On 2015/09/03 17:34:31, kalyank wrote:
> On 2015/09/03 14:09:28, alexst wrote:
> > On 2015/09/03 08:00:53, kalyank wrote:
> > > Looking for some early feedback, if this approach is correct. With these
> > > changes, I see graphics artifacts when video is using Hardware Overlay and
> we
> > > scroll the page.
> > 
> > This is a popular topic these days.
> > 
> > We started it here https://codereview.chromium.org/1044093005/, and now
> 
> Thanks for sharing. Will go through the CL. 
> 
> > ccameron@ is trying to do more of this for OSX. 
> > 
> > We really need a general solution for this and should combine everyone's
> > efforts.
> Ya, that would be good.

I CC'd you on Chris's bug, let me know if you didn't get it.

[kalyank, 2015-09-03 18:49:11]

On 2015/09/03 18:35:47, alexst wrote:
> On 2015/09/03 17:34:31, kalyank wrote:
> > On 2015/09/03 14:09:28, alexst wrote:
> > > On 2015/09/03 08:00:53, kalyank wrote:
> > > > Looking for some early feedback, if this approach is correct. With these
> > > > changes, I see graphics artifacts when video is using Hardware Overlay
and
> > we
> > > > scroll the page.
> > > 
> > > This is a popular topic these days.
> > > 
> > > We started it here https://codereview.chromium.org/1044093005/, and now
> > 
> > Thanks for sharing. Will go through the CL. 
> > 
> > > ccameron@ is trying to do more of this for OSX. 
> > > 
> > > We really need a general solution for this and should combine everyone's
> > > efforts.
> > Ya, that would be good.
> 
> I CC'd you on Chris's bug, let me know if you didn't get it.

Ya, got the notification.

[kalyank, 2015-10-06 20:14:15]

kalyan.kondapally@intel.com changed reviewers:
+ piman@chromium.org

[kalyank, 2015-10-07 04:35:46]

@ccameron, I see that in ImageTransportSurfaceOverlayMac, we actually 
remove Overlay rect from Damage rect. We could do it in DirectRenderer 
itself, so both ChromeOS and Mac could use this functionality. 
WDYT?

[kalyank, 2015-10-07 21:45:27]

On 2015/10/07 04:35:46, kalyank wrote:
> @ccameron, I see that in ImageTransportSurfaceOverlayMac, we actually 
> remove Overlay rect from Damage rect. We could do it in DirectRenderer 
> itself, so both ChromeOS and Mac could use this functionality. 
> WDYT?

Uploaded corresponding change. Now, the damage rect passed to 
SwapBuffers is frame.root_damage_rect - Overaly Rect.
Can you PTAL.

[ccameron, 2015-10-07 21:53:32]

This lgtm

[kalyank, 2015-10-07 22:04:23]

On 2015/10/07 21:53:32, ccameron wrote:
> This lgtm

@alexst, @piman ping

[piman, 2015-10-08 22:51:31]

https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
cc/output/direct_renderer.cc:248:
frame.root_damage_rect.Subtract(ToNearestRect(overlay.display_rect));
That's only true if the overlay is opaque. Also, ToNearestRect looks a bit
sloppy, could cause issues for non-screen-aligned rects. It seems like this
should really be handled by OverlayProcessor, or even the various
OverlayStrategy which have a clearer grasp on the exact intent. I'm comfortable
passing the damage rect down to there.

[kalyank, 2015-10-12 22:58:46]

https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
cc/output/direct_renderer.cc:248:
frame.root_damage_rect.Subtract(ToNearestRect(overlay.display_rect));
On 2015/10/08 22:51:31, piman (slow to review) wrote:
> That's only true if the overlay is opaque. Also, ToNearestRect looks a bit
> sloppy, could cause issues for non-screen-aligned rects. It seems like this
> should really be handled by OverlayProcessor, or even the various
> OverlayStrategy which have a clearer grasp on the exact intent. I'm
comfortable
> passing the damage rect down to there.

Acknowledged.

https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
cc/output/direct_renderer.cc:248:
frame.root_damage_rect.Subtract(ToNearestRect(overlay.display_rect));
On 2015/10/08 22:51:31, piman (slow to review) wrote:
> That's only true if the overlay is opaque. Also, ToNearestRect looks a bit
> sloppy, could cause issues for non-screen-aligned rects. It seems like this
> should really be handled by OverlayProcessor, or even the various
> OverlayStrategy which have a clearer grasp on the exact intent. I'm
comfortable
> passing the damage rect down to there.

Moved this to processor now. I have added a bool to candidate to know if it
needs blending. If blending is needed, we dont subtract the candidates display
rect from root damage rect.

Reg ToNearestRect, which should be the right conversion API ?

[kalyank, 2015-10-19 17:14:22]

@piman, @danakj ping

[piman, 2015-10-20 01:49:19]

https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
cc/output/direct_renderer.cc:248:
frame.root_damage_rect.Subtract(ToNearestRect(overlay.display_rect));
On 2015/10/12 22:58:45, kalyank wrote:
> On 2015/10/08 22:51:31, piman (slow to review) wrote:
> > That's only true if the overlay is opaque. Also, ToNearestRect looks a bit
> > sloppy, could cause issues for non-screen-aligned rects. It seems like this
> > should really be handled by OverlayProcessor, or even the various
> > OverlayStrategy which have a clearer grasp on the exact intent. I'm
> comfortable
> > passing the damage rect down to there.
> 
> Moved this to processor now. I have added a bool to candidate to know if it
> needs blending. If blending is needed, we dont subtract the candidates display
> rect from root damage rect.
> 
> Reg ToNearestRect, which should be the right conversion API ?

Whatever damage you remove must be fully covered by the opaque overlay. So that
means the opaque overlay rect must enclose the removed rect. So it seems like
ToEnclosedRect is the right conservative choice?

[kalyank, 2015-10-20 09:55:48]

On 2015/10/20 01:49:19, piman (slow to review) wrote:
>
https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
> File cc/output/direct_renderer.cc (right):
> 
>
https://codereview.chromium.org/1330563004/diff/100001/cc/output/direct_rende...
> cc/output/direct_renderer.cc:248:
> frame.root_damage_rect.Subtract(ToNearestRect(overlay.display_rect));
> On 2015/10/12 22:58:45, kalyank wrote:
> > On 2015/10/08 22:51:31, piman (slow to review) wrote:
> > > That's only true if the overlay is opaque. Also, ToNearestRect looks a bit
> > > sloppy, could cause issues for non-screen-aligned rects. It seems like
this
> > > should really be handled by OverlayProcessor, or even the various
> > > OverlayStrategy which have a clearer grasp on the exact intent. I'm
> > comfortable
> > > passing the damage rect down to there.
> > 
> > Moved this to processor now. I have added a bool to candidate to know if it
> > needs blending. If blending is needed, we dont subtract the candidates
display
> > rect from root damage rect.
> > 
> > Reg ToNearestRect, which should be the right conversion API ?
> 
> Whatever damage you remove must be fully covered by the opaque overlay. So
that
> means the opaque overlay rect must enclose the removed rect. So it seems like
> ToEnclosedRect is the right conservative choice?

Done,also added a test. PTAL

[kalyank, 2015-10-23 17:50:49]

@piman ping

[piman, 2015-10-23 17:59:43]

lgtm

[kalyank, 2015-10-23 18:03:50]

On 2015/10/23 17:59:43, piman (slow to review) wrote:
> lgtm

@ccameron can you PTAL as things have changed a bit.

[kalyank, 2015-10-27 16:11:41]

On 2015/10/23 18:03:50, kalyank wrote:
> On 2015/10/23 17:59:43, piman (slow to review) wrote:
> > lgtm
> 
> @ccameron can you PTAL as things have changed a bit.

@ccameron ping

[ccameron, 2015-10-27 16:15:20]

On 2015/10/27 16:11:41, kalyank wrote:
> On 2015/10/23 18:03:50, kalyank wrote:
> > On 2015/10/23 17:59:43, piman (slow to review) wrote:
> > > lgtm
> > 
> > @ccameron can you PTAL as things have changed a bit.
> 
> @ccameron ping

lgtm

[kalyank, 2015-10-27 17:16:07]

The CQ bit was checked by kalyan.kondapally@intel.com

[commit-bot: I haz the power, 2015-10-27 17:17:37]

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

[commit-bot: I haz the power, 2015-10-27 18:36:09]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-10-27 18:37:16]

Patchset 12 (id:??) landed as
https://crrev.com/2904998edff0cfcaab43eeb3b1db013d6169bd17
Cr-Commit-Position: refs/heads/master@{#356352}

[oshima, 2015-10-27 23:12:41]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1414923006/ by oshima@chromium.org.

The reason for reverting is: The reason for revert:

use-of-uninitiailzed value is reported on msan bot.

Filed crbug.com/548452


[ RUN      ] SandwichTest.MultiQuadOverlay
==7008==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f1d2774a400 in
cc::OverlayProcessor::ProcessForOverlays(cc::ResourceProvider*,
cc::ScopedPtrVector\u003Ccc::RenderPass>*,
std::__1::vector\u003Ccc::OverlayCandidate,
std::__1::allocator\u003Ccc::OverlayCandidate> >*, gfx::Rect*)
cc/output/overlay_processor.cc:45:13
    #1 0x7f1d259f6e5e in cc::(anonymous
namespace)::SandwichTest_MultiQuadOverlay_Test::TestBody()
cc/output/overlay_unittest.cc:617:3
    #2 0x7f1d274a3922 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test,
void> testing/gtest/src/gtest.cc:2458:12.

[kalyank, 2015-10-28 09:40:43]

Description was changed from

==========
Avoid Copying damage rect when using Overlays

When using Overlays, the damage rect can be completely covered by
the layer being composited by Overlay. We still copy damage rect
on to the current surface(Primary frame buffer) in BufferQueue. This
should be un-necessary.

BUG=370522
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2904998edff0cfcaab43eeb3b1db013d6169bd17
Cr-Commit-Position: refs/heads/master@{#356352}
==========

to

==========
Avoid Copying damage rect when using Overlays

When using Overlays, the damage rect can be completely covered by
the layer being composited by Overlay. We still copy damage rect
on to the current surface(Primary frame buffer) in BufferQueue. This
should be un-necessary.

BUG=370522
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2904998edff0cfcaab43eeb3b1db013d6169bd17
Cr-Commit-Position: refs/heads/master@{#356352}
==========

[kalyank, 2015-10-28 09:42:11]

Description was changed from

==========
Avoid Copying damage rect when using Overlays

When using Overlays, the damage rect can be completely covered by
the layer being composited by Overlay. We still copy damage rect
on to the current surface(Primary frame buffer) in BufferQueue. This
should be un-necessary.

BUG=370522
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2904998edff0cfcaab43eeb3b1db013d6169bd17
Cr-Commit-Position: refs/heads/master@{#356352}
==========

to

==========
Avoid Copying damage rect when using Overlays

When using Overlays, the damage rect can be completely covered by
the layer being composited by Overlay. We still copy damage rect
on to the current surface(Primary frame buffer) in BufferQueue. This
should be un-necessary.

BUG=370522,548452
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2904998edff0cfcaab43eeb3b1db013d6169bd17
Cr-Commit-Position: refs/heads/master@{#356352}
==========

[kalyank, 2015-10-28 16:36:15]

On 2015/10/27 23:12:41, oshima wrote:
> A revert of this CL (patchset #12 id:220001) has been created in
> https://codereview.chromium.org/1414923006/ by mailto:oshima@chromium.org.
> 
> The reason for reverting is: The reason for revert:
> 
> use-of-uninitiailzed value is reported on msan bot.
> 
> Filed crbug.com/548452
> 
> 
> [ RUN      ] SandwichTest.MultiQuadOverlay
> ==7008==WARNING: MemorySanitizer: use-of-uninitialized-value
>     #0 0x7f1d2774a400 in
> cc::OverlayProcessor::ProcessForOverlays(cc::ResourceProvider*,
> cc::ScopedPtrVector\u003Ccc::RenderPass>*,
> std::__1::vector\u003Ccc::OverlayCandidate,
> std::__1::allocator\u003Ccc::OverlayCandidate> >*, gfx::Rect*)
> cc/output/overlay_processor.cc:45:13
>     #1 0x7f1d259f6e5e in cc::(anonymous
> namespace)::SandwichTest_MultiQuadOverlay_Test::TestBody()
> cc/output/overlay_unittest.cc:617:3
>     #2 0x7f1d274a3922 in
HandleExceptionsInMethodIfSupported\u003Ctesting::Test,
> void> testing/gtest/src/gtest.cc:2458:12.

@oshima, was missing a member variable initialization and this issue is fixed
with
the updated patch. There seem to be UI and Browser test failures but unrelated
to this
patch.

[piman, 2015-10-28 16:55:31]

lgtm

[kalyank, 2015-10-28 17:01:31]

The CQ bit was checked by kalyan.kondapally@intel.com

[kalyank, 2015-10-28 17:01:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1330563004/#ps240001
(title: "Initialize member variable")

[commit-bot: I haz the power, 2015-10-28 17:03:09]

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

[commit-bot: I haz the power, 2015-10-28 17:07:23]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-10-28 17:09:21]

Patchset 13 (id:??) landed as
https://crrev.com/5ee13b006abdf74d0f6b3fbdde2d936c00ad97e7
Cr-Commit-Position: refs/heads/master@{#356570}

[ccameron, 2015-10-30 23:26:40]

On 2015/10/28 17:09:21, commit-bot: I haz the power wrote:
> Patchset 13 (id:??) landed as
> https://crrev.com/5ee13b006abdf74d0f6b3fbdde2d936c00ad97e7
> Cr-Commit-Position: refs/heads/master@{#356570}

This will need a revert, once https://codereview.chromium.org/1422423003/ is
reverted. This broke YouTube on Mac
BUG=549350

[ccameron, 2015-10-30 23:30:28]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/1408953005/ by ccameron@chromium.org.

The reason for reverting is: This broke YouTube on Mac, in particular the
sandwich overlay strategy.
BUG=549350.

[kalyank, 2015-10-30 23:51:04]

Description was changed from

==========
Avoid Copying damage rect when using Overlays

When using Overlays, the damage rect can be completely covered by
the layer being composited by Overlay. We still copy damage rect
on to the current surface(Primary frame buffer) in BufferQueue. This
should be un-necessary.

BUG=370522,548452
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2904998edff0cfcaab43eeb3b1db013d6169bd17
Cr-Commit-Position: refs/heads/master@{#356352}

Committed: https://crrev.com/5ee13b006abdf74d0f6b3fbdde2d936c00ad97e7
Cr-Commit-Position: refs/heads/master@{#356570}
==========

to

==========
Avoid Copying damage rect when using Overlays

When using Overlays, the damage rect can be completely covered by
the layer being composited by Overlay. We still copy damage rect
on to the current surface(Primary frame buffer) in BufferQueue. This
should be un-necessary.

BUG=370522,548452
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2904998edff0cfcaab43eeb3b1db013d6169bd17
Cr-Commit-Position: refs/heads/master@{#356352}

Committed: https://crrev.com/5ee13b006abdf74d0f6b3fbdde2d936c00ad97e7
Cr-Commit-Position: refs/heads/master@{#356570}
==========

[kalyank, 2015-11-02 16:55:23]

On 2015/10/30 23:30:28, ccameron wrote:
> A revert of this CL (patchset #13 id:240001) has been created in
> https://codereview.chromium.org/1408953005/ by mailto:ccameron@chromium.org.
> 
> The reason for reverting is: This broke YouTube on Mac, in particular the
> sandwich overlay strategy.
> BUG=549350.

I have changed it so that damage_rect is passed down to strategy, which should
have definitive picture of how the quads are composited by overlays. It's upto
each strategy as to how it wants to deal with it. Also, removed needs_blending
boolean as FromDrawQuad(in OverlayCandidate) already rejects quads which need
blending
support.
@piman @ccameron can you PTAL

[piman, 2015-11-03 03:42:43]

lgtm

[kalyank, 2015-11-04 18:11:21]

On 2015/11/03 03:42:43, piman (slow to review) wrote:
> lgtm

@ccameron ping

[ccameron, 2015-11-04 19:33:11]

On 2015/11/04 18:11:21, kalyank wrote:
> On 2015/11/03 03:42:43, piman (slow to review) wrote:
> > lgtm
> 
> @ccameron ping

I don't feel confident in this -- I'd rather that this be pushed down to the
output surface, the way it is done on Mac (which, IIRC, was the original patch
to do this) -- changing with the damage rect can have unexpected consequences,
and makes the system harder to reason about.

If we *are* going to adjust the damage rect, we should do a full recalculation
of it, with the replaced quads taken into account.

[kalyank, 2015-11-04 21:03:33]

On 2015/11/04 19:33:11, ccameron wrote:
> On 2015/11/04 18:11:21, kalyank wrote:
> > On 2015/11/03 03:42:43, piman (slow to review) wrote:
> > > lgtm
> > 
> > @ccameron ping
> 
> I don't feel confident in this -- I'd rather that this be pushed down to the
> output surface, the way it is done on Mac (which, IIRC, was the original patch
> to do this) -- changing with the damage rect can have unexpected consequences,
> and makes the system harder to reason about.

The way Mac does it, we will still be doing some unnecessary gl rendering of
primary
buffer equal to damage rect ? 

> If we *are* going to adjust the damage rect, we should do a full recalculation
> of it, with the replaced quads taken into account.

K. This was the very reason, I changed so that strategy can adjust the damage
rect 
depending on how they promote quads to overlays. If I understand it correct you
think
we should re-calculated the whole damage rect after the quads promoted to
Overlays are 
removed from the pass.

[piman, 2015-11-04 23:00:48]

On Wed, Nov 4, 2015 at 11:33 AM, <ccameron@chromium.org> wrote:

> On 2015/11/04 18:11:21, kalyank wrote:
>
>> On 2015/11/03 03:42:43, piman (slow to review) wrote:
>> > lgtm
>>
>
> @ccameron ping
>>
>
> I don't feel confident in this -- I'd rather that this be pushed down to
> the
> output surface, the way it is done on Mac (which, IIRC, was the original
> patch
> to do this) -- changing with the damage rect can have unexpected
> consequences,
> and makes the system harder to reason about.
>
> If we *are* going to adjust the damage rect, we should do a full
> recalculation
> of it, with the replaced quads taken into account.
>

We don't have per-quad damage information (and indeed, maintaining it is
costly when you may skip frames).

I think of this in terms of damage culling in the same way we do in the
compositor, so it doesn't bother me outright.


>
> https://codereview.chromium.org/1330563004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kalyank, 2015-11-05 20:33:00]

On 2015/11/04 23:00:48, piman (slow to review) wrote:
> On Wed, Nov 4, 2015 at 11:33 AM, <mailto:ccameron@chromium.org> wrote:
> 
> > On 2015/11/04 18:11:21, kalyank wrote:
> >
> >> On 2015/11/03 03:42:43, piman (slow to review) wrote:
> >> > lgtm
> >>
> >
> > @ccameron ping
> >>
> >
> > I don't feel confident in this -- I'd rather that this be pushed down to
> > the
> > output surface, the way it is done on Mac (which, IIRC, was the original
> > patch
> > to do this) -- changing with the damage rect can have unexpected
> > consequences,
> > and makes the system harder to reason about.
> >
> > If we *are* going to adjust the damage rect, we should do a full
> > recalculation
> > of it, with the replaced quads taken into account.
> >
> 
> We don't have per-quad damage information (and indeed, maintaining it is
> costly when you may skip frames).

Ya, also the calculations done here might be void depending on what the given
strategy is doing i.e. replacing existing quads or adding new transparent quads.

I would prefer the current approach and let individual strategy ensure the
damage
rect is correct  (Taking into account any new quads it's adding or which get
promoted to overlays etc).

> I think of this in terms of damage culling in the same way we do in the
> compositor, so it doesn't bother me outright.
> 
> 
> >
> > https://codereview.chromium.org/1330563004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[kalyank, 2015-11-06 16:57:49]

Ok to commit this ?

[ccameron, 2015-11-06 16:59:07]

On 2015/11/06 16:57:49, kalyank wrote:
> Ok to commit this ?

Yes, OK by me

[ccameron, 2015-11-06 19:06:05]

On 2015/11/04 23:00:48, piman (slow to review) wrote:
> On Wed, Nov 4, 2015 at 11:33 AM, <mailto:ccameron@chromium.org> wrote:
> 
> > On 2015/11/04 18:11:21, kalyank wrote:
> >
> >> On 2015/11/03 03:42:43, piman (slow to review) wrote:
> >> > lgtm
> >>
> >
> > @ccameron ping
> >>
> >
> > I don't feel confident in this -- I'd rather that this be pushed down to
> > the
> > output surface, the way it is done on Mac (which, IIRC, was the original
> > patch
> > to do this) -- changing with the damage rect can have unexpected
> > consequences,
> > and makes the system harder to reason about.
> >
> > If we *are* going to adjust the damage rect, we should do a full
> > recalculation
> > of it, with the replaced quads taken into account.
> >
> 
> We don't have per-quad damage information (and indeed, maintaining it is
> costly when you may skip frames).
> 
> I think of this in terms of damage culling in the same way we do in the
> compositor, so it doesn't bother me outright.

I guess I feel strange about doing an empty PostSubBuffer -- I worry that it
will trigger various parts of the pipeline to elide it. In principle, this can
be protected by tests, but that seems to me to grow complexity. The
ImageTransportSurface layer seems safer in that it has a better picture of
everything, and can be more surgical about effects.

Anyway, still okay to commit by me, just want to register my "feels like we're
doing this the hard way" opinion, in case it causes any other second thoughts.

[piman, 2015-11-06 19:28:21]

On Fri, Nov 6, 2015 at 11:06 AM, <ccameron@chromium.org> wrote:

> On 2015/11/04 23:00:48, piman (slow to review) wrote:
>
>> On Wed, Nov 4, 2015 at 11:33 AM, <mailto:ccameron@chromium.org> wrote:
>>
>
> > On 2015/11/04 18:11:21, kalyank wrote:
>> >
>> >> On 2015/11/03 03:42:43, piman (slow to review) wrote:
>> >> > lgtm
>> >>
>> >
>> > @ccameron ping
>> >>
>> >
>> > I don't feel confident in this -- I'd rather that this be pushed down to
>> > the
>> > output surface, the way it is done on Mac (which, IIRC, was the original
>> > patch
>> > to do this) -- changing with the damage rect can have unexpected
>> > consequences,
>> > and makes the system harder to reason about.
>> >
>> > If we *are* going to adjust the damage rect, we should do a full
>> > recalculation
>> > of it, with the replaced quads taken into account.
>> >
>>
>
> We don't have per-quad damage information (and indeed, maintaining it is
>> costly when you may skip frames).
>>
>
> I think of this in terms of damage culling in the same way we do in the
>> compositor, so it doesn't bother me outright.
>>
>
> I guess I feel strange about doing an empty PostSubBuffer -- I worry that
> it
> will trigger various parts of the pipeline to elide it. In principle, this
> can
> be protected by tests, but that seems to me to grow complexity. The
> ImageTransportSurface layer seems safer in that it has a better picture of
> everything, and can be more surgical about effects.
>

ImageTransportSurface is essentially not used anymore on anything but mac.
One could argue that we could put the logic in the GLSurface
implementation, but that would mean a duplicate implementation on each
platform, whereas it seems to me that the logic isn't platform-specific,
and so doing it in cc lets us test it more easily (as evidenced by the new
unit test in this CL as opposed to the, ahem, lack of them at the
ImageTransportSurface/GLSurface level).

Fair point about empty PostSubBuffer, it might make sense to have a
separate CommitOverlayPlanes that makes no changes to the framebuffer.


>
> Anyway, still okay to commit by me, just want to register my "feels like
> we're
> doing this the hard way" opinion, in case it causes any other second
> thoughts.
>
> https://codereview.chromium.org/1330563004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kalyank, 2015-11-06 22:43:54]

The CQ bit was checked by kalyan.kondapally@intel.com

[kalyank, 2015-11-06 22:43:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1330563004/#ps280001
(title: "Remove blending checks")

[commit-bot: I haz the power, 2015-11-06 22:44:27]

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

[commit-bot: I haz the power, 2015-11-06 22:51:42]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-11-06 22:52:38]

Patchset 15 (id:??) landed as
https://crrev.com/050e8dff0fd427ab33d8adb9e9b1d12cd2682067
Cr-Commit-Position: refs/heads/master@{#358434}