gpu: Disallow zero sized offscreen framebuffers

gpu: Disallow zero sized offscreen framebuffers

Modify initial the offscreen size if empty
DCHECK to assert that we don't run into this situation again

BUG=481035
Committed: https://crrev.com/2e3b5c8ccaf50206788f433f74af67a4e153230a
Cr-Commit-Position: refs/heads/master@{#327180}

[hendrikw, 2015-04-24 18:33:38]

hendrikw@chromium.org changed reviewers:
+ jbauman@chromium.org, piman@chromium.org

[hendrikw, 2015-04-24 18:33:39]

PTAL, Thanks!

https://codereview.chromium.org/1106773002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1106773002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:206: if (is_offscreen &&
initial_size_.IsEmpty()) {
I might rename initial_size_ to initial_offscreen_size_ later, it's a bit
misleading as it.

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

https://codereview.chromium.org/1106773002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2467: DCHECK(!offscreen ||
!offscreen_size.IsEmpty());
In my first change, I also DCHECK'd that glGetError() == GL_NO_ERROR near the
end of the function, but we have some mocks that didn't agree with that.

I think it makes sense to catch this, but maybe I should be calling
LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER, then DCHECK on the result of
LOCAL_PEEK_GL_ERROR? 

I suspect that LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER will cause the same mock
functions to still fail, but that could probably be fixed, I think I need to
override GetError somewhere.

If an error were to happen, we probably want it copied into the state, right? 
Maybe we want to even fail the initialize when an error occurs (we'd probably
want to check the error going into the function too then).

What do you think?

[hendrikw, 2015-04-27 22:11:03]

On 2015/04/24 18:33:39, hendrikw wrote:
> PTAL, Thanks!
> 
>
https://codereview.chromium.org/1106773002/diff/20001/content/common/gpu/gpu_...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/1106773002/diff/20001/content/common/gpu/gpu_...
> content/common/gpu/gpu_command_buffer_stub.cc:206: if (is_offscreen &&
> initial_size_.IsEmpty()) {
> I might rename initial_size_ to initial_offscreen_size_ later, it's a bit
> misleading as it.
> 
>
https://codereview.chromium.org/1106773002/diff/20001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/1106773002/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:2467: DCHECK(!offscreen ||
> !offscreen_size.IsEmpty());
> In my first change, I also DCHECK'd that glGetError() == GL_NO_ERROR near the
> end of the function, but we have some mocks that didn't agree with that.
> 
> I think it makes sense to catch this, but maybe I should be calling
> LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER, then DCHECK on the result of
> LOCAL_PEEK_GL_ERROR? 
> 
> I suspect that LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER will cause the same mock
> functions to still fail, but that could probably be fixed, I think I need to
> override GetError somewhere.
> 
> If an error were to happen, we probably want it copied into the state, right? 
> Maybe we want to even fail the initialize when an error occurs (we'd probably
> want to check the error going into the function too then).
> 
> What do you think?

ping

[jbauman, 2015-04-27 22:24:48]

lgtm

[hendrikw, 2015-04-27 23:01:01]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-04-27 23:02:16]

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

[piman, 2015-04-27 23:07:02]

belated lgtm

[commit-bot: I haz the power, 2015-04-27 23:52:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-27 23:53:21]

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