gpu: gpu decoder sets viewport size same to the size of the default framebuffer.

gpu: gpu decoder sets viewport size same to the size of the default framebuffer.

For onscreen, viewport should match window size initially. For offscreen,
viewport should match the default FBO size initially as-is.

Committed: https://crrev.com/4c04a16b74821239a8d850aa1c5357cc5a41cee9
Cr-Commit-Position: refs/heads/master@{#319025}

[dshwang, 2015-03-01 15:09:04]

dongseong.hwang@intel.com changed reviewers:
+ sievers@chromium.org

[dshwang, 2015-03-01 15:09:18]

could you review this simple CL?

[no sievers, 2015-03-02 20:37:12]

https://codereview.chromium.org/965223002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/965223002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2746: state_.viewport_height =
size.height();
What's the bug?

We call InitState() in line 2758, which calls glViewport() with the value in
|state_|, so the driver's state will match our initial state.

Your patch changes the initial state for onscreen surface from 0,0 to 0,1 (w,h),
why is better?

If we want to match the spec better (viewport should match window size
initially) we could try surface_->GetSize() for onscreen contexts. But not sure
that will always work or fix anything. We probably rely on the client to do the
right thing.

[dshwang, 2015-03-03 09:20:30]

I update this CL following your good comment.

https://codereview.chromium.org/965223002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/965223002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2746: state_.viewport_height =
size.height();
On 2015/03/02 20:37:12, sievers wrote:
> What's the bug?

There's no bug. I just confused why onscreen viewport is initially 0, 0.

> We call InitState() in line 2758, which calls glViewport() with the value in
> |state_|, so the driver's state will match our initial state.
> 
> Your patch changes the initial state for onscreen surface from 0,0 to 0,1
(w,h),
> why is better?

previous patch changes from 0,0 to 1,1


> If we want to match the spec better (viewport should match window size
> initially) we could try surface_->GetSize() for onscreen contexts. But not
sure
> that will always work or fix anything.

That's good point! onscreen viewport should match surface_->GetSize()
it's always work because surface must be created before this function.
It doesn't fix anything but it's what code is supposed to be.

[no sievers, 2015-03-03 19:26:27]

lgtm

[dshwang, 2015-03-04 06:32:56]

On 2015/03/03 19:26:27, sievers wrote:
> lgtm

thx for reviewing!

[dshwang, 2015-03-04 06:33:01]

The CQ bit was checked by dongseong.hwang@intel.com

[dshwang, 2015-03-04 06:33:01]

The CQ bit was unchecked by dongseong.hwang@intel.com

[dshwang, 2015-03-04 06:33:02]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2015-03-04 06:33:44]

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

[commit-bot: I haz the power, 2015-03-04 07:27:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-04 07:28:19]

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