Retain references to surfaces from both active AND pending CompositorFrames

Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

BUG=672962
TBR=jam@chromium.org for mechanical renaming in content/
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2688043002
Cr-Commit-Position: refs/heads/master@{#449543}
Committed: https://chromium.googlesource.com/chromium/src/+/033d9e89ed5ce8b5ee20d7a5d56256c0038fd9f5

[Fady Samuel, 2017-02-10 00:02:30]

Description was changed from

==========
Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

BUG=672062
==========

to

==========
Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

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

[Fady Samuel, 2017-02-10 00:02:53]

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

[commit-bot: I haz the power, 2017-02-10 00:03:27]

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

[Fady Samuel, 2017-02-10 00:03:36]

fsamuel@chromium.org changed reviewers:
+ vmpstr@chromium.org

[Fady Samuel, 2017-02-10 00:03:37]

+vmpstr@ for review.
+rjkroege@, +kylechar@ FYI

[vmpstr, 2017-02-10 00:24:40]

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/compositor_fram...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/compositor_fram...
cc/surfaces/compositor_frame_sink_support.cc:95:
referenced_surfaces.insert(referenced_surfaces.end(),
This is, somewhat poor from an API perspective.

if pending_referenced_surfaces are indeed referenced surfaces of the surface,
then ::referenced_surfaces() should include it in its vector. If you want to
retain this distinction, then I think referenced_surfaces need to be renamed to
active_referenced_surfaces.

Line 93 also incurs a copy of a vector, which ideally is something we should try
and avoid. Is it reasonable for UpdateReferences to take two vectors?

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/compositor_fram...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/compositor_fram...
cc/surfaces/compositor_frame_sink_support.h:49: const ReferencedSurfaceTracker&
reference_tracker_for_testing() const {
just as a style thing (sorry I missed it in the previous review), I think we
prefer ReferenceTrackerForTesting[1] (and in general that for ForTesting)
functions, and could you put them at the end of the public section with "// For
testing functions." comment for the block.

[1]:
cc$ git gs 'for_testing(' | wc -l
301
cc$ git gs 'ForTesting(' | wc -l
1702

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/surface.cc#newc...
cc/surfaces/surface.cc:183: referenced_surfaces_ =
active_frame_->metadata.referenced_surfaces;
out of curiosity, why is this a member variable? If active_frame_ is already a
member, then we should just be returning
active_frame_->metadata.referenced_surfaces when requested? (I haven't searched
for all uses, I'm hoping you just know the answer)

[commit-bot: I haz the power, 2017-02-10 00:32:12]

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

[commit-bot: I haz the power, 2017-02-10 00:32:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2017-02-10 00:55:31]

PTAL Vlad!

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/compositor_fram...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/compositor_fram...
cc/surfaces/compositor_frame_sink_support.cc:95:
referenced_surfaces.insert(referenced_surfaces.end(),
On 2017/02/10 00:24:40, vmpstr wrote:
> This is, somewhat poor from an API perspective.
> 
> if pending_referenced_surfaces are indeed referenced surfaces of the surface,
> then ::referenced_surfaces() should include it in its vector. If you want to
> retain this distinction, then I think referenced_surfaces need to be renamed
to
> active_referenced_surfaces.
> 
> Line 93 also incurs a copy of a vector, which ideally is something we should
try
> and avoid. Is it reasonable for UpdateReferences to take two vectors?

I have renamed referenced_surfaces => active_referenced_surfaces and added a
TODO to avoid this copy.

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/compositor_fram...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/compositor_fram...
cc/surfaces/compositor_frame_sink_support.h:49: const ReferencedSurfaceTracker&
reference_tracker_for_testing() const {
On 2017/02/10 00:24:40, vmpstr wrote:
> just as a style thing (sorry I missed it in the previous review), I think we
> prefer ReferenceTrackerForTesting[1] (and in general that for ForTesting)
> functions, and could you put them at the end of the public section with "//
For
> testing functions." comment for the block.
> 
> [1]:
> cc$ git gs 'for_testing(' | wc -l
> 301
> cc$ git gs 'ForTesting(' | wc -l
> 1702

Done.

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2688043002/diff/1/cc/surfaces/surface.cc#newc...
cc/surfaces/surface.cc:183: referenced_surfaces_ =
active_frame_->metadata.referenced_surfaces;
On 2017/02/10 00:24:40, vmpstr wrote:
> out of curiosity, why is this a member variable? If active_frame_ is already a
> member, then we should just be returning
> active_frame_->metadata.referenced_surfaces when requested? (I haven't
searched
> for all uses, I'm hoping you just know the answer)

It isn't really. I can return a pointer to
active_frame_->metadata.referenced_surfaces if active_frame_ is available for
example, or a nullptr, but all callsites need to check for nullptr. I added a
TODO on the member variables to do that.

[Fady Samuel, 2017-02-10 01:06:51]

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

[commit-bot: I haz the power, 2017-02-10 01:07:13]

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

[vmpstr, 2017-02-10 01:15:37]

cc lgtm

[Fady Samuel, 2017-02-10 01:54:48]

fsamuel@chromium.org changed reviewers:
+ jam@chromium.org

[Fady Samuel, 2017-02-10 01:54:49]

TBR'ing jam@ for a mechanical renaming in content/

[Fady Samuel, 2017-02-10 01:55:14]

Description was changed from

==========
Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

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

to

==========
Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

BUG=672062
TBR=jam@chromium.org for mechanical renaming in content/
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-02-10 03:22:07]

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

[commit-bot: I haz the power, 2017-02-10 03:22:08]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-02-10 03:25:39]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2017-02-10 03:26:04]

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-10 03:30:33]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486697139694620,
"parent_rev": "ee80e6800ef427209e043de32330c99b243a0037", "commit_rev":
"033d9e89ed5ce8b5ee20d7a5d56256c0038fd9f5"}

[commit-bot: I haz the power, 2017-02-10 03:31:09]

Description was changed from

==========
Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

BUG=672062
TBR=jam@chromium.org for mechanical renaming in content/
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

BUG=672062
TBR=jam@chromium.org for mechanical renaming in content/
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2688043002
Cr-Commit-Position: refs/heads/master@{#449543}
Committed:
https://chromium.googlesource.com/chromium/src/+/033d9e89ed5ce8b5ee20d7a5d562...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/033d9e89ed5ce8b5ee20d7a5d562...

[Fady Samuel, 2017-02-13 16:44:40]

Description was changed from

==========
Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

BUG=672062
TBR=jam@chromium.org for mechanical renaming in content/
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2688043002
Cr-Commit-Position: refs/heads/master@{#449543}
Committed:
https://chromium.googlesource.com/chromium/src/+/033d9e89ed5ce8b5ee20d7a5d562...
==========

to

==========
Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

BUG=672962
TBR=jam@chromium.org for mechanical renaming in content/
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2688043002
Cr-Commit-Position: refs/heads/master@{#449543}
Committed:
https://chromium.googlesource.com/chromium/src/+/033d9e89ed5ce8b5ee20d7a5d562...
==========