gpu: Use a rectangle to keep track of the cleared area of each texture level.

gpu: Use a rectangle to keep track of the cleared area of each texture level.

This makes it possible for a client to initialize a texture using
partial updates without causing the texture level to be implicitly
cleared at the time of the first partial update.

BUG=460288
TEST=gl_tests, gpu_unittests

Committed: https://crrev.com/7c4a13b0f04027293aecec40dc91544307f29746
Cr-Commit-Position: refs/heads/master@{#333154}

[reveman, 2015-05-26 14:47:08]

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

[reveman, 2015-05-26 22:48:35]

Just realized that this is broken. Please ignore this patch until I've uploaded
a new version.

[reveman, 2015-05-26 22:48:49]

reveman@chromium.org changed reviewers:
- piman@chromium.org, zmo@chromium.org

[reveman, 2015-05-27 17:43:26]

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

[reveman, 2015-05-27 17:43:26]

This is now ready for review. Please take a look.

[piman, 2015-05-27 20:50:17]

Could you add a test for the new logic?

https://codereview.chromium.org/1154053002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1154053002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:668: info.cleared_rect =
cleared_rect;
nit: could we have a DCHECK that cleared_rect only grows?

https://codereview.chromium.org/1154053002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:694: }
I worry about the extra tracking cost here. This is called in many places, among
others TexParameteri which we do at run-time to change filtering mode even
though in this case it wouldn't impact cleared state.
level_infos is always the size of max_levels_ (i.e. typically 12-14), and
Rect::Contains is really non-trivial.

Given that this is just initialization state, I think I'd prefer keeping the
existing incremental tracking. We can keep the bulk of the existing logic, it's
just that we need to update when the cleared_rect changes from non-complete to
complete (i.e. you can simply change LevelInfo::cleared to a bool
LevelInfo::cleared() const { return cleared_rect.Contains(gfx::Rect(width,
height); }

[reveman, 2015-06-03 22:40:04]

Latest patch adds these same optimizations to TexSubImage2D and
CopyTexSubImage2D commands. Unit tests have been updated for the new behavior
and
GLES2DecoderTest.CopyTexSubImage2DTwiceClearsUnclearedTexture+GLES2DecoderTest.CopyTexSubImage2DTwiceMarksTextureAsCleared
explicitly check that these optimizations work as expected.

PTAL

https://codereview.chromium.org/1154053002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1154053002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:668: info.cleared_rect =
cleared_rect;
On 2015/05/27 at 20:50:17, piman (Very slow to review) wrote:
> nit: could we have a DCHECK that cleared_rect only grows?

Done. By adding DCHECKs to the caller as we can't always assume that the rect is
growing when setting the LevelInfo::cleared_rect in latest patch.

https://codereview.chromium.org/1154053002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:694: }
On 2015/05/27 at 20:50:17, piman (Very slow to review) wrote:
> I worry about the extra tracking cost here. This is called in many places,
among others TexParameteri which we do at run-time to change filtering mode even
though in this case it wouldn't impact cleared state.
> level_infos is always the size of max_levels_ (i.e. typically 12-14), and
Rect::Contains is really non-trivial.
> 
> Given that this is just initialization state, I think I'd prefer keeping the
existing incremental tracking. We can keep the bulk of the existing logic, it's
just that we need to update when the cleared_rect changes from non-complete to
complete (i.e. you can simply change LevelInfo::cleared to a bool
LevelInfo::cleared() const { return cleared_rect.Contains(gfx::Rect(width,
height); }

Done. However, this is a bit more complicated than simply changing
LevelInfo::cleared into a function as LevelInfo::width/height affects this
state. Latest patch handles that by requiring that all state that affects
whether the level is cleared to be passed to UpdateMipCleared.

[reveman, 2015-06-05 19:38:37]

Fixed some webgl conformance test issues in latest patch. Antoine, please take a
look.

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10005: internal_format,
GL_UNSIGNED_BYTE, zero.get());
I changed this code to be more consistent with DoCopyTexSubImage2D below as we
shouldn't rely on ClearLevel to call TexImage2D. This problem existed prior to
this change- we've just been lucky (or unlucky) that TexImage2D happen to have
been called by ClearLevel in all our tests.

[piman, 2015-06-05 19:55:50]

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8903: // on depth formats.
It sounds like this is still true in the xoffset != 0 / yoffset != 0.

Should we use scissors in that case instead of falling back to texsubimage2d?

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10005: internal_format,
GL_UNSIGNED_BYTE, zero.get());
On 2015/06/05 19:38:37, reveman wrote:
> I changed this code to be more consistent with DoCopyTexSubImage2D below as we
> shouldn't rely on ClearLevel to call TexImage2D. This problem existed prior to
> this change- we've just been lucky (or unlucky) that TexImage2D happen to have
> been called by ClearLevel in all our tests.

That's a nice catch, thanks!

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10011: ScopedModifyPixels
modify(texture_ref);
Can ScopedModifyPixels be nested? Maybe just remove this one?

[reveman, 2015-06-05 20:39:44]

ptal

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8903: // on depth formats.
On 2015/06/05 at 19:55:49, piman (Very slow to review) wrote:
> It sounds like this is still true in the xoffset != 0 / yoffset != 0.
> 
> Should we use scissors in that case instead of falling back to texsubimage2d?

Yes, I was trying to avoid that but we have to handle width/height properly so
scissors is needed anyhow. Fixed in latest patch.

https://codereview.chromium.org/1154053002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10011: ScopedModifyPixels
modify(texture_ref);
On 2015/06/05 at 19:55:50, piman (Very slow to review) wrote:
> Can ScopedModifyPixels be nested? Maybe just remove this one?

Done.

[piman, 2015-06-05 20:46:31]

lgtm

[reveman, 2015-06-05 20:47:06]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-06-05 20:49:26]

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

[commit-bot: I haz the power, 2015-06-05 21:13:54]

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

[commit-bot: I haz the power, 2015-06-05 21:13:57]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[reveman, 2015-06-05 21:21:37]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-06-05 21:21:38]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1154053002/#ps220001
(title: "restore scissor state in GLES2DecoderImpl::ClearLevel and update
GLES2DecoderManualInitTest.DrawCle…")

[commit-bot: I haz the power, 2015-06-05 21:22:30]

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

[commit-bot: I haz the power, 2015-06-05 22:28:23]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-06-05 22:30:18]

Patchset 12 (id:??) landed as
https://crrev.com/7c4a13b0f04027293aecec40dc91544307f29746
Cr-Commit-Position: refs/heads/master@{#333154}

[Ken Russell (switch to Gerrit), 2015-06-06 00:33:04]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-06-06 00:33:05]

https://codereview.chromium.org/1154053002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1154053002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8935: glScissor(xoffset,
yoffset, width, height);
Can this end up changing the user's scissor rectangle out from under them?

[piman, 2015-06-06 00:48:13]

https://codereview.chromium.org/1154053002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1154053002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8935: glScissor(xoffset,
yoffset, width, height);
On 2015/06/06 00:33:05, Ken Russell wrote:
> Can this end up changing the user's scissor rectangle out from under them?

Good point, we should restore the rectangle, either immediately after the clear
or in ApplyDirtyState

[reveman, 2015-06-09 02:30:36]

On 2015/06/06 at 00:48:13, piman wrote:
>
https://codereview.chromium.org/1154053002/diff/220001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/1154053002/diff/220001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:8935: glScissor(xoffset,
yoffset, width, height);
> On 2015/06/06 00:33:05, Ken Russell wrote:
> > Can this end up changing the user's scissor rectangle out from under them?
> 
> Good point, we should restore the rectangle, either immediately after the
clear or in ApplyDirtyState

Uploaded at patch for this here: https://codereview.chromium.org/1168213003