Made GL_RGB textures map to D3DFMT_X8R8G8B8....

Made GL_RGB textures map to D3DFMT_X8R8G8B8.

I suspect there is a subtle reason why this is the wrong thing to do.

The issue I am trying to address is glBlitFramebufferANGLE generating an error when bliting from a GL_RGB8_OES renderbuffer to a GL_RGB texture bound to the read framebuffer and draw framebuffer respectively. It verifies that both surfaces have the same underlying D3DFMT but GL_RGB8_OES renderbuffers are backed by X8R8G8B8 and GL_RGB textures were previously backed by A8R8G8B8.

Since textures cannot as far as I know be created GL_RGB8_OES, it would be useful to have a way to blit from a GL_RGB8_OES renderbuffer to a GL_RGB texture.

[apatrick1, 2010-11-01 23:21:55]

Hi Daniel,

Can I run this idea by you?

Thanks,

Al

[dgkoch, 2010-11-09 13:24:32]

haven't forgotten about this.  

At an initial glance this seems reasonable, but I'm concerned about what other
areas might be impacted by this change (eg CopyTex{Sub}Image, etc).   Nicolas
will take a closer look at those interactions

[nicolas, 2010-11-12 17:35:51]

I verified that D3DFMT_X8R8G8B8 textures are handled correctly. The tests to
select it may need to be revisited though (comments added to patch review).

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

http://codereview.appspot.com/2822041/diff/1/src/libGLESv2/Texture.cpp#newcod...
src/libGLESv2/Texture.cpp:233: if (format == GL_RGBA || format == GL_ALPHA)
I believe D3DFMT_X8R8G8B8 should only be selected for type GL_UNSIGNED_BYTE and
formats GL_RGB and GL_RGB8_OES. In case an unknown format is encountered, it
should fall back to D3DFMT_A8R8G8B8.

[apatrick1, 2010-11-15 21:10:39]

Thanks,

Al

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

http://codereview.appspot.com/2822041/diff/1/src/libGLESv2/Texture.cpp#newcod...
src/libGLESv2/Texture.cpp:233: if (format == GL_RGBA || format == GL_ALPHA)
I didn't do it for RGB8_OES because I think that is only valid as a renderbuffer
format.

[dgkoch, 2010-11-16 02:00:54]

LGTM

Daniel

[nicolas, 2010-11-16 07:04:15]

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

http://codereview.appspot.com/2822041/diff/1/src/libGLESv2/Texture.cpp#newcod...
src/libGLESv2/Texture.cpp:233: if (format == GL_RGBA || format == GL_ALPHA)
Indeed, but that still leaves the (minor) issue that when a new texture format
is added, it may go unnoticed that D3DFMT_X8R8G8B8 is selected instead of
D3DFMT_A8R8G8B8. Ideally I think there should be an assert for unknown formats,
and the safest fallback is D3DFMT_A8R8G8B8.

[dgkoch, 2010-11-16 13:27:33]

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

http://codereview.appspot.com/2822041/diff/1/src/libGLESv2/Texture.cpp#newcod...
src/libGLESv2/Texture.cpp:233: if (format == GL_RGBA || format == GL_ALPHA)
On 2010/11/16 07:04:16, nicolas wrote:
> Indeed, but that still leaves the (minor) issue that when a new texture format
> is added, it may go unnoticed that D3DFMT_X8R8G8B8 is selected instead of
> D3DFMT_A8R8G8B8. Ideally I think there should be an assert for unknown
formats,
> and the safest fallback is D3DFMT_A8R8G8B8.

Nicolas: look at the new patch.  It's handled as you wanted there.

[nicolas, 2010-11-16 13:37:36]

Ah, sorry, somehow I was looking at the first patch again...

The new patch looks fine to me!