Surfaceless OutputSurface implementation.

Surfaceless OutputSurface implementation.

Implements an OutputSurface that uses render-to-texture and overlays
to display the main surface. Buffering of frames is implemented
internally as there is no system-provided surface.

BUG=380861
Committed: https://crrev.com/d438d276746f08c88ee7c69450ff1c2f41a46197
Cr-Commit-Position: refs/heads/master@{#295119}

[achaulk, 2014-08-29 23:35:50]

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

[achaulk, 2014-08-29 23:36:06]

still need to clean up the tests but this is basically the patch

[alexst (slow to review), 2014-09-02 13:16:00]

Some initial comments.

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:62: int
z_order)
I see primary surface as always being 0, others can allocate around it with
negative indices.

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:69:
expects_pageflip_messages_(false),
Please provide a way to change this.

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:82: const
gfx::Rect& rect = frame->gl_frame_data->sub_buffer_rect;
sub_buffer_rect provides a dirty region, if I am not mistaken. It will only be
smaller than the whole frame size if partial swap is enabled. This should use
size, and the rect will be used when we implement partial updates.

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:146:
gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
Please split all the GL code from the constructor into an init function.

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:235:
unsigned int id = context_provider_->ContextGL()->CreateImageCHROMIUM(
Allocating these involves a sync IPC, and I would also like to reduce the churn
of new buffers, so We should re-use as much as possible, and should only be
deleting these buffers when we are reshaping and during teardown.

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.h (right):

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.h:49: class
CONTENT_EXPORT BufferedOutputSurface {
Let's move this into a separate file.

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.h:82: class
GpuSurfacelessBrowserCompositorOutputSurface
Let's put this in a separate file too.

[achaulk, 2014-09-02 17:48:45]

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:62: int
z_order)
On 2014/09/02 13:15:59, alexst wrote:
> I see primary surface as always being 0, others can allocate around it with
> negative indices.

I just made this configurable so that if someone did want to do a multiple
compositor thing they can set up the levels the way they want

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:69:
expects_pageflip_messages_(false),
On 2014/09/02 13:15:59, alexst wrote:
> Please provide a way to change this.

That will be when the IPC gets integrated

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:82: const
gfx::Rect& rect = frame->gl_frame_data->sub_buffer_rect;
On 2014/09/02 13:15:59, alexst wrote:
> sub_buffer_rect provides a dirty region, if I am not mistaken. It will only be
> smaller than the whole frame size if partial swap is enabled. This should use
> size, and the rect will be used when we implement partial updates.

Sure. I'm not sure how we can really do partial updates anyway. Maybe only every
other frame and keep old buffers around?

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:146:
gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
On 2014/09/02 13:15:59, alexst wrote:
> Please split all the GL code from the constructor into an init function.

Done.

https://codereview.chromium.org/516663003/diff/100001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:235:
unsigned int id = context_provider_->ContextGL()->CreateImageCHROMIUM(
On 2014/09/02 13:15:59, alexst wrote:
> Allocating these involves a sync IPC, and I would also like to reduce the
churn
> of new buffers, so We should re-use as much as possible, and should only be
> deleting these buffers when we are reshaping and during teardown.

I can make it keep the third buffer around instead of reducing to two ASAP, but
I don't think we want to keep the extra buffers in the degenerate N-buffer case
where page flip messages are delayed. We might not have the memory for a half
dozen framebuffers anyway

[alexst (slow to review), 2014-09-02 18:51:28]

> Sure. I'm not sure how we can really do partial updates anyway. Maybe only
every
> other frame and keep old buffers around?

Right, you keep the previous frame around and copy from it. This has been done
in Chrome a bunch of times, then deleted, then resurrected. You don't have to
re-implement it from scratch. There is a good chance this code still exists
somewhere in the tree, but the best way to find it is to look through the
history of this function, it should have all the damage tracking and GL code.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

But let's leave partial swap to a followup, though. It's an optimization,
swapping whole frames is fine for now.

> I can make it keep the third buffer around instead of reducing to two ASAP,
but
> I don't think we want to keep the extra buffers in the degenerate N-buffer
case
> where page flip messages are delayed. We might not have the memory for a half
> dozen framebuffers anyway

It shouldn't get into that state, scheduler paces itself with swap buffer acks.
Right now acks may be arriving too soon because we assume that SwapBuffers
blocks in the gpu, and as soon as there is an echo from the GPU process,
scheduler assumes swap buffers finished. We need to rely on the page flip IPC to
inform it instead.

Let's keep a buffer queue and reduce allocs/de-allocs. I think in practice it'll
probably mean 3 buffers for now, but later we can co-ordinate with the scheduler
and on the gpu side to get back to 2. We can either make the scheduler only
produce 2 buffers and we can also de-schedule the channel in the gpu process to
emulate the surfaced swap buffers behavior.

[achaulk, 2014-09-10 15:11:27]

On 2014/09/02 18:51:28, alexst wrote:
> > Sure. I'm not sure how we can really do partial updates anyway. Maybe only
> every
> > other frame and keep old buffers around?
> 
> Right, you keep the previous frame around and copy from it. This has been done
> in Chrome a bunch of times, then deleted, then resurrected. You don't have to
> re-implement it from scratch. There is a good chance this code still exists
> somewhere in the tree, but the best way to find it is to look through the
> history of this function, it should have all the damage tracking and GL code.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> But let's leave partial swap to a followup, though. It's an optimization,
> swapping whole frames is fine for now.
> 
> > I can make it keep the third buffer around instead of reducing to two ASAP,
> but
> > I don't think we want to keep the extra buffers in the degenerate N-buffer
> case
> > where page flip messages are delayed. We might not have the memory for a
half
> > dozen framebuffers anyway
> 
> It shouldn't get into that state, scheduler paces itself with swap buffer
acks.
> Right now acks may be arriving too soon because we assume that SwapBuffers
> blocks in the gpu, and as soon as there is an echo from the GPU process,
> scheduler assumes swap buffers finished. We need to rely on the page flip IPC
to
> inform it instead.
> 
> Let's keep a buffer queue and reduce allocs/de-allocs. I think in practice
it'll
> probably mean 3 buffers for now, but later we can co-ordinate with the
scheduler
> and on the gpu side to get back to 2. We can either make the scheduler only
> produce 2 buffers and we can also de-schedule the channel in the gpu process
to
> emulate the surfaced swap buffers behavior.

Sure, though in practice only one of them gets removed and we just keep 3
buffers around if we ever triple buffer

[alexst (slow to review), 2014-09-10 15:39:08]

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:90: if
(available_surfaces_.size() < 2)
Let's not try to limit things to 2 buffers here, we should let scheduler deal
with it, if we expect multiple pending buffers, we should have a queue for them,
as put them back to available buffers as ACK's arrive.

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:103:
context_provider_->ContextGL()->DestroyImageCHROMIUM(
You are calling FreeSurface above, but Destroy directly here. Let's make it
consistent. Also, we bound these images to texture id's, we should release them
with releaseTexImage2DCHROMIUM before destroying the images.

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:122: unsigned int id =
context_provider_->ContextGL()->CreateImageCHROMIUM(
Maybe we can associate an image with a unique texture id here. That would make
releaseTexImage2DCHROMIUM above more clear and would probably make debugging a
bit easier since we'll see different tex_id's scheduled instead of the same one
with an image changing from underneath it.

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:9: #include
"content/browser/gpu/gpu_surface_tracker.h"
If this is used in the surfaceless file, let's move it there.

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.h (right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.h:10: #include
"cc/output/context_provider.h"
Please move it into the cc file.

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:47:
gl->Finish();
Put a comment here to indicate this glFinish is temporary and will be replaced
with a fence.

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:50:
output_surface_->tex_id(),
Let's rename this to something more descriptive, maybe last_swapped_texture()?

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h
(right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h:24:
int z_order);
Let's leave z order for a followup CL.

[achaulk, 2014-09-10 16:47:55]

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:90: if
(available_surfaces_.size() < 2)
On 2014/09/10 15:39:07, alexst wrote:
> Let's not try to limit things to 2 buffers here, we should let scheduler deal
> with it, if we expect multiple pending buffers, we should have a queue for
them,
> as put them back to available buffers as ACK's arrive.

Actually thinking about it the check is redundant anyway

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:103:
context_provider_->ContextGL()->DestroyImageCHROMIUM(
On 2014/09/10 15:39:07, alexst wrote:
> You are calling FreeSurface above, but Destroy directly here. Let's make it
> consistent. Also, we bound these images to texture id's, we should release
them
> with releaseTexImage2DCHROMIUM before destroying the images.

The free function is just a shortcut for release-and-zero. I could change the
name

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:122: unsigned int id =
context_provider_->ContextGL()->CreateImageCHROMIUM(
On 2014/09/10 15:39:07, alexst wrote:
> Maybe we can associate an image with a unique texture id here. That would make
> releaseTexImage2DCHROMIUM above more clear and would probably make debugging a
> bit easier since we'll see different tex_id's scheduled instead of the same
one
> with an image changing from underneath it.

We could, but then we would just have more things to track. The tex id is only
used for a short while anyway before it gets translated into a GLImage

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:9: #include
"content/browser/gpu/gpu_surface_tracker.h"
On 2014/09/10 15:39:07, alexst wrote:
> If this is used in the surfaceless file, let's move it there.

Leftover from older stuff

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.h (right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.h:10: #include
"cc/output/context_provider.h"
On 2014/09/10 15:39:07, alexst wrote:
> Please move it into the cc file.

Leftover from the combined file

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/140001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:50:
output_surface_->tex_id(),
On 2014/09/10 15:39:07, alexst wrote:
> Let's rename this to something more descriptive, maybe last_swapped_texture()?


Well the texture never changes, we just rebind new buffers to the same id

[alexst (slow to review), 2014-09-10 17:37:09]

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:56:
gl->BindTexImage2DCHROMIUM(GL_TEXTURE_2D, current_surface_);
I looked at the implementation, and this has a non-trivial cost. It reshuffles a
bunch of internal stuff and calls glEGLImageTargetTexture2DOES. I would prefer
we have a one to one mapping of texture_id to image_id to avoid calling into egl
unnecessarily. I don't we it makes tracking that much more complicated, you just
keep a pair instead of an int.

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:63: if
(in_flight_surface_) {
Let's move this to bind.

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:67: DCHECK(false);
LOG(FATAL) << why we crashed

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:66:
if (!expects_pageflip_messages_)
Page flip message is what should trigger the OnSwapBuffersComplete, so I don't
think we need this variable.

We should modify
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

to understand surfaceless and not just send an echo, but to send an actual swap
complete message.

[achaulk, 2014-09-10 18:56:08]

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:56:
gl->BindTexImage2DCHROMIUM(GL_TEXTURE_2D, current_surface_);
On 2014/09/10 17:37:08, alexst wrote:
> I looked at the implementation, and this has a non-trivial cost. It reshuffles
a
> bunch of internal stuff and calls glEGLImageTargetTexture2DOES. I would prefer
> we have a one to one mapping of texture_id to image_id to avoid calling into
egl
> unnecessarily. I don't we it makes tracking that much more complicated, you
just
> keep a pair instead of an int.

Done.

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:63: if
(in_flight_surface_) {
On 2014/09/10 17:37:08, alexst wrote:
> Let's move this to bind.

We can't. Multiply calling bind is probably allowed, so we can't just advance on
every call, we need something else to push surfaces along the path

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/160001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:66:
if (!expects_pageflip_messages_)
On 2014/09/10 17:37:09, alexst wrote:
> Page flip message is what should trigger the OnSwapBuffersComplete, so I don't
> think we need this variable.
> 
> We should modify
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
> 
> to understand surfaceless and not just send an echo, but to send an actual
swap
> complete message.

Done.

[achaulk, 2014-09-10 19:03:52]

Do we want to preallocate the first two buffers when we resize?

[alexst (slow to review), 2014-09-10 21:25:14]

I don't think we need to pre-allocate. Looks good, let me just look through the
tests and we should expand the reviewer list.

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:103: gl->DeleteTextures(1,
&surface->first);
Use ReleaseTexImage2DCHROMIUM before deleting the texture please

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:123:
DCHECK(allocated_count_ < 4);
There is a DCHECK_LT, please use that instead.

[alexst (slow to review), 2014-09-11 15:41:33]

after the nits: lgtm! thank you!

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:31:
z_order_(z_order),
Let's save the z_order for when we need it.

[achaulk, 2014-09-11 17:25:48]

Who should do owners review? piman?

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:103: gl->DeleteTextures(1,
&surface->first);
On 2014/09/10 21:25:14, alexst wrote:
> Use ReleaseTexImage2DCHROMIUM before deleting the texture please

Done.

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:123:
DCHECK(allocated_count_ < 4);
On 2014/09/10 21:25:14, alexst wrote:
> There is a DCHECK_LT, please use that instead.

Done.

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/180001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:31:
z_order_(z_order),
On 2014/09/11 15:41:32, alexst wrote:
> Let's save the z_order for when we need it.

Done.

[achaulk, 2014-09-11 17:37:13]

achaulk@chromium.org changed reviewers:
+ piman@chromium.org

[achaulk, 2014-09-11 17:37:15]

+piman for owners review

[piman, 2014-09-11 20:25:29]

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:31:
gl->DeleteRenderbuffers(1, &depth_rb_);
We don't need a depth buffer for the compositor.

We will need a stencil buffer when we do GPU rasterization in the browser
process, but we don't do that yet (worth a TODO). We'd only want to allocate the
stencil buffer when that is enabled.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:39: DCHECK(depth_rb_);
Both these DCHECKs can fail in case of a lost context.

Initialization should be allowed to fail (resulting in
GpuSurfacelessBrowserCompositorOutputSurface::BindToClient returning false)

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:51:
current_surface_.second,
I'm confused. In BufferedOutputSurface::GetNextSurface, the texture id is
returned as the first member of the pair, the image id is returned as the second
member. It looks like here you're using the image id instead of the texture id.

See also comment in the .h

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:58:
in_flight_surfaces_.push(last_surface_);
in_flight_surfaces_ is confusing. It contains all in-flight surfaces except the
earliest one.
How about:
- always pushing the current surface into in_flight_surfaces_ (i.e. you don't
need last_surface_ any more)
- pushing an empty surface at the beginning, to ensure we don't recycle the
first one too early.
- in_flight_surfaces_ can never be empty (since you have at most as many
PageFlipComplete as SwapBuffers)

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:68: FreeAllSurfaces();
Is it really ok to delete the surface that is currently being displayed (the
last one you swapped)?

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:69: 
Can you add a DCHECK that current_surface_ is empty?
I think that's true today - Reshape is called before BindFramebuffer after a
swap, but if that changed to not be true, then you'd need to reallocate now and
update the FBO attachment.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:71: return;  // Nothing to
do.
You probably want to at least unbind the current attachments, so that you can
free the memory.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:121: unsigned int tex;
nit: texture

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.h (right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.h:25: class CONTENT_EXPORT
BufferedOutputSurface {
nit: the name BufferedOutputSurface feels like it implements the
cc::OutputSurface interface, but it doesn't. Could we use a name that doesn't
suggest it? BufferQueue? SwapChain? Something like this?

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.h:38: unsigned int
current_tex_id() { return current_surface_.first; }
nit: no abbreviation. current_texture_id() ?

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.h:42: typedef
std::pair<unsigned int, unsigned int> AllocatedSurface;
nit: can you make this a struct? It helps readability a lot. "first" and
"second" are poor names at the call sites.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.h:58: last_surface_;  // This
surface has been sent to the hardware.
nit: keep member identifier and type on the same line, move the comment on the
line before.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:43:
gl->Finish();  // This will eventually be replaced by sync fences.
I don't think the client should do this. This implies both a GL-level glFinish
in the GPU process, as well as a command-buffer finish, which blocks the client
until all the GL commands have executed.

I think you added this as a workaround for some drivers that need GL-level
synchronization. But that should be handled in the GPU process, in
GLSurfaceOzoneSurfaceless::SwapBuffers (or the ozone bits below).

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:80:
DCHECK(output_surface_);
This DCHECK looks wrong. The only place you're assigning output_surface_ is this
method.

BindToClient should be called only once. So I think what you want is:
DCHECK(!output_surface_)

and below, remove the if (!output_surface_)

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h
(right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h:35:
scoped_ptr<BufferedOutputSurface> output_surface_;
nit: DISALLOW_COPY_AND_ASSIGN

[achaulk, 2014-09-12 19:51:08]

I'll rename the file/class in another patch

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:31:
gl->DeleteRenderbuffers(1, &depth_rb_);
On 2014/09/11 20:25:29, piman (Very slow to review) wrote:
> We don't need a depth buffer for the compositor.
> 
> We will need a stencil buffer when we do GPU rasterization in the browser
> process, but we don't do that yet (worth a TODO). We'd only want to allocate
the
> stencil buffer when that is enabled.

Done.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:39: DCHECK(depth_rb_);
On 2014/09/11 20:25:28, piman (Very slow to review) wrote:
> Both these DCHECKs can fail in case of a lost context.
> 
> Initialization should be allowed to fail (resulting in
> GpuSurfacelessBrowserCompositorOutputSurface::BindToClient returning false)

Done.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:51:
current_surface_.second,
On 2014/09/11 20:25:28, piman (Very slow to review) wrote:
> I'm confused. In BufferedOutputSurface::GetNextSurface, the texture id is
> returned as the first member of the pair, the image id is returned as the
second
> member. It looks like here you're using the image id instead of the texture
id.
> 
> See also comment in the .h

Oh yes this should be first instead

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:58:
in_flight_surfaces_.push(last_surface_);
On 2014/09/11 20:25:28, piman (Very slow to review) wrote:
> in_flight_surfaces_ is confusing. It contains all in-flight surfaces except
the
> earliest one.
> How about:
> - always pushing the current surface into in_flight_surfaces_ (i.e. you don't
> need last_surface_ any more)
> - pushing an empty surface at the beginning, to ensure we don't recycle the
> first one too early.
> - in_flight_surfaces_ can never be empty (since you have at most as many
> PageFlipComplete as SwapBuffers)

I don't really like having a dummy surface, but I can merge last into here.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:68: FreeAllSurfaces();
On 2014/09/11 20:25:28, piman (Very slow to review) wrote:
> Is it really ok to delete the surface that is currently being displayed (the
> last one you swapped)?

Yes. Only the GL-side structures will get torn down, the actual buffers are kept
around separately

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:69: 
On 2014/09/11 20:25:28, piman (Very slow to review) wrote:
> Can you add a DCHECK that current_surface_ is empty?
> I think that's true today - Reshape is called before BindFramebuffer after a
> swap, but if that changed to not be true, then you'd need to reallocate now
and
> update the FBO attachment.

Done.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:71: return;  // Nothing to
do.
On 2014/09/11 20:25:29, piman (Very slow to review) wrote:
> You probably want to at least unbind the current attachments, so that you can
> free the memory.

Done.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:121: unsigned int tex;
On 2014/09/11 20:25:28, piman (Very slow to review) wrote:
> nit: texture

Done.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.h (right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.h:38: unsigned int
current_tex_id() { return current_surface_.first; }
On 2014/09/11 20:25:29, piman (Very slow to review) wrote:
> nit: no abbreviation. current_texture_id() ?

Done.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.h:42: typedef
std::pair<unsigned int, unsigned int> AllocatedSurface;
On 2014/09/11 20:25:29, piman (Very slow to review) wrote:
> nit: can you make this a struct? It helps readability a lot. "first" and
> "second" are poor names at the call sites.

Done.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h
(right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h:35:
scoped_ptr<BufferedOutputSurface> output_surface_;
On 2014/09/11 20:25:29, piman (Very slow to review) wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

[piman, 2014-09-12 21:58:30]

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:43:
gl->Finish();  // This will eventually be replaced by sync fences.
On 2014/09/11 20:25:29, piman (Very slow to review) wrote:
> I don't think the client should do this. This implies both a GL-level glFinish
> in the GPU process, as well as a command-buffer finish, which blocks the
client
> until all the GL commands have executed.
> 
> I think you added this as a workaround for some drivers that need GL-level
> synchronization. But that should be handled in the GPU process, in
> GLSurfaceOzoneSurfaceless::SwapBuffers (or the ozone bits below).

ping?

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:110: unsigned int texture;
nit: =0

If the context is lost, GenTextures may not initialize texture.

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:122: DCHECK(id);
nit: if the context is lost, id will be 0.

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
File content/browser/compositor/buffered_output_surface_unittest.cc (right):

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
content/browser/compositor/buffered_output_surface_unittest.cc:68: if
(set->find(value) != set->end())
nit: std::set::insert returns a (iterator, bool) pair that tells you if the
insert did actually insert or not (because of existing one). I.e. you can simply
return set->insert(value).second.

[achaulk, 2014-09-15 16:31:43]

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.h (right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/buffered_output_surface.h:25: class CONTENT_EXPORT
BufferedOutputSurface {
On 2014/09/11 20:25:29, piman (Very slow to review) wrote:
> nit: the name BufferedOutputSurface feels like it implements the
> cc::OutputSurface interface, but it doesn't. Could we use a name that doesn't
> suggest it? BufferQueue? SwapChain? Something like this?

Done.

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/516663003/diff/220001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:43:
gl->Finish();  // This will eventually be replaced by sync fences.
On 2014/09/12 21:58:29, piman (Very slow to review) wrote:
> On 2014/09/11 20:25:29, piman (Very slow to review) wrote:
> > I don't think the client should do this. This implies both a GL-level
glFinish
> > in the GPU process, as well as a command-buffer finish, which blocks the
> client
> > until all the GL commands have executed.
> > 
> > I think you added this as a workaround for some drivers that need GL-level
> > synchronization. But that should be handled in the GPU process, in
> > GLSurfaceOzoneSurfaceless::SwapBuffers (or the ozone bits below).
> 
> ping?

Done.

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:110: unsigned int texture;
On 2014/09/12 21:58:29, piman (Very slow to review) wrote:
> nit: =0
> 
> If the context is lost, GenTextures may not initialize texture.

Done.

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:122: DCHECK(id);
On 2014/09/12 21:58:30, piman (Very slow to review) wrote:
> nit: if the context is lost, id will be 0.

Done.

[piman, 2014-09-15 18:29:58]

LGTM + 2 nits

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:122: DCHECK(id);
On 2014/09/15 16:31:42, achaulk wrote:
> On 2014/09/12 21:58:30, piman (Very slow to review) wrote:
> > nit: if the context is lost, id will be 0.
> 
> Done.

I still see the DCHECK in the new code.

Is the DCHECK important? If the context is lost, all the later calls on |gl|
will essentially noop. Eventually the lost context callback will make the
compositor destroy and recreate the output surface (hence the BufferQueue). The
only place |id| is used is in gl calls, so it should be safe if it's 0.

So can we just remove the DCHECK?

https://codereview.chromium.org/516663003/diff/260001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/516663003/diff/260001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:125: glFinish();
nit: can you add a TODO/bug?

[achaulk, 2014-09-15 18:41:25]

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
File content/browser/compositor/buffered_output_surface.cc (right):

https://codereview.chromium.org/516663003/diff/240001/content/browser/composi...
content/browser/compositor/buffered_output_surface.cc:122: DCHECK(id);
On 2014/09/15 18:29:58, piman (Very slow to review) wrote:
> On 2014/09/15 16:31:42, achaulk wrote:
> > On 2014/09/12 21:58:30, piman (Very slow to review) wrote:
> > > nit: if the context is lost, id will be 0.
> > 
> > Done.
> 
> I still see the DCHECK in the new code.
> 
> Is the DCHECK important? If the context is lost, all the later calls on |gl|
> will essentially noop. Eventually the lost context callback will make the
> compositor destroy and recreate the output surface (hence the BufferQueue).
The
> only place |id| is used is in gl calls, so it should be safe if it's 0.
> 
> So can we just remove the DCHECK?

Yeah, forgot to save the file after removing that line

https://codereview.chromium.org/516663003/diff/260001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/516663003/diff/260001/ui/gl/gl_surface_ozone....
ui/gl/gl_surface_ozone.cc:125: glFinish();
On 2014/09/15 18:29:58, piman (Very slow to review) wrote:
> nit: can you add a TODO/bug?

Done.

[achaulk, 2014-09-15 19:12:04]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-09-15 19:16:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/516663003/280001

[commit-bot: I haz the power, 2014-09-15 20:31:36]

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

[commit-bot: I haz the power, 2014-09-15 20:31:38]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[achaulk, 2014-09-16 16:08:56]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-09-16 16:09:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/516663003/280001

[commit-bot: I haz the power, 2014-09-16 17:22:55]

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

[commit-bot: I haz the power, 2014-09-16 17:22:57]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[achaulk, 2014-09-16 17:50:07]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-09-16 17:50:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/516663003/300001

[commit-bot: I haz the power, 2014-09-16 19:14:05]

Committed patchset #16 (id:300001) as f60953f5383693c4816c32af308c0834b4e6303a

[commit-bot: I haz the power, 2014-09-16 19:16:00]

Patchset 16 (id:??) landed as
https://crrev.com/d438d276746f08c88ee7c69450ff1c2f41a46197
Cr-Commit-Position: refs/heads/master@{#295119}