Don't delete CopyOutputRequests when queueing a new Surface frame.

Don't delete CopyOutputRequests when queueing a new Surface frame.

This reduces the risk that the CopyOutputRequests will be discarded
unnecessarily. This only affects CopyOutputRequests created through
SurfaceFactory::RequestCopyOfSurface, as requests submitted on
RenderPasses directly with the frame will still be discarded.


BUG=681988
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2695243006
Cr-Commit-Position: refs/heads/master@{#453807}
Committed: https://chromium.googlesource.com/chromium/src/+/37b0e1379938325304eeaff6afb9e7cb9f8bb618

[jbauman, 2017-02-18 00:38:28]

Description was changed from

==========
Don't delete CopyOutputRequests when queueing a new Surface frame.
==========

to

==========
Don't delete CopyOutputRequests when queueing a new Surface frame.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jbauman, 2017-02-18 00:38:34]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 00:40:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2017-02-18 01:54:46]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 01:55:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 03:57:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 03:57:48]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[jbauman, 2017-02-22 00:59:43]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 01:00:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 03:06:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 03:06:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[jbauman, 2017-02-23 02:16:44]

Description was changed from

==========
Don't delete CopyOutputRequests when queueing a new Surface frame.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Don't delete CopyOutputRequests when queueing a new Surface frame.

This reduces the risk that the CopyOutputRequests will be discarded
unnecessarily. This only affects CopyOutputRequests created through
SurfaceFactory::RequestCopyOfSurface, as requests submitted on
RenderPasses directly with the frame will still be discarded.


BUG=681988
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jbauman, 2017-02-23 02:18:42]

jbauman@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2017-02-23 22:52:28]

I'm concerned about the complexity of now having two stashes of copy requests.
Some questions

1) Why do we want the frame-based ones to be cancelled?

2) Could we pull the frame's requests off on submit and stash them in the
Surface, so we only have to look at once place in TakeCopyRequests and we don't
have multiple notions of requests?

https://codereview.chromium.org/2695243006/diff/40001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2695243006/diff/40001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:34: ClearCopyRequests();
Can you change this things name maybe so it doesn't look as redundant with the
new code below?

[jbauman, 2017-02-23 23:02:19]

On 2017/02/23 22:52:28, danakj wrote:
> I'm concerned about the complexity of now having two stashes of copy requests.
> Some questions
> 

The basic problem with storing frame-based ones for the next frame is they're
supposed to be on specific renderpasses. So if the renderpass disappears between
frames (very likely) or changes size or other properties then it wouldn't make
sense to try to do a copy of the new renderpass.

> 1) Why do we want the frame-based ones to be cancelled?
> 
> 2) Could we pull the frame's requests off on submit and stash them in the
> Surface, so we only have to look at once place in TakeCopyRequests and we
don't
> have multiple notions of requests?
> 
> https://codereview.chromium.org/2695243006/diff/40001/cc/surfaces/surface.cc
> File cc/surfaces/surface.cc (right):
> 
>
https://codereview.chromium.org/2695243006/diff/40001/cc/surfaces/surface.cc#...
> cc/surfaces/surface.cc:34: ClearCopyRequests();
> Can you change this things name maybe so it doesn't look as redundant with the
> new code below?

[chromium-reviews, 2017-02-23 23:19:10]

On Thu, Feb 23, 2017 at 6:02 PM, <jbauman@chromium.org> wrote:

> On 2017/02/23 22:52:28, danakj wrote:
> > I'm concerned about the complexity of now having two stashes of copy
> requests.
> > Some questions
> >
>
> The basic problem with storing frame-based ones for the next frame is
> they're
> supposed to be on specific renderpasses. So if the renderpass disappears
> between
> frames (very likely) or changes size or other properties then it wouldn't
> make
> sense to try to do a copy of the new renderpass.
>

Ah that's fair. What about just for the root requests? I think that would
still make an easier system to explain, wdyt?


>
>
> > 1) Why do we want the frame-based ones to be cancelled?
> >
> > 2) Could we pull the frame's requests off on submit and stash them in the
> > Surface, so we only have to look at once place in TakeCopyRequests and we
> don't
> > have multiple notions of requests?
> >
> > https://codereview.chromium.org/2695243006/diff/40001/cc/
> surfaces/surface.cc
> > File cc/surfaces/surface.cc (right):
> >
> >
> https://codereview.chromium.org/2695243006/diff/40001/cc/
> surfaces/surface.cc#newcode34
> > cc/surfaces/surface.cc:34: ClearCopyRequests();
> > Can you change this things name maybe so it doesn't look as redundant
> with the
> > new code below?
>
>
>
> https://codereview.chromium.org/2695243006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2017-02-24 00:21:01]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 00:21:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 00:24:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 00:24:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jbauman, 2017-02-24 01:09:46]

The CQ bit was checked by jbauman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 01:10:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 03:34:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 03:34:16]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-02-24 22:16:09]

PTAL. I've made it just save and copy over all requests on the root pass.

[danakj, 2017-02-28 17:33:15]

This is a lot simpler, thanks! LGTM

https://codereview.chromium.org/2695243006/diff/80001/cc/surfaces/surface_uni...
File cc/surfaces/surface_unittest.cc (right):

https://codereview.chromium.org/2695243006/diff/80001/cc/surfaces/surface_uni...
cc/surfaces/surface_unittest.cc:105:
surface->GetActiveFrame().render_pass_list.back()->copy_requests.size());
This is some serious pointer travelling, and I think this is covered by below
already, so I suggest maybe remove this line. It might be a pain to maintain.

[jbauman, 2017-02-28 22:59:33]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2017-02-28 23:00:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 00:18:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 00:18:32]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbauman, 2017-03-01 01:50:31]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2017-03-01 01:51:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 03:04:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488333031876880,
"parent_rev": "c75f6f0f5286abfacf96b9ef7ac088a9692ed897", "commit_rev":
"37b0e1379938325304eeaff6afb9e7cb9f8bb618"}

[commit-bot: I haz the power, 2017-03-01 03:05:08]

Description was changed from

==========
Don't delete CopyOutputRequests when queueing a new Surface frame.

This reduces the risk that the CopyOutputRequests will be discarded
unnecessarily. This only affects CopyOutputRequests created through
SurfaceFactory::RequestCopyOfSurface, as requests submitted on
RenderPasses directly with the frame will still be discarded.


BUG=681988
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Don't delete CopyOutputRequests when queueing a new Surface frame.

This reduces the risk that the CopyOutputRequests will be discarded
unnecessarily. This only affects CopyOutputRequests created through
SurfaceFactory::RequestCopyOfSurface, as requests submitted on
RenderPasses directly with the frame will still be discarded.


BUG=681988
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2695243006
Cr-Commit-Position: refs/heads/master@{#453807}
Committed:
https://chromium.googlesource.com/chromium/src/+/37b0e1379938325304eeaff6afb9...
==========

[commit-bot: I haz the power, 2017-03-01 03:05:10]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/37b0e1379938325304eeaff6afb9...