gpu: Avoid detaching images with glTexSubImage2D

gpu: Avoid detaching images with glTexSubImage2D

Avoid optimization of using glTexImage2D over glTexSubImage2D when there
are EGLImages attached to the texture, as glTexImage2D will orphan the
EGLImage. Orphaning and reattachment is probably more expensive than the
win of using glTexImage2D over glTexSubImage2D.

BUG=460344
Committed: https://crrev.com/45dc77a1ea86f69cce11e72bf5782310b5cb084b
Cr-Commit-Position: refs/heads/master@{#318141}

[boliu, 2015-02-25 00:18:02]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2015-02-25 00:18:03]

ptal

[no sievers, 2015-02-25 01:20:14]

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

[no sievers, 2015-02-25 01:23:27]

+reveman also

I think this might also fix a theoretical bug in 0-copy where we could otherwise
end up orphaning the texture from an EGL image if the client was updating it
through texSubImage2D and the decoder did texImage2D instead. (The
clear-before-draw path probably doesn't affect sw raster and 0-/1-copy.)

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

https://codereview.chromium.org/954053002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8494:
texture_manager()->GetTextureForServiceId(service_id);
Can you change it instead to pass in texture?
And then just do texture->service_id() wherever it needs that in here.

[no sievers, 2015-02-25 01:27:14]

Oh can you add a unit test? GLES2CmdDecoderTest*,  something of the sort.
I'm pretty sure something already exists that tests GLImages (and probably the
orphaning logic) for reference.

For the one test of the two you'd need to init it with
texsubimage2d_faster_than_teximage2d forced, which you can do through
initializing with a cmdline:

base::CommandLine command_line(0, NULL);
command_line.AppendSwitchASCII(
    switches::kGpuDriverBugWorkarounds,
    base::IntToString(gpu::TEXSUBIMAGE2D_FASTER_THAN_TEXIMAGE2D));
InitDecoderWithCommandLine(init, &command_line);

[boliu, 2015-02-25 02:18:34]

On 2015/02/25 01:27:14, sievers wrote:
> Oh can you add a unit test? GLES2CmdDecoderTest*,  something of the sort.
> I'm pretty sure something already exists that tests GLImages (and probably the
> orphaning logic) for reference.

Meh, the existing test OrphanGLImageWithTexImage2D isn't actually testing
anything. The GLImage never got attached in the first place.

> 
> For the one test of the two you'd need to init it with
> texsubimage2d_faster_than_teximage2d forced, which you can do through
> initializing with a cmdline:
> 
> base::CommandLine command_line(0, NULL);
> command_line.AppendSwitchASCII(
>     switches::kGpuDriverBugWorkarounds,
>     base::IntToString(gpu::TEXSUBIMAGE2D_FASTER_THAN_TEXIMAGE2D));
> InitDecoderWithCommandLine(init, &command_line);

[reveman, 2015-02-25 03:48:53]

lgtm % sievers review

[boliu, 2015-02-25 18:37:32]

PS2 address review comments, and fixes the existing orphan unit test. No new
tests yet.

So for new tests, they can only veirfy glSubTexImage2D instead of glTexImage2D,
since there's nothing else to check when all the GL is mocked out.

[boliu, 2015-02-25 19:52:38]

On 2015/02/25 01:27:14, sievers wrote:
> Oh can you add a unit test? GLES2CmdDecoderTest*,  something of the sort.
> I'm pretty sure something already exists that tests GLImages (and probably the
> orphaning logic) for reference.
> 
> For the one test of the two you'd need to init it with
> texsubimage2d_faster_than_teximage2d forced, which you can do through
> initializing with a cmdline:
> 
> base::CommandLine command_line(0, NULL);
> command_line.AppendSwitchASCII(
>     switches::kGpuDriverBugWorkarounds,
>     base::IntToString(gpu::TEXSUBIMAGE2D_FASTER_THAN_TEXIMAGE2D));
> InitDecoderWithCommandLine(init, &command_line);

For the TexSubImage2D test, I want the exact opposite of this.
TEXSUBIMAGE2D_FASTER_THAN_TEXIMAGE2D should be *false*, so TexSubImage2D would
get optimized to TexImage2D without this fix. Is it ok to just assume the
workaround is false in the test?

[no sievers, 2015-02-25 20:50:31]

On 2015/02/25 19:52:38, boliu wrote:
> On 2015/02/25 01:27:14, sievers wrote:
> > Oh can you add a unit test? GLES2CmdDecoderTest*,  something of the sort.
> > I'm pretty sure something already exists that tests GLImages (and probably
the
> > orphaning logic) for reference.
> > 
> > For the one test of the two you'd need to init it with
> > texsubimage2d_faster_than_teximage2d forced, which you can do through
> > initializing with a cmdline:
> > 
> > base::CommandLine command_line(0, NULL);
> > command_line.AppendSwitchASCII(
> >     switches::kGpuDriverBugWorkarounds,
> >     base::IntToString(gpu::TEXSUBIMAGE2D_FASTER_THAN_TEXIMAGE2D));
> > InitDecoderWithCommandLine(init, &command_line);
> 
> For the TexSubImage2D test, I want the exact opposite of this.
> TEXSUBIMAGE2D_FASTER_THAN_TEXIMAGE2D should be *false*, so TexSubImage2D would
> get optimized to TexImage2D without this fix. Is it ok to just assume the
> workaround is false in the test?

My bad, so just
DCHECK(!group_->feature_info()->workarounds().texsubimage2d_faster_than_teximage2d)
then in the test?
With a comment saying that it specifically tests that the presence of image
attachments should override the optimization.

[boliu, 2015-02-25 21:34:02]

new tests added in PS3, ptal

[no sievers, 2015-02-25 21:52:49]

lgtm

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

https://codereview.chromium.org/954053002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:2235:
ASSERT_FALSE(
nit: put comment "this specifically tests that we don't use texImage2D instead
of texSubImage2D in the presence of image attachments"

https://codereview.chromium.org/954053002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:2252: uint32_t
pixels_shm_offset = 0;
kSharedMemoryOffset

https://codereview.chromium.org/954053002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:2260:
BindTexImage2DCHROMIUM bind_tex_image_2d_cmd;
nit: line 2260-2276 seems to be duplicated a few times (incl preexisting and new
code), maybe we should have a GLES2CmdDecoderBase::DoBindTexImage2D(target,
image_id)?

https://codereview.chromium.org/954053002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:2306: uint32_t
pixels_shm_offset = 0;
kSharedMemoryOffset

https://codereview.chromium.org/954053002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:2331: //
ClearLevel use glTexSubImage2D to avoid unbinding GLImage.
nit: 'should use'

[boliu, 2015-02-25 22:18:50]

all done

Factored out DoBindTexImage2DCHROMIUM and also updated a few other call sites

[boliu, 2015-02-25 22:19:10]

The CQ bit was checked by boliu@chromium.org

[boliu, 2015-02-25 22:19:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/954053002/#ps60001 (title:
"review")

[commit-bot: I haz the power, 2015-02-25 22:20:41]

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

[commit-bot: I haz the power, 2015-02-25 23:26:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-25 23:28:10]

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