[cc] Mailbox Output Surface Support

[cc] Mailbox Output Surface Support

This allows to render to textures managed by the compositor,
that are sent to the browser using GL_CHROMIUM_texture_mailbox.

Needs support in the browser and
'--composite-to-mailbox' and '--enable-compositor-frame-message'.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185812

[no sievers, 2013-02-27 23:00:47]

Renderer/cc-side changes extracted from
https://codereview.chromium.org/11861020/

[jamesr, 2013-02-27 23:02:43]

The OutputSurface changes in this patch are really cool but not really directly
related to mailboxing, are they?

[no sievers, 2013-02-27 23:04:45]

On 2013/02/27 23:02:43, jamesr wrote:
> The OutputSurface changes in this patch are really cool but not really
directly
> related to mailboxing, are they?

Are you suggesting I split this one further up into two patches? :)

[jamesr, 2013-02-27 23:06:37]

On 2013/02/27 23:04:45, Daniel Sievers wrote:
> On 2013/02/27 23:02:43, jamesr wrote:
> > The OutputSurface changes in this patch are really cool but not really
> directly
> > related to mailboxing, are they?
> 
> Are you suggesting I split this one further up into two patches? :)

I'm just trying to understand what you're trying to accomplish here.

[no sievers, 2013-02-27 23:10:51]

On 2013/02/27 23:06:37, jamesr wrote:
> On 2013/02/27 23:04:45, Daniel Sievers wrote:
> > On 2013/02/27 23:02:43, jamesr wrote:
> > > The OutputSurface changes in this patch are really cool but not really
> > directly
> > > related to mailboxing, are they?
> > 
> > Are you suggesting I split this one further up into two patches? :)
> 
> I'm just trying to understand what you're trying to accomplish here.

I split the patch from https://codereview.chromium.org/11861020/ into smaller
patches. These are the renderer-side changes for being able to render to mailbox
textures. The output surface refactor is basically to be able to factor the
mailbox-related code into mailbox_output_surface.cc.

I'll gladly split this into two patches though, it doesn't seem unreasonable :) 
The added unit tests ensure that gl_renderer does not call framebuffer GL apis
directly but goes through OutputSurface, so the refactor should be good on its
own.

[piman, 2013-02-27 23:51:04]

Mostly small stuff.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
File content/renderer/gpu/mailbox_output_surface.cc (right):

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:39: while
(returned_textures_.size()) {
!returned_textures_.empty()

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:77: while
(returned_textures_.size()) {
!returned_textures_.empty()

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:124:
context3d_->deleteTexture(current_backing_.texture_id);
note: for a follow-up probably, but you may want to keep the texture around, to
avoid having to recreate it (it's non-trivial).

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:126:
CompositorOutputSurface::SendFrameToParentCompositor(frame);
nit: can you move this above the deleteTexture? It does non trivial work (lock,
flush) in the possibly latency-sensitive path.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:141:
context3d_->waitSyncPoint(ack.gl_frame_data->sync_point);
note: a potential optimization (for a follow-up too) is to not wait/consume
until you reuse the texture, to avoid over-constraining the renderer.

https://codereview.chromium.org/12371002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12371002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2048:
cc::switches::kEnableCompositorFrameMessage));
Could we instead make sure settings.compositorFrameMessage is set when
kCompositeToMailbox) is set?

Maybe as a follow-up, we should look at removing the setting (the decision
should really be done by the OutputSurface).

[danakj, 2013-02-28 03:57:44]

driveby style nits

https://codereview.chromium.org/12371002/diff/1/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/12371002/diff/1/cc/output_surface.h#newcode81
cc/output_surface.h:81: virtual void Reshape(const gfx::Size& size);
nit: pass Size/Point/Rect/Vector2d/SizeF/PointF by value

https://codereview.chromium.org/12371002/diff/1/cc/output_surface.h#newcode85
cc/output_surface.h:85: virtual void PostSubBuffer(const gfx::Rect& rect);
nit: by value

https://codereview.chromium.org/12371002/diff/1/cc/output_surface.h#newcode97
cc/output_surface.h:97: bool  has_gl_discard_backbuffer_;
nit: extra space

[Sami, 2013-02-28 12:46:58]

https://codereview.chromium.org/12371002/diff/1/cc/switches.cc
File cc/switches.cc (right):

https://codereview.chromium.org/12371002/diff/1/cc/switches.cc#newcode94
cc/switches.cc:94: // through the texture mailbox mechanism. Requires
kEnableCompositorFrameMessage.
Nit: say --enable-compositor-frame-message instead of
kEnableCompositorFrameMessage.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
File content/renderer/gpu/mailbox_output_surface.cc (right):

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:90: size_ = size;
Should we DCHECK here that size_ != size? Hopefully we're not doing this
uselessly.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:100: fbo_ =
context3d_->createFramebuffer();
Nit: indent by 2.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
File content/renderer/gpu/mailbox_output_surface.h (right):

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.h:51: const gfx::Size& size)
Nit: pass by value.

[no sievers, 2013-02-28 18:43:40]

Thanks all!

https://codereview.chromium.org/12371002/diff/1/cc/output_surface.h
File cc/output_surface.h (right):

https://codereview.chromium.org/12371002/diff/1/cc/output_surface.h#newcode81
cc/output_surface.h:81: virtual void Reshape(const gfx::Size& size);
On 2013/02/28 03:57:44, danakj wrote:
> nit: pass Size/Point/Rect/Vector2d/SizeF/PointF by value

Done.

https://codereview.chromium.org/12371002/diff/1/cc/output_surface.h#newcode85
cc/output_surface.h:85: virtual void PostSubBuffer(const gfx::Rect& rect);
On 2013/02/28 03:57:44, danakj wrote:
> nit: by value

Done.

https://codereview.chromium.org/12371002/diff/1/cc/output_surface.h#newcode97
cc/output_surface.h:97: bool  has_gl_discard_backbuffer_;
On 2013/02/28 03:57:44, danakj wrote:
> nit: extra space

Done.

https://codereview.chromium.org/12371002/diff/1/cc/switches.cc
File cc/switches.cc (right):

https://codereview.chromium.org/12371002/diff/1/cc/switches.cc#newcode94
cc/switches.cc:94: // through the texture mailbox mechanism. Requires
kEnableCompositorFrameMessage.
On 2013/02/28 12:46:58, Sami wrote:
> Nit: say --enable-compositor-frame-message instead of
> kEnableCompositorFrameMessage.

Done.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
File content/renderer/gpu/mailbox_output_surface.cc (right):

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:39: while
(returned_textures_.size()) {
On 2013/02/27 23:51:04, piman wrote:
> !returned_textures_.empty()

Done.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:77: while
(returned_textures_.size()) {
On 2013/02/27 23:51:04, piman wrote:
> !returned_textures_.empty()

Done.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:90: size_ = size;
On 2013/02/28 12:46:58, Sami wrote:
> Should we DCHECK here that size_ != size? Hopefully we're not doing this
> uselessly.

I added an early-return.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:100: fbo_ =
context3d_->createFramebuffer();
On 2013/02/28 12:46:58, Sami wrote:
> Nit: indent by 2.

Done.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:124:
context3d_->deleteTexture(current_backing_.texture_id);
On 2013/02/27 23:51:04, piman wrote:
> note: for a follow-up probably, but you may want to keep the texture around,
to
> avoid having to recreate it (it's non-trivial).

TODO added.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:126:
CompositorOutputSurface::SendFrameToParentCompositor(frame);
On 2013/02/27 23:51:04, piman wrote:
> nit: can you move this above the deleteTexture? It does non trivial work
(lock,
> flush) in the possibly latency-sensitive path.

Done.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.cc:141:
context3d_->waitSyncPoint(ack.gl_frame_data->sync_point);
On 2013/02/27 23:51:04, piman wrote:
> note: a potential optimization (for a follow-up too) is to not wait/consume
> until you reuse the texture, to avoid over-constraining the renderer.

TODO added.

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
File content/renderer/gpu/mailbox_output_surface.h (right):

https://codereview.chromium.org/12371002/diff/1/content/renderer/gpu/mailbox_...
content/renderer/gpu/mailbox_output_surface.h:51: const gfx::Size& size)
On 2013/02/28 12:46:58, Sami wrote:
> Nit: pass by value.

Done.

https://codereview.chromium.org/12371002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12371002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2048:
cc::switches::kEnableCompositorFrameMessage));
On 2013/02/27 23:51:04, piman wrote:
> Could we instead make sure settings.compositorFrameMessage is set when
> kCompositeToMailbox) is set?
> 

I'd rather remove kEnableCompositorFrameMessage soon and force consistent
behavior in the meantime (there are unfortunately some places in
render_view_impl_android.cc that check the cmdline, and this functionality -
sending scroll offsets - will be used in conjunction with composite-to-mailbox).
Sound good?

> Maybe as a follow-up, we should look at removing the setting (the decision
> should really be done by the OutputSurface).

[piman, 2013-02-28 19:30:31]

lgtm

https://codereview.chromium.org/12371002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12371002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2048:
cc::switches::kEnableCompositorFrameMessage));
On 2013/02/28 18:43:40, Daniel Sievers wrote:
> On 2013/02/27 23:51:04, piman wrote:
> > Could we instead make sure settings.compositorFrameMessage is set when
> > kCompositeToMailbox) is set?
> > 
> 
> I'd rather remove kEnableCompositorFrameMessage soon and force consistent
> behavior in the meantime (there are unfortunately some places in
> render_view_impl_android.cc that check the cmdline, and this functionality -
> sending scroll offsets - will be used in conjunction with
composite-to-mailbox).
> Sound good?
> 
> > Maybe as a follow-up, we should look at removing the setting (the decision
> > should really be done by the OutputSurface).
> 

As long as we come back and clean this soon, ok.

[Sami, 2013-03-01 10:56:40]

lgtm

[commit-bot: I haz the power, 2013-03-01 21:53:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/12371002/14003

[commit-bot: I haz the power, 2013-03-02 16:11:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/12371002/14003

[commit-bot: I haz the power, 2013-03-02 19:11:50]

Failed to apply patch for
content/browser/renderer_host/render_process_host_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/render_process_host_impl.cc
  Hunk #1 FAILED at 871.
  1 out of 1 hunk FAILED -- saving rejects to file
content/browser/renderer_host/render_process_host_impl.cc.rej

Patch:       content/browser/renderer_host/render_process_host_impl.cc
Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc
b/content/browser/renderer_host/render_process_host_impl.cc
index
2e4757d3aacab80dd073ea716ec72847f0494a84..82ccb6a2545d9a0b00ffa0baf508216942665ec2
100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -871,6 +871,7 @@ void
RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
     cc::switches::kSlowDownRasterScaleFactor,
     cc::switches::kUseCheapnessEstimator,
     cc::switches::kLowResolutionContentsScaleFactor,
+    cc::switches::kCompositeToMailbox,
   };
   renderer_cmd->CopySwitchesFrom(browser_cmd, kSwitchNames,
                                  arraysize(kSwitchNames));

[commit-bot: I haz the power, 2013-03-03 18:32:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/12371002/14005

[commit-bot: I haz the power, 2013-03-03 21:58:18]

Change committed as 185812