Add support for compressed GPU memory buffers.

Add support for compressed GPU memory buffers.

This CL adds support for compressed GPU buffer formats.

BUG=434699
Committed: https://crrev.com/3113db1250a2687c030585f6045f2ef242b98db6
Cr-Commit-Position: refs/heads/master@{#317765}

Committed: https://crrev.com/07b6d8b077f5a5e09b1106c7aa6d916e5d4f9e63
Cr-Commit-Position: refs/heads/master@{#318004}

[christiank, 2015-02-11 13:06:28]

christiank@opera.com changed reviewers:
+ jbauman@chromium.org, reveman@chromium.org

[christiank, 2015-02-11 13:06:29]

Hello,

Here's another CL that adds functionality needed by the tile texture compression
feature.

It's basically a continuation of the following CL:
https://codereview.chromium.org/806653006

[reveman, 2015-02-11 13:58:42]

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:243: glCompressedTexSubImage2D(target,
Should we avoid calling CompressedTexSubImage2D if the format is not supported
by the GL?

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:387: size_t
GLImageMemory::MemoryBytes(gfx::GpuMemoryBuffer::Format format) {
Please move this to anonymous namespace, rename it to SizeInBytes and pass size
to it as parameter.

[christiank, 2015-02-12 08:41:52]

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:243: glCompressedTexSubImage2D(target,
On 2015/02/11 13:58:40, reveman wrote:
> Should we avoid calling CompressedTexSubImage2D if the format is not supported
> by the GL?

Yes, that's the idea. The code assumes that we only try to use a compressed
format that's supported by the GL device.

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:387: size_t
GLImageMemory::MemoryBytes(gfx::GpuMemoryBuffer::Format format) {
On 2015/02/11 13:58:40, reveman wrote:
> Please move this to anonymous namespace, rename it to SizeInBytes and pass
size
> to it as parameter.

Sounds good, fixed.

[reveman, 2015-02-12 12:51:28]

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:243: glCompressedTexSubImage2D(target,
On 2015/02/12 08:41:52, christiank wrote:
> On 2015/02/11 13:58:40, reveman wrote:
> > Should we avoid calling CompressedTexSubImage2D if the format is not
supported
> > by the GL?
> 
> Yes, that's the idea. The code assumes that we only try to use a compressed
> format that's supported by the GL device.

What's preventing a malicious renderer from being able to issue a
CompressedTexSubImage call with an invalid format here?

[christiank, 2015-02-12 13:54:43]

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:243: glCompressedTexSubImage2D(target,
On 2015/02/12 12:51:28, reveman wrote:
> On 2015/02/12 08:41:52, christiank wrote:
> > On 2015/02/11 13:58:40, reveman wrote:
> > > Should we avoid calling CompressedTexSubImage2D if the format is not
> supported
> > > by the GL?
> > 
> > Yes, that's the idea. The code assumes that we only try to use a compressed
> > format that's supported by the GL device.
> 
> What's preventing a malicious renderer from being able to issue a
> CompressedTexSubImage call with an invalid format here?

I guess that's possible. One could argue that a malicious renderer could do the
same for the non-compressed path. How is the compressed path different from the
non-compressed in this case? Is it the difference in buffer size that's cause
for concern?

I could add some checks, but I am not quite sure how to query GL features from
here. Any ideas?

[reveman, 2015-02-12 14:18:00]

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:243: glCompressedTexSubImage2D(target,
On 2015/02/12 13:54:42, christiank wrote:
> On 2015/02/12 12:51:28, reveman wrote:
> > On 2015/02/12 08:41:52, christiank wrote:
> > > On 2015/02/11 13:58:40, reveman wrote:
> > > > Should we avoid calling CompressedTexSubImage2D if the format is not
> > supported
> > > > by the GL?
> > > 
> > > Yes, that's the idea. The code assumes that we only try to use a
compressed
> > > format that's supported by the GL device.
> > 
> > What's preventing a malicious renderer from being able to issue a
> > CompressedTexSubImage call with an invalid format here?
> 
> I guess that's possible. One could argue that a malicious renderer could do
the
> same for the non-compressed path. How is the compressed path different from
the
> non-compressed in this case? Is it the difference in buffer size that's cause
> for concern?

Yes, that could be a problem. In general, we shouldn't allow the renderer to use
the GL in a way that generate errors. We should detect incorrect usage and
handle these errors ourselves. 

> 
> I could add some checks, but I am not quite sure how to query GL features from
> here. Any ideas?

I don't think the check belong here. I think they should either be in
GLES2CmdDecoder or maybe we should prevent the GLImage from being created in the
first place with a format that is not supported by the GL. Maybe we can add a
check to GpuCommandBufferStub::OnCreateImage.

[christiank, 2015-02-13 10:08:36]

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:243: glCompressedTexSubImage2D(target,
On 2015/02/12 14:18:00, reveman wrote:
> On 2015/02/12 13:54:42, christiank wrote:
> > On 2015/02/12 12:51:28, reveman wrote:
> > > On 2015/02/12 08:41:52, christiank wrote:
> > > > On 2015/02/11 13:58:40, reveman wrote:
> > > > > Should we avoid calling CompressedTexSubImage2D if the format is not
> > > supported
> > > > > by the GL?
> > > > 
> > > > Yes, that's the idea. The code assumes that we only try to use a
> compressed
> > > > format that's supported by the GL device.
> > > 
> > > What's preventing a malicious renderer from being able to issue a
> > > CompressedTexSubImage call with an invalid format here?
> > 
> > I guess that's possible. One could argue that a malicious renderer could do
> the
> > same for the non-compressed path. How is the compressed path different from
> the
> > non-compressed in this case? Is it the difference in buffer size that's
cause
> > for concern?
> 
> Yes, that could be a problem. In general, we shouldn't allow the renderer to
use
> the GL in a way that generate errors. We should detect incorrect usage and
> handle these errors ourselves. 
> 
> > 
> > I could add some checks, but I am not quite sure how to query GL features
from
> > here. Any ideas?
> 
> I don't think the check belong here. I think they should either be in
> GLES2CmdDecoder or maybe we should prevent the GLImage from being created in
the
> first place with a format that is not supported by the GL. Maybe we can add a
> check to GpuCommandBufferStub::OnCreateImage.

I have now added some checks to GpuCommandBufferStub::OnCreateImage, what do you
think?

[reveman, 2015-02-13 23:51:24]

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

[reveman, 2015-02-13 23:51:25]

lgtm % piman's review and adding utility function for format check

https://codereview.chromium.org/916083002/diff/40001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/916083002/diff/40001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:987: }
Looks good. Can you just move this into a IsSupportedImageFormat helper function
with

NOTREACHED();
return false;

at the end? I think that would be a bit cleaner and we'd also detect if format
was ever an invalid value.

[piman, 2015-02-14 00:50:21]

We need to be very careful with the block size.

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:162: DCHECK_EQ(width % 2, 0U);
Hmm, this needs to be validated somewhere, we can't rely on the client side to
do the right thing.

OTOH, I think most (all?) compressed formats support texture sizes that are not
a multiple of the block size... essentially they need to provide a full block
but they can limit the texture definition to a subset of it. I.e it is valid to
call glCompressedTexImage2D(GL_TEXTURE_2D, 0, COMPRESSED_RGBA_S3TC_DXT1_EXT, 3,
3, 0, 8, data).

Is this something we should support? Otherwise, we need to add logic to verify
both width and height are both a multiple of the block size.

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:256: size_.width(), size_.height(),
For the SubImage case, we definitely need width and height to be a multiple of
the block size.

[christiank, 2015-02-18 14:38:25]

https://codereview.chromium.org/916083002/diff/40001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/916083002/diff/40001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:987: }
On 2015/02/13 23:51:25, reveman wrote:
> Looks good. Can you just move this into a IsSupportedImageFormat helper
function
> with
> 
> NOTREACHED();
> return false;
> 
> at the end? I think that would be a bit cleaner and we'd also detect if format
> was ever an invalid value.

Sure! I'll fix this.

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:162: DCHECK_EQ(width % 2, 0U);
On 2015/02/14 00:50:21, piman (Very slow to review) wrote:
> Hmm, this needs to be validated somewhere, we can't rely on the client side to
> do the right thing.
> 
> OTOH, I think most (all?) compressed formats support texture sizes that are
not
> a multiple of the block size... essentially they need to provide a full block
> but they can limit the texture definition to a subset of it. I.e it is valid
to
> call glCompressedTexImage2D(GL_TEXTURE_2D, 0, COMPRESSED_RGBA_S3TC_DXT1_EXT,
3,
> 3, 0, 8, data).
> 
> Is this something we should support? Otherwise, we need to add logic to verify
> both width and height are both a multiple of the block size.

We only plan on supporting compressed textures that are a multiple of the block
size in size.

What do you think about adding a check in GpuCommandBufferStub::OnCreateImage?
Will that cover it?

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:256: size_.width(), size_.height(),
On 2015/02/14 00:50:21, piman (Very slow to review) wrote:
> For the SubImage case, we definitely need width and height to be a multiple of
> the block size.

Adding a check in GpuCommandBufferStub::OnCreateImage would cover this as well,
right?

[reveman, 2015-02-18 16:10:30]

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:162: DCHECK_EQ(width % 2, 0U);
On 2015/02/18 at 14:38:25, christiank wrote:
> On 2015/02/14 00:50:21, piman (Very slow to review) wrote:
> > Hmm, this needs to be validated somewhere, we can't rely on the client side
to
> > do the right thing.
> > 
> > OTOH, I think most (all?) compressed formats support texture sizes that are
not
> > a multiple of the block size... essentially they need to provide a full
block
> > but they can limit the texture definition to a subset of it. I.e it is valid
to
> > call glCompressedTexImage2D(GL_TEXTURE_2D, 0, COMPRESSED_RGBA_S3TC_DXT1_EXT,
3,
> > 3, 0, 8, data).
> > 
> > Is this something we should support? Otherwise, we need to add logic to
verify
> > both width and height are both a multiple of the block size.
> 
> We only plan on supporting compressed textures that are a multiple of the
block size in size.
> 
> What do you think about adding a check in GpuCommandBufferStub::OnCreateImage?
Will that cover it?

Could we have SizeInBytes in gl_image_shared_memory.cc fail instead? Maybe add a
GLImageMemory::ValidHeight(size_t height, gfx::GpuMemoryBuffer::Format format)
function and this function can just return false if width is not valid.

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:162: DCHECK_EQ(width % 2, 0U);
On 2015/02/18 at 14:38:25, christiank wrote:
> On 2015/02/14 00:50:21, piman (Very slow to review) wrote:
> > Hmm, this needs to be validated somewhere, we can't rely on the client side
to
> > do the right thing.
> > 
> > OTOH, I think most (all?) compressed formats support texture sizes that are
not
> > a multiple of the block size... essentially they need to provide a full
block
> > but they can limit the texture definition to a subset of it. I.e it is valid
to
> > call glCompressedTexImage2D(GL_TEXTURE_2D, 0, COMPRESSED_RGBA_S3TC_DXT1_EXT,
3,
> > 3, 0, 8, data).
> > 
> > Is this something we should support? Otherwise, we need to add logic to
verify
> > both width and height are both a multiple of the block size.
> 
> We only plan on supporting compressed textures that are a multiple of the
block size in size.
> 
> What do you think about adding a check in GpuCommandBufferStub::OnCreateImage?
Will that cover it?

Could we have SizeInBytes in gl_image_shared_memory.cc fail instead? Maybe add a
GLImageMemory::ValidHeight(size_t height, gfx::GpuMemoryBuffer::Format format)
function and this function can just return false if width is not valid. Would be
nice to keep these checks at this level rather than GpuCommandBufferStub if
possible as it would cover more use-cases. ie. single process unit tests..

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:256: size_.width(), size_.height(),
On 2015/02/18 at 14:38:25, christiank wrote:
> On 2015/02/14 00:50:21, piman (Very slow to review) wrote:
> > For the SubImage case, we definitely need width and height to be a multiple
of
> > the block size.
> 
> Adding a check in GpuCommandBufferStub::OnCreateImage would cover this as
well, right?

Yes, or a check in SizeInBytes if we go that way.

[christiank, 2015-02-19 10:56:25]

New patchsets have been uploaded after l-g-t-m from reveman@chromium.org

[christiank, 2015-02-19 10:56:56]

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/40001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:162: DCHECK_EQ(width % 2, 0U);
On 2015/02/18 16:10:30, reveman wrote:
> On 2015/02/18 at 14:38:25, christiank wrote:
> > On 2015/02/14 00:50:21, piman (Very slow to review) wrote:
> > > Hmm, this needs to be validated somewhere, we can't rely on the client
side
> to
> > > do the right thing.
> > > 
> > > OTOH, I think most (all?) compressed formats support texture sizes that
are
> not
> > > a multiple of the block size... essentially they need to provide a full
> block
> > > but they can limit the texture definition to a subset of it. I.e it is
valid
> to
> > > call glCompressedTexImage2D(GL_TEXTURE_2D, 0,
COMPRESSED_RGBA_S3TC_DXT1_EXT,
> 3,
> > > 3, 0, 8, data).
> > > 
> > > Is this something we should support? Otherwise, we need to add logic to
> verify
> > > both width and height are both a multiple of the block size.
> > 
> > We only plan on supporting compressed textures that are a multiple of the
> block size in size.
> > 
> > What do you think about adding a check in
GpuCommandBufferStub::OnCreateImage?
> Will that cover it?
> 
> Could we have SizeInBytes in gl_image_shared_memory.cc fail instead? Maybe add
a
> GLImageMemory::ValidHeight(size_t height, gfx::GpuMemoryBuffer::Format format)
> function and this function can just return false if width is not valid. Would
be
> nice to keep these checks at this level rather than GpuCommandBufferStub if
> possible as it would cover more use-cases. ie. single process unit tests..

Sure, sounds good to me. I have now added size checking.

[reveman, 2015-02-19 14:21:53]

thanks! lgtm

[piman, 2015-02-19 23:52:04]

LGTM+nits

https://codereview.chromium.org/916083002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/916083002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.cc:92: DCHECK_EQ(width % 2,
0U);
Could we add a DCHECK, somewhere on the client side (the constructor?), that the
size is valid? E.g. by sharing that ValidSize function.
It's easier to find the root cause of errors on the client side with a DCHEKC
than on the service side with a logged message. WDYT?

https://codereview.chromium.org/916083002/diff/60001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/60001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:47: bool CompressedFormat(gfx::GpuMemoryBuffer::Format
format) {
nit: IsCompressedFormat?

[christiank, 2015-02-20 08:21:46]

New patchsets have been uploaded after l-g-t-m from
reveman@chromium.org,piman@chromium.org

[christiank, 2015-02-20 08:32:15]

jbauman, would you please have a look?

Thanks!

https://codereview.chromium.org/916083002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/916083002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.cc:92: DCHECK_EQ(width % 2,
0U);
On 2015/02/19 23:52:04, piman (Very slow to review) wrote:
> Could we add a DCHECK, somewhere on the client side (the constructor?), that
the
> size is valid? E.g. by sharing that ValidSize function.
> It's easier to find the root cause of errors on the client side with a DCHEKC
> than on the service side with a logged message. WDYT?

I agree, sounds like a good idea. Fixed.

https://codereview.chromium.org/916083002/diff/60001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/916083002/diff/60001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:47: bool CompressedFormat(gfx::GpuMemoryBuffer::Format
format) {
On 2015/02/19 23:52:04, piman (Very slow to review) wrote:
> nit: IsCompressedFormat?

Fixed! I originally went with CompressedFormat to follow the spirit of
ValidInternalFormat and ValidFormat. But they should probably be renamed as
well.

[jbauman, 2015-02-20 23:27:50]

lgtm

[christiank, 2015-02-23 07:35:45]

The CQ bit was checked by christiank@opera.com

[commit-bot: I haz the power, 2015-02-23 07:36:36]

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

[commit-bot: I haz the power, 2015-02-23 08:02:47]

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

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

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

[christiank, 2015-02-23 08:44:59]

New patchsets have been uploaded after l-g-t-m from jbauman@chromium.org

[christiank, 2015-02-23 08:46:04]

The CQ bit was checked by christiank@opera.com

[commit-bot: I haz the power, 2015-02-23 08:47:00]

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

[commit-bot: I haz the power, 2015-02-23 08:49:53]

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

[commit-bot: I haz the power, 2015-02-23 08:49:55]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[christiank, 2015-02-24 07:02:31]

The CQ bit was checked by christiank@opera.com

[commit-bot: I haz the power, 2015-02-24 07:03:12]

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

[commit-bot: I haz the power, 2015-02-24 08:19:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-24 08:21:11]

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

[phoglund_chromium, 2015-02-24 08:57:13]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/948323002/ by phoglund@chromium.org.

The reason for reverting is: Breaks ChromiumOS ozone compile:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

../../ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:78:11:
error: 5 enumeration values not handled in switch: 'ATC', 'ATCIA', 'DXT1'...
[-Werror,-Wswitch]
.

[christiank, 2015-02-24 11:22:07]

christiank@opera.com changed reviewers:
+ spang@chromium.org

[christiank, 2015-02-24 11:23:14]

spang, would you please have a look at the ozone changes that I forgot to add in
the initial commit?

[spang, 2015-02-24 18:09:22]

lgtm

[christiank, 2015-02-25 08:02:20]

The CQ bit was checked by christiank@opera.com

[christiank, 2015-02-25 08:02:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, jbauman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/916083002/#ps140001
(title: "Fix ozone")

[commit-bot: I haz the power, 2015-02-25 08:02:38]

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

[commit-bot: I haz the power, 2015-02-25 09:41:38]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-25 09:42:22]

Patchset 8 (id:??) landed as
https://crrev.com/07b6d8b077f5a5e09b1106c7aa6d916e5d4f9e63
Cr-Commit-Position: refs/heads/master@{#318004}