Use a cc::Display for WebView resourceless software draws.

Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOTRY=true

Committed: https://crrev.com/554c98ac020b61bd06fc498759ae24dfa321fe0f
Cr-Commit-Position: refs/heads/master@{#404272}

[danakj, 2016-07-06 22:31:58]

Description was changed from

==========
Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
==========

to

==========
Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-07-06 22:32:04]

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

[danakj, 2016-07-06 22:33:06]

Confirming with bots, and 1 TODO in the Display

[commit-bot: I haz the power, 2016-07-06 22:33:08]

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

[danakj, 2016-07-06 23:52:04]

TODO resolved. Tests passed locally.

[danakj, 2016-07-06 23:52:11]

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

[commit-bot: I haz the power, 2016-07-06 23:53:06]

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

[enne (OOO), 2016-07-06 23:54:03]

I think my only weak concern here is having multiple SurfaceManagers around,
just in the sense of anybody ever trying to fix surface ids from different
managers.  This has bitten me before, but maybe this only happens here inside of
SynchronousCompositorOutputSurface, so maybe it's ok?

[danakj, 2016-07-06 23:58:06]

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

[danakj, 2016-07-06 23:58:06]

> I think my only weak concern here is having multiple SurfaceManagers around,
> just in the sense of anybody ever trying to fix surface ids from different
> managers. This has bitten me before, but maybe this only happens here inside
of
> SynchronousCompositorOutputSurface, so maybe it's ok?

I think it might be okay here because it's a separate system from the rest?
(Also diff process but is that related?)

This output surface owns all the surface things, sets itself as the root
surface. I guess what breaks is that if you had nested surfaces that were meant
to work with another surface manager, they wouldn't work here? But that is okay
for us? plz confirm. Also +jbauman

[enne (OOO), 2016-07-07 00:00:08]

Yeah, I guess this SurfaceManager is in the renderer, so it seems super unlikely
to ever get misused.

I was also going to suggest to put the disable flag in CompositorFrameMetadata,
so excellent.  :)

lgtm

[danakj, 2016-07-07 00:03:56]

danakj@chromium.org changed reviewers:
+ dcheng@chromium.org, sievers@chromium.org

[danakj, 2016-07-07 00:03:57]

+sievers for content/
+dcheng for cc/ipc/

[boliu, 2016-07-07 00:37:54]

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:21: #include
"cc/output/texture_mailbox_deleter.h"
is this needed?

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:158:
base::MakeUnique<SoftwareDevice>(&current_sw_canvas_)),
just confirming c++11 knowledge :p MakeUnique is doing the perfect forwarding
thingy in c++11?

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:160:
display_->Initialize(&display_client_);
GLRenderer used to be created on the stack

if this stuff lives for a long time, can the Display hold onto software
resources for longer than before? I assume shared_bitmap_manager_ is the thing
sitting in between compositor and display that maybe can hold onto software
resources?

Also I assume these objects by themsleves don't use that much memory, right?

Basically should always optimize for the case of not having any software draws
of any kind

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:199: if
(in_software_draw_) {
should be ok just skip this and return the resources if it's fallback tick? no
worse than before though I guess

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.h (right):

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.h:21: #include
"cc/surfaces/display.h"
should be ok to forward declare

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.h:130: bool
registered_ = false;
inline init is just ok for all constant values now?

[danakj, 2016-07-07 00:46:30]

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:21: #include
"cc/output/texture_mailbox_deleter.h"
On 2016/07/07 00:37:54, boliu wrote:
> is this needed?

Ya, it's written as nullptr, but it fails to compile otherwise cuz it's making a
unique_ptr<TMD>

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:158:
base::MakeUnique<SoftwareDevice>(&current_sw_canvas_)),
On 2016/07/07 00:37:54, boliu wrote:
> just confirming c++11 knowledge :p MakeUnique is doing the perfect forwarding
> thingy in c++11?

It is a function call, so it returns an rvalue, if that is what you're asking.
(Since the SoftwareOutputSurface constructor is taking a unique_ptr by value,
and it's move-only, it wouldn't compile otherwise :))

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:160:
display_->Initialize(&display_client_);
On 2016/07/07 00:37:54, boliu wrote:
> GLRenderer used to be created on the stack
> 
> if this stuff lives for a long time, can the Display hold onto software
> resources for longer than before? I assume shared_bitmap_manager_ is the thing
> sitting in between compositor and display that maybe can hold onto software
> resources?
> 
> Also I assume these objects by themsleves don't use that much memory, right?
> 
> Basically should always optimize for the case of not having any software draws
> of any kind

SoftwareRenderer::DrawPictureQuad doesn't store anything onto the
SoftwareRenderer, so I don't think deleting the renderer is going to free
anything.

The SharedBitmapManager would actually only be used if the compositor was
sending quads with bitmap resources in them. It's not.. so I could maybe make it
null, if you prefer. But SoftwareRenderer doesn't generate shared bitmaps, it
consumes them in SoftwareRenderer::DrawTextureQuad and DrawTileQuad.

The one exception to all that is non-root render surfaces, which we don't have
in resourceless mode.

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:199: if
(in_software_draw_) {
On 2016/07/07 00:37:54, boliu wrote:
> should be ok just skip this and return the resources if it's fallback tick? no
> worse than before though I guess

Don't produce a frame in fallback tick? So before we would have drawn
regardless, right? I think this is the same behaviour, or can you point out why
not?

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.h (right):

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.h:21: #include
"cc/surfaces/display.h"
On 2016/07/07 00:37:54, boliu wrote:
> should be ok to forward declare

Done.

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.h:130: bool
registered_ = false;
On 2016/07/07 00:37:54, boliu wrote:
> inline init is just ok for all constant values now?

For non-constant also. The contructor will set them to these values. I did it
for everything with a default value the constructor doesn't set dynamically.

[dcheng, 2016-07-07 00:50:26]

rs lgtm for //cc/ipc

[danakj, 2016-07-07 00:55:41]

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

[commit-bot: I haz the power, 2016-07-07 00:56:05]

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

[boliu, 2016-07-07 00:58:01]

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:160:
display_->Initialize(&display_client_);
On 2016/07/07 00:46:30, danakj wrote:
> On 2016/07/07 00:37:54, boliu wrote:
> > GLRenderer used to be created on the stack
> > 
> > if this stuff lives for a long time, can the Display hold onto software
> > resources for longer than before? I assume shared_bitmap_manager_ is the
thing
> > sitting in between compositor and display that maybe can hold onto software
> > resources?
> > 
> > Also I assume these objects by themsleves don't use that much memory, right?
> > 
> > Basically should always optimize for the case of not having any software
draws
> > of any kind
> 
> SoftwareRenderer::DrawPictureQuad doesn't store anything onto the
> SoftwareRenderer, so I don't think deleting the renderer is going to free
> anything.
> 
> The SharedBitmapManager would actually only be used if the compositor was
> sending quads with bitmap resources in them. It's not.. so I could maybe make
it
> null, if you prefer.

Yes please.

> But SoftwareRenderer doesn't generate shared bitmaps, it
> consumes them in SoftwareRenderer::DrawTextureQuad and DrawTileQuad.
> 
> The one exception to all that is non-root render surfaces, which we don't have
> in resourceless mode.

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:199: if
(in_software_draw_) {
On 2016/07/07 00:46:30, danakj wrote:
> On 2016/07/07 00:37:54, boliu wrote:
> > should be ok just skip this and return the resources if it's fallback tick?
no
> > worse than before though I guess
> 
> Don't produce a frame in fallback tick? So before we would have drawn
> regardless, right? I think this is the same behaviour, or can you point out
why
> not?

This is the same behavior. I'm just pointing out a potential optimization this
CL enables. Can totally do it in a follow up.

[commit-bot: I haz the power, 2016-07-07 03:34:32]

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

[commit-bot: I haz the power, 2016-07-07 03:34:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[danakj, 2016-07-07 20:34:08]

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:160:
display_->Initialize(&display_client_);
On 2016/07/07 00:58:01, boliu wrote:
> On 2016/07/07 00:46:30, danakj wrote:
> > On 2016/07/07 00:37:54, boliu wrote:
> > > GLRenderer used to be created on the stack
> > > 
> > > if this stuff lives for a long time, can the Display hold onto software
> > > resources for longer than before? I assume shared_bitmap_manager_ is the
> thing
> > > sitting in between compositor and display that maybe can hold onto
software
> > > resources?
> > > 
> > > Also I assume these objects by themsleves don't use that much memory,
right?
> > > 
> > > Basically should always optimize for the case of not having any software
> draws
> > > of any kind
> > 
> > SoftwareRenderer::DrawPictureQuad doesn't store anything onto the
> > SoftwareRenderer, so I don't think deleting the renderer is going to free
> > anything.
> > 
> > The SharedBitmapManager would actually only be used if the compositor was
> > sending quads with bitmap resources in them. It's not.. so I could maybe
make
> it
> > null, if you prefer.
> 
> Yes please.

OK I think that worked. Trying it @ bots now. PTAL

https://codereview.chromium.org/2128113002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:199: if
(in_software_draw_) {
On 2016/07/07 00:58:01, boliu wrote:
> On 2016/07/07 00:46:30, danakj wrote:
> > On 2016/07/07 00:37:54, boliu wrote:
> > > should be ok just skip this and return the resources if it's fallback
tick?
> no
> > > worse than before though I guess
> > 
> > Don't produce a frame in fallback tick? So before we would have drawn
> > regardless, right? I think this is the same behaviour, or can you point out
> why
> > not?
> 
> This is the same behavior. I'm just pointing out a potential optimization this
> CL enables. Can totally do it in a follow up.

OK ya let me send you a patch and we can see if I understand.

[boliu, 2016-07-07 21:12:15]

https://codereview.chromium.org/2128113002/diff/100001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2128113002/diff/100001/content/renderer/andro...
content/renderer/android/synchronous_compositor_output_surface.cc:390: const
cc::ReturnedResourceArray& resources) {
one more, should be safe to just do DCHECK(resources.empty()) or NOTREACHED
here, right?

[danakj, 2016-07-07 21:15:50]

PTAL

https://codereview.chromium.org/2128113002/diff/100001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2128113002/diff/100001/content/renderer/andro...
content/renderer/android/synchronous_compositor_output_surface.cc:390: const
cc::ReturnedResourceArray& resources) {
On 2016/07/07 21:12:14, boliu wrote:
> one more, should be safe to just do DCHECK(resources.empty()) or NOTREACHED
> here, right?

Ahhh um. Ya I guess so! SurfaceResourceHolder::UnrefResources appears to call it
unconditionally, so DCHECK empty.

[danakj, 2016-07-07 21:17:54]

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

[commit-bot: I haz the power, 2016-07-07 21:18:47]

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

[boliu, 2016-07-07 21:18:54]

https://codereview.chromium.org/2128113002/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2128113002/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_output_surface.cc:393:
client_->ReclaimResources(&ack);
then should certainly be safe to remove these 2 lines too, no?

[boliu, 2016-07-07 21:19:49]

SCOS lgtm either way

[Fady Samuel, 2016-07-07 21:23:08]

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

[Fady Samuel, 2016-07-07 21:23:09]

https://codereview.chromium.org/2128113002/diff/120001/cc/ipc/cc_param_traits...
File cc/ipc/cc_param_traits_macros.h (right):

https://codereview.chromium.org/2128113002/diff/120001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits_macros.h:188:
is_resourceless_software_draw_with_scroll_or_animation)
Please also update compositor_frame_metadata.mojom
https://cs.chromium.org/chromium/src/cc/ipc/compositor_frame_metadata.mojom?q...

[danakj, 2016-07-07 21:30:21]

https://codereview.chromium.org/2128113002/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2128113002/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_output_surface.cc:393:
client_->ReclaimResources(&ack);
On 2016/07/07 21:18:53, boliu wrote:
> then should certainly be safe to remove these 2 lines too, no?

The client can do more things in the method, so this is more risky.
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=0&l...

I think we should continue to send acks regardless.

[danakj, 2016-07-07 21:33:39]

The patchset with the null bitmap manager failed but it might be bot error..

[boliu, 2016-07-07 21:43:42]

On 2016/07/07 21:33:39, danakj wrote:
> The patchset with the null bitmap manager failed but it might be bot error..

Ok, that's a swarming timeout, which apparently means no logcat, so don't even
know which DCHECK it failed on

but looks like it's crashing all over the place, so maybe not that hart to hit,
let me try

[boliu, 2016-07-07 21:50:17]

signal 6 (SIGABRT), code -6 in tid 23465 (Compositor)^M
pid: 23135, tid: 23465, name: Compositor  >>> com.google.android.gm <<<^M
signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------^M
[FATAL:display.cc(55)] Check failed: texture_mailbox_deleter_. ^M

Stack Trace:
  RELADDR   FUNCTION            FILE:LINE
  000859d3  ~LogMessage         /android/chromium/src/base/logging.cc:532
  0000945f  Display             /android/chromium/src/cc/surfaces/display.cc:55
  00984df3  BindToClient       
/android/chromium/src/content/renderer/android/synchronous_compositor_output_surface.cc:156
  00150a6d  InitializeRenderer 
/android/chromium/src/cc/trees/layer_tree_host_impl.cc:2316

[danakj, 2016-07-07 21:59:15]

On Thu, Jul 7, 2016 at 2:50 PM, <boliu@chromium.org> wrote:

> signal 6 (SIGABRT), code -6 in tid 23465 (Compositor)^M
> pid: 23135, tid: 23465, name: Compositor >>> com.google.android.gm <<<^M
> signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------^M
> [FATAL:display.cc(55)] Check failed: texture_mailbox_deleter_. ^M
>
> Stack Trace:
> RELADDR FUNCTION FILE:LINE
> 000859d3 ~LogMessage /android/chromium/src/base/logging.cc:532
> 0000945f Display /android/chromium/src/cc/surfaces/display.cc:55
> 00984df3 BindToClient
>
>
/android/chromium/src/content/renderer/android/synchronous_compositor_output_surface.cc:156
> 00150a6d InitializeRenderer
> /android/chromium/src/cc/trees/layer_tree_host_impl.cc:2316
>

Ah.. ok I will just DCHECK that only when making GL renderer. So you don't
need it for software. Since it isn't used!

-- 
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.

[danakj, 2016-07-07 21:59:49]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-07-07 21:59:50]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
dcheng@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2128113002/#ps140001
(title: "displaywebview: dcheckmove")

[commit-bot: I haz the power, 2016-07-07 22:00:47]

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

[danakj, 2016-07-07 22:05:37]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-07-07 22:05:38]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
dcheng@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2128113002/#ps160001
(title: "displaywebview: mojom")

[commit-bot: I haz the power, 2016-07-07 22:06:26]

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

[danakj, 2016-07-07 22:09:01]

Updated the mojo stuff for CompositorFrameData too woops.

[commit-bot: I haz the power, 2016-07-08 00:07:03]

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

[commit-bot: I haz the power, 2016-07-08 00:07:04]

Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[danakj, 2016-07-08 00:12:43]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-07-08 00:13:16]

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

[danakj, 2016-07-08 00:18:07]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-07-08 00:18:14]

Description was changed from

==========
Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOTRY=true
==========

[danakj, 2016-07-08 00:18:20]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-07-08 00:19:19]

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, 2016-07-08 00:26:22]

Description was changed from

==========
Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOTRY=true
==========

to

==========
Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOTRY=true
==========

[commit-bot: I haz the power, 2016-07-08 00:26:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-08 00:26:49]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 00:29:40]

Description was changed from

==========
Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOTRY=true
==========

to

==========
Use a cc::Display for WebView resourceless software draws.

R=boliu, enne
BUG=606053
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOTRY=true

Committed: https://crrev.com/554c98ac020b61bd06fc498759ae24dfa321fe0f
Cr-Commit-Position: refs/heads/master@{#404272}
==========

[commit-bot: I haz the power, 2016-07-08 00:29:42]

Patchset 9 (id:??) landed as
https://crrev.com/554c98ac020b61bd06fc498759ae24dfa321fe0f
Cr-Commit-Position: refs/heads/master@{#404272}