Fix for surfaceless implementation

Fix for surfaceless implementation

- The image format used wasn't supported with the memory buffer framework
- The glClear on the init path causes an INVALID_FRAMEBUFFER_OPERATION error
  when executed with no framebuffer, as in the surfaceless case

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

[achaulk, 2014-09-23 17:20:38]

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

[alexst (slow to review), 2014-09-23 17:48:22]

Let's not bundle spec change and usage together.

Please make a change with just the new token for the extension and then the
second one that uses it.

https://codereview.chromium.org/592933003/diff/1/gpu/GLES2/extensions/CHROMIU...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt (right):

https://codereview.chromium.org/592933003/diff/1/gpu/GLES2/extensions/CHROMIU...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:48: Accepted by the
<internalformat> parameter of CreateImageCHROMIUM:
please add a new line after CreateImageCHROMIUM

[alexst (slow to review), 2014-09-24 13:00:51]

https://codereview.chromium.org/592933003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/592933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2694:
glCheckFramebufferStatusEXT(GL_FRAMEBUFFER) == GL_FRAMEBUFFER_COMPLETE) {
When does this happen? I'm not really familiar with this code path, is this
always supposed to have a framebuffer? Even in the cases where there is a
surface?

[achaulk, 2014-09-24 14:16:18]

https://codereview.chromium.org/592933003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/592933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2694:
glCheckFramebufferStatusEXT(GL_FRAMEBUFFER) == GL_FRAMEBUFFER_COMPLETE) {
On 2014/09/24 13:00:51, alexst wrote:
> When does this happen? I'm not really familiar with this code path, is this
> always supposed to have a framebuffer? Even in the cases where there is a
> surface?

This is done during initialization, before any regular BindFramebuffer calls
occur

[achaulk, 2014-09-24 15:23:57]

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

[piman, 2014-09-24 18:28:18]

https://codereview.chromium.org/592933003/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/592933003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2694:
glCheckFramebufferStatusEXT(GL_FRAMEBUFFER) == GL_FRAMEBUFFER_COMPLETE) {
So there's more to it than the clear, higher up we're asking the
alpha/depth/stencil bits (line 2621).
I'm not a big fan of asking GL if it thinks the framebuffer is complete. We know
whether we're surfaceless or not, we should pass the information through to here
(and that way we can also fail the operations that touch the framebuffer).

[achaulk, 2014-09-24 20:00:04]

https://codereview.chromium.org/592933003/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/592933003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2694:
glCheckFramebufferStatusEXT(GL_FRAMEBUFFER) == GL_FRAMEBUFFER_COMPLETE) {
On 2014/09/24 18:28:18, piman (Very slow to review) wrote:
> So there's more to it than the clear, higher up we're asking the
> alpha/depth/stencil bits (line 2621).
> I'm not a big fan of asking GL if it thinks the framebuffer is complete. We
know
> whether we're surfaceless or not, we should pass the information through to
here
> (and that way we can also fail the operations that touch the framebuffer).

The clear is the only operation that requires a complete framebuffer. However if
we need to have a stencil buffer setup earlier to pull out info, I'm not sure
how well this will work. We could try to bind the framebuffer early in
BindToClient, but I'm not sure if that's early enough to affect this

[piman, 2014-09-24 21:12:24]

On Wed, Sep 24, 2014 at 1:00 PM, <achaulk@chromium.org> wrote:

>
> https://codereview.chromium.org/592933003/diff/40001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.org/592933003/diff/40001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode2694
> gpu/command_buffer/service/gles2_cmd_decoder.cc:2694:
> glCheckFramebufferStatusEXT(GL_FRAMEBUFFER) == GL_FRAMEBUFFER_COMPLETE)
> {
> On 2014/09/24 18:28:18, piman (Very slow to review) wrote:
>
>> So there's more to it than the clear, higher up we're asking the
>> alpha/depth/stencil bits (line 2621).
>> I'm not a big fan of asking GL if it thinks the framebuffer is
>>
> complete. We know
>
>> whether we're surfaceless or not, we should pass the information
>>
> through to here
>
>> (and that way we can also fail the operations that touch the
>>
> framebuffer).
>
> The clear is the only operation that requires a complete framebuffer.
>

So would draws, copies, etc. Noted, those don't happen at initialization
time, but they can happen later if the client (possibly maliciously) tries
to execute those with FBO 0 bound.
The common pattern in GPU code is to avoid sending invalid state to the
driver: these are very untested paths in the drivers and a typical area for
crashes or worse.

It would be consistent to do the same for surfaceless, and treat FBO 0
bound as an incomplete FBO at the decoder level (that would be handled by
the CheckBoundFramebuffersValid call in DoDrawArrays etc.).

However if we need to have a stencil buffer setup earlier to pull out
> info, I'm not sure how well this will work. We could try to bind the
> framebuffer early in BindToClient, but I'm not sure if that's early
> enough to affect this
>

It's not really about that. There is no default stencil buffer in the
surfaceless case. It the client needs a stencil buffer, it has to create it
on its FBO. The code in lines 2621 and on is about knowing the properties
of the default framebuffer, and check if they differ from what the client
requested, in which case we need to handle some things specially (e.g.
extra processing on ReadPixels to avoid leaking uninitialized data).
glGetIntegerv(GL_ALPHA_BITS) (or stencil or depth or anything) is undefined
on an incomplete framebuffer per spec, so we should avoid doing it in the
surfaceless case.


> https://codereview.chromium.org/592933003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[achaulk, 2014-09-25 21:05:10]

On 2014/09/24 21:12:24, piman (Very slow to review) wrote:
> On Wed, Sep 24, 2014 at 1:00 PM, <mailto:achaulk@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/592933003/diff/40001/gpu/
> > command_buffer/service/gles2_cmd_decoder.cc
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> >
> > https://codereview.chromium.org/592933003/diff/40001/gpu/
> > command_buffer/service/gles2_cmd_decoder.cc#newcode2694
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:2694:
> > glCheckFramebufferStatusEXT(GL_FRAMEBUFFER) == GL_FRAMEBUFFER_COMPLETE)
> > {
> > On 2014/09/24 18:28:18, piman (Very slow to review) wrote:
> >
> >> So there's more to it than the clear, higher up we're asking the
> >> alpha/depth/stencil bits (line 2621).
> >> I'm not a big fan of asking GL if it thinks the framebuffer is
> >>
> > complete. We know
> >
> >> whether we're surfaceless or not, we should pass the information
> >>
> > through to here
> >
> >> (and that way we can also fail the operations that touch the
> >>
> > framebuffer).
> >
> > The clear is the only operation that requires a complete framebuffer.
> >
> 
> So would draws, copies, etc. Noted, those don't happen at initialization
> time, but they can happen later if the client (possibly maliciously) tries
> to execute those with FBO 0 bound.
> The common pattern in GPU code is to avoid sending invalid state to the
> driver: these are very untested paths in the drivers and a typical area for
> crashes or worse.
> 
> It would be consistent to do the same for surfaceless, and treat FBO 0
> bound as an incomplete FBO at the decoder level (that would be handled by
> the CheckBoundFramebuffersValid call in DoDrawArrays etc.).
> 
> However if we need to have a stencil buffer setup earlier to pull out
> > info, I'm not sure how well this will work. We could try to bind the
> > framebuffer early in BindToClient, but I'm not sure if that's early
> > enough to affect this
> >
> 
> It's not really about that. There is no default stencil buffer in the
> surfaceless case. It the client needs a stencil buffer, it has to create it
> on its FBO. The code in lines 2621 and on is about knowing the properties
> of the default framebuffer, and check if they differ from what the client
> requested, in which case we need to handle some things specially (e.g.
> extra processing on ReadPixels to avoid leaking uninitialized data).
> glGetIntegerv(GL_ALPHA_BITS) (or stencil or depth or anything) is undefined
> on an incomplete framebuffer per spec, so we should avoid doing it in the
> surfaceless case.
> 
> 
> > https://codereview.chromium.org/592933003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Something like this then?

[piman, 2014-09-25 21:42:50]

Yes, this is fine for now... I think you have a stale change in this CL though.

Doesn't have to be this CL, but I think we should in the future have the
surfaceless vs not decision at only one place (probably in the browser in
GpuProcessTransportFactory::CreateOutputSurface), and make sure that information
flows to the other place (ozone's GLSurface::CreateViewGLSurface), e.g. by
passing a context creation attribute. Right now there's an implicit knowledge
encoded in 2 places (surfaceless == ozone AND
SurfaceFactoryOzone::CanShowPrimaryPlaneAsOverlay() ) which makes it fragile
(actually, the 2 tests are not even identical).

https://codereview.chromium.org/592933003/diff/60001/base/debug/trace_event_i...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/592933003/diff/60001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:88: "host,-blink,-browser,-net,-loader";
?

[achaulk, 2014-09-26 15:07:37]

> Yes, this is fine for now... I think you have a stale change in this CL
though.
> 
> Doesn't have to be this CL, but I think we should in the future have the
> surfaceless vs not decision at only one place (probably in the browser in
> GpuProcessTransportFactory::CreateOutputSurface), and make sure that
information
> flows to the other place (ozone's GLSurface::CreateViewGLSurface), e.g. by
> passing a context creation attribute. Right now there's an implicit knowledge
> encoded in 2 places (surfaceless == ozone AND
> SurfaceFactoryOzone::CanShowPrimaryPlaneAsOverlay() ) which makes it fragile
> (actually, the 2 tests are not even identical).

Well there didn't seem to be a clean way to have the browser side test be
ozone-agnostic. Ideally it would just be one place deciding things, yes

> 
>
https://codereview.chromium.org/592933003/diff/60001/base/debug/trace_event_i...
> File base/debug/trace_event_impl.cc (right):
> 
>
https://codereview.chromium.org/592933003/diff/60001/base/debug/trace_event_i...
> base/debug/trace_event_impl.cc:88: "host,-blink,-browser,-net,-loader";
> ?

Oh right, forgot that upload squashes all CLs. Debug code

[piman, 2014-09-26 18:33:24]

lgtm

[achaulk, 2014-09-26 18:34:11]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-09-26 18:34:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/592933003/80001

[piman, 2014-09-26 18:34:42]

On Fri, Sep 26, 2014 at 8:07 AM, <achaulk@chromium.org> wrote:

> Yes, this is fine for now... I think you have a stale change in this CL
>>
> though.
>
>  Doesn't have to be this CL, but I think we should in the future have the
>> surfaceless vs not decision at only one place (probably in the browser in
>> GpuProcessTransportFactory::CreateOutputSurface), and make sure that
>>
> information
>
>> flows to the other place (ozone's GLSurface::CreateViewGLSurface), e.g.
>> by
>> passing a context creation attribute. Right now there's an implicit
>> knowledge
>> encoded in 2 places (surfaceless == ozone AND
>> SurfaceFactoryOzone::CanShowPrimaryPlaneAsOverlay() ) which makes it
>> fragile
>> (actually, the 2 tests are not even identical).
>>
>
> Well there didn't seem to be a clean way to have the browser side test be
> ozone-agnostic. Ideally it would just be one place deciding things, yes


We can have the browser decide and tell the GPU process what to do (via
context creation attributes).


>
>
>
>
> https://codereview.chromium.org/592933003/diff/60001/base/
> debug/trace_event_impl.cc
>
>> File base/debug/trace_event_impl.cc (right):
>>
>
>
> https://codereview.chromium.org/592933003/diff/60001/base/
> debug/trace_event_impl.cc#newcode88
>
>> base/debug/trace_event_impl.cc:88: "host,-blink,-browser,-net,-loader";
>> ?
>>
>
> Oh right, forgot that upload squashes all CLs. Debug code
>
> https://codereview.chromium.org/592933003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-09-26 19:28:50]

Committed patchset #5 (id:80001) as 65e7cc5940b532cef2498dbf14b14031985a9816

[commit-bot: I haz the power, 2014-09-26 19:29:52]

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