Add support for DXT3 and DXT5

Add support for DXT3 and DXT5

TEST=none
BUG=none
Committed: http://code.google.com/p/angleproject/source/detail?r=721

[apatrick1, 2011-08-01 19:10:57]

You'll need to flip the blocks differently for DXT3 and DXT5. The reason for
that code is because we store rows of surfaces in the reverse order to the usual
D3D convention. It saves a redundant blit to flip the back buffer vertically on
swapbuffers if all surfaces are "upside down" in D3D terms.

It should be fairly easy, the DXT3 and DXT5 blocks just have a slightly
different layout.

[dgkoch, 2011-08-01 20:11:47]

Please provide an specification for the new extension(s) (we're keeping them
locally in the extensions/ folder).  It seems reasonable that you could include
both in the same document. I presume this should be largely be an augmentation
of EXT_texture_compression_dxt1 to support dxt3/5, but with the same
restrictions compared to EXT_texture_compression_s3tc.  Once you've got it
documented we can see if there is interest from any of the other ES vendors to
making it an EXT.

I'm fine with the white-space changes you made, but I'd prefer if they were done
as a separate patch, just to make it easier to find the functional changes.

As Al stated, the twiddling of the compressed blocks is to make up for the
different origin convention between OpenGL and D3D (and due to the way we've
implemented it by flipping the texture coordinates in the shaders).

http://codereview.appspot.com/4809068/diff/1/include/GLES2/gl2ext.h
File include/GLES2/gl2ext.h (right):

http://codereview.appspot.com/4809068/diff/1/include/GLES2/gl2ext.h#newcode830
include/GLES2/gl2ext.h:830: #define GL_COMPRESSED_RGBA_S3TC_DXT3_EXT  0x83F2
this should be labelled as an _ANGLE token not _EXT (it can still use the same
value though).

http://codereview.appspot.com/4809068/diff/1/include/GLES2/gl2ext.h#newcode836
include/GLES2/gl2ext.h:836: #define GL_COMPRESSED_RGBA_S3TC_DXT5_EXT  0x83F3
this should be labelled as an _ANGLE token not _EXT (it can still use the same
value though).

http://codereview.appspot.com/4809068/diff/1/src/libEGL/Display.cpp
File src/libEGL/Display.cpp (right):

http://codereview.appspot.com/4809068/diff/1/src/libEGL/Display.cpp#newcode783
src/libEGL/Display.cpp:783: SUCCEEDED(mD3d9->CheckDeviceFormat(mAdapter,
mDeviceType, currentDisplayMode.Format, 0, D3DRTYPE_TEXTURE, D3DFMT_DXT5));
This logic will prevent us from supporting the DXT1 one extension on hardware
that doesn't also support DXT3/5.  (I don't actually know of any
hardware/drivers light this, but it might be better to maintain separate
checks).

http://codereview.appspot.com/4809068/diff/1/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/4809068/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:1248: *params = 2;
This needs to be updated to handle the new formats.

http://codereview.appspot.com/4809068/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:1427: case GL_COMPRESSED_TEXTURE_FORMATS: /* no
compressed texture formats are supported */
Whups -- this isn't true any more. This should be updated.

[gmanchromium, 2011-08-04 04:10:13]

http://codereview.appspot.com/4809068/diff/1/include/GLES2/gl2ext.h
File include/GLES2/gl2ext.h (right):

http://codereview.appspot.com/4809068/diff/1/include/GLES2/gl2ext.h#newcode830
include/GLES2/gl2ext.h:830: #define GL_COMPRESSED_RGBA_S3TC_DXT3_EXT  0x83F2
On 2011/08/01 20:11:48, dgkoch wrote:
> this should be labelled as an _ANGLE token not _EXT (it can still use the same
> value though).

Done.

http://codereview.appspot.com/4809068/diff/1/include/GLES2/gl2ext.h#newcode836
include/GLES2/gl2ext.h:836: #define GL_COMPRESSED_RGBA_S3TC_DXT5_EXT  0x83F3
On 2011/08/01 20:11:48, dgkoch wrote:
> this should be labelled as an _ANGLE token not _EXT (it can still use the same
> value though).

Done.

http://codereview.appspot.com/4809068/diff/1/src/libEGL/Display.cpp
File src/libEGL/Display.cpp (right):

http://codereview.appspot.com/4809068/diff/1/src/libEGL/Display.cpp#newcode783
src/libEGL/Display.cpp:783: SUCCEEDED(mD3d9->CheckDeviceFormat(mAdapter,
mDeviceType, currentDisplayMode.Format, 0, D3DRTYPE_TEXTURE, D3DFMT_DXT5));
On 2011/08/01 20:11:48, dgkoch wrote:
> This logic will prevent us from supporting the DXT1 one extension on hardware
> that doesn't also support DXT3/5.  (I don't actually know of any
> hardware/drivers light this, but it might be better to maintain separate
> checks).

Done.

http://codereview.appspot.com/4809068/diff/1/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/4809068/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:1248: *params = 2;
On 2011/08/01 20:11:48, dgkoch wrote:
> This needs to be updated to handle the new formats.

Done.

http://codereview.appspot.com/4809068/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:1427: case GL_COMPRESSED_TEXTURE_FORMATS: /* no
compressed texture formats are supported */
On 2011/08/01 20:11:48, dgkoch wrote:
> Whups -- this isn't true any more. This should be updated.

Done.

[dgkoch, 2011-08-05 05:36:24]

Hi Gregg,

A few suggestions for the extension spec, but otherwise it looks good with the
exception of a couple of indenting issues.

Daniel

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
File extensions/ANGLE_texture_compression_dxt.txt (right):

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:12: Contributors to
ANGLE_texture_compression_dxt
normally this would be used to refer to authors of an extension this is based
on..

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:15: Al Patrick, TransGaming Inc.
really?

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:45: See
EXT_texture_compression_dxt1 for the full list of changes.
Might also want to include a reference to EXT_texture_compression_s3tc for the
documentation of the formats.

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:60: COMPRESSED_RGBA_S3TC_DXT3_EXT  
                  0x83F2
should be COMPRESSED_RGBA_S3TC_DXT3_ANGLE

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:61: COMPRESSED_RGBA_S3TC_DXT5_EXT  
                  0x83F3
should be COMPRESSED_RGBA_S3TC_DXT5_ANGLE

http://codereview.appspot.com/4809068/diff/10001/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/4809068/diff/10001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:975: dest[x] = source[x];
is the indenting off here?

http://codereview.appspot.com/4809068/diff/10001/src/libGLESv2/libGLESv2.cpp
File src/libGLESv2/libGLESv2.cpp (right):

http://codereview.appspot.com/4809068/diff/10001/src/libGLESv2/libGLESv2.cpp#...
src/libGLESv2/libGLESv2.cpp:832: }
indenting of this block looks a bit off?

[gmanchromium, 2011-08-08 05:42:23]

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
File extensions/ANGLE_texture_compression_dxt.txt (right):

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:12: Contributors to
ANGLE_texture_compression_dxt
On 2011/08/05 05:36:24, dgkoch wrote:
> normally this would be used to refer to authors of an extension this is based
> on..

I'm not sure you what you mean. Should I copy the contributors list from
EXT_texture_compression_dxt1? I'm not comfortable adding names of people I don't
know. I kind of assuming referring to the other extensions was enough. No?

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:15: Al Patrick, TransGaming Inc.
On 2011/08/05 05:36:24, dgkoch wrote:
> really?

Done.

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:45: See
EXT_texture_compression_dxt1 for the full list of changes.
On 2011/08/05 05:36:24, dgkoch wrote:
> Might also want to include a reference to EXT_texture_compression_s3tc for the
> documentation of the formats.

Done.

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:60: COMPRESSED_RGBA_S3TC_DXT3_EXT  
                  0x83F2
On 2011/08/05 05:36:24, dgkoch wrote:
> should be COMPRESSED_RGBA_S3TC_DXT3_ANGLE

Done.

http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
extensions/ANGLE_texture_compression_dxt.txt:61: COMPRESSED_RGBA_S3TC_DXT5_EXT  
                  0x83F3
On 2011/08/05 05:36:24, dgkoch wrote:
> should be COMPRESSED_RGBA_S3TC_DXT5_ANGLE

Done.

[gmanchromium, 2011-08-08 07:31:06]

PS: if this looks good to you could you either check it in or add me as a
committer? Thanks.

[dgkoch, 2011-08-08 15:04:46]

On 2011/08/08 05:42:23, gmanchromium wrote:
>
http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
> File extensions/ANGLE_texture_compression_dxt.txt (right):
> 
>
http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
> extensions/ANGLE_texture_compression_dxt.txt:12: Contributors to
> ANGLE_texture_compression_dxt
> On 2011/08/05 05:36:24, dgkoch wrote:
> > normally this would be used to refer to authors of an extension this is
based
> > on..
> 
> I'm not sure you what you mean. Should I copy the contributors list from
> EXT_texture_compression_dxt1? I'm not comfortable adding names of people I
don't
> know. I kind of assuming referring to the other extensions was enough. No?
> 

It was self-referential.  It should have said "Contributors to
EXT_texture_compression_dxt1"  
Instead it say the contributors to *this* extension are the contributors to this
extension.

[gmanchromium, 2011-08-08 15:15:15]

On 2011/08/08 15:04:46, dgkoch wrote:
> On 2011/08/08 05:42:23, gmanchromium wrote:
> >
>
http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
> > File extensions/ANGLE_texture_compression_dxt.txt (right):
> > 
> >
>
http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
> > extensions/ANGLE_texture_compression_dxt.txt:12: Contributors to
> > ANGLE_texture_compression_dxt
> > On 2011/08/05 05:36:24, dgkoch wrote:
> > > normally this would be used to refer to authors of an extension this is
> based
> > > on..
> > 
> > I'm not sure you what you mean. Should I copy the contributors list from
> > EXT_texture_compression_dxt1? I'm not comfortable adding names of people I
> don't
> > know. I kind of assuming referring to the other extensions was enough. No?
> > 
> 
> It was self-referential.  It should have said "Contributors to
> EXT_texture_compression_dxt1"  
> Instead it say the contributors to *this* extension are the contributors to
this
> extension.

Ok, so is it correct now? The current version is here

http://codereview.appspot.com/4809068/diff/15001/extensions/ANGLE_texture_com...

It seems to match this one

http://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_type_2_10_10_...

Otherwise I can just remove the 'contributors' section. Most of the other
extension specs don't have one. I just copied some other spec's format and
happened to pick one with a 'contributors' section.

[dgkoch, 2011-08-08 15:18:51]

LGTM.

Note that you need to update BUILD_REVISION in src/common/version.h to match the
expected SVN revision of your checkin before committing.
(yes it's horribly manual there should be a better way, etc etc).

Your google.com account was already a committer but I've also added the chromium
version as well.

Daniel

[dgkoch, 2011-08-08 15:19:38]

On 2011/08/08 15:15:15, gmanchromium wrote:
> On 2011/08/08 15:04:46, dgkoch wrote:
> > On 2011/08/08 05:42:23, gmanchromium wrote:
> > >
> >
>
http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
> > > File extensions/ANGLE_texture_compression_dxt.txt (right):
> > > 
> > >
> >
>
http://codereview.appspot.com/4809068/diff/10001/extensions/ANGLE_texture_com...
> > > extensions/ANGLE_texture_compression_dxt.txt:12: Contributors to
> > > ANGLE_texture_compression_dxt
> > > On 2011/08/05 05:36:24, dgkoch wrote:
> > > > normally this would be used to refer to authors of an extension this is
> > based
> > > > on..
> > > 
> > > I'm not sure you what you mean. Should I copy the contributors list from
> > > EXT_texture_compression_dxt1? I'm not comfortable adding names of people I
> > don't
> > > know. I kind of assuming referring to the other extensions was enough. No?
> > > 
> > 
> > It was self-referential.  It should have said "Contributors to
> > EXT_texture_compression_dxt1"  
> > Instead it say the contributors to *this* extension are the contributors to
> this
> > extension.
> 
> Ok, so is it correct now? The current version is here
> 
>
http://codereview.appspot.com/4809068/diff/15001/extensions/ANGLE_texture_com...
> 
> It seems to match this one
> 
>
http://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_type_2_10_10_...
> 
> Otherwise I can just remove the 'contributors' section. Most of the other
> extension specs don't have one. I just copied some other spec's format and
> happened to pick one with a 'contributors' section.

It's fine how you  have it now.  It's good practice to include contributors.