xuxiandi / angleproject

Automatically exported from code.google.com/p/angleproject
Other
0 stars 0 forks source link

glCompressedTexSubImage2D crashes #237

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
The following snippet of code crashes for me every time.

static char dummy_data[51200];
GLuint texture;
glGenTextures(1, &texture);
glBindTexture(GL_TEXTURE_2D, texture);
glCompressedTexImage2D(GL_TEXTURE_2D, 0, GL_COMPRESSED_RGB_S3TC_DXT1_EXT, 1024, 
1024, 0, 524288, 0);
glCompressedTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 320, 320, 
GL_COMPRESSED_RGB_S3TC_DXT1_EXT, 51200, dummy_data);

The crash is an access violation in FlipCopyDXT1BlockFull coming from 
loadDXT1ImageData.

It seems to me that the address calculation in loadDXT1ImageData is part of the 
problem.
unsigned int *dest = reinterpret_cast<unsigned int*>(static_cast<unsigned 
char*>(output) + (y + yoffset) * outputPitch + xoffset * 8);

'output' points to the locked area of the texture from subImageCompressed, so 
why is the xoffset and yoffset added?
It seems the offset is effectively added twice, and the code should simply have 
been:

unsigned int *dest = reinterpret_cast<unsigned int*>(static_cast<unsigned 
char*>(output) + y * outputPitch);

Also, shouldn't the updateRegion be flipped in y to be consistent with the 
flipping of the dxt data and the yoffset?
From:
updateRegion.bottom = yoffset + height; updateRegion.top = yoffset;
To:
updateRegion.top = image->height - (yoffset + height); updateRegion.bottom = 
image->height - yoffset;

The code for dxt3 and dxt5 textures follow the same pattern.

Maybe there is something I'm missing, but applying these two changes makes 
glCompressedTexSubImage2D behave as I expect :)

Looking at the code, it seems that you are only supporting po2 textures. As you 
require every mip level to have
a dimensions that are either 1, 2 or a multiple of 4. Is this intentional? d3d9 
is a lot less restrictive and only
requires the top mip to be a multiple of 4.

Original issue reported on code.google.com by stu...@cs.au.dk on 2 Nov 2011 at 9:52

GoogleCodeExporter commented 9 years ago
I'm currently rewriting a significant portion of the texture code so I'll make 
sure this gets fixed as well...

Original comment by nicolas....@gmail.com on 2 Nov 2011 at 8:48

GoogleCodeExporter commented 9 years ago
Have you started on this? If not I'm happy to take a look.

Original comment by g...@google.com on 12 Nov 2011 at 7:13

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Sure, go ahead Gregg.

Original comment by dan...@transgaming.com on 12 Nov 2011 at 2:19

GoogleCodeExporter commented 9 years ago
so I finally got started on this but I'm running into my limit of DX knowledge.

If I understand OpenGL ES 2.0 correctly you can make any size DXT texture, 1x1, 
2x2, 3x4, 7x9 etc. You just have to give it whole blocks of data

But, DirectX seems to have the restriction that sizes must multiple's of 4. I'm 
guessing if ask for a 4x4 with 3 levels it will give you 3 levels, each 4x4 and 
only use the top left corner of each level but otherwise, as far as I can tell 
there is no way to create a 2x2 or 1x1 and there is no way to create odd sizes.

Note: I don't know where to look for docs on this. I just tried a 15x15 texture 
and device->CreateTexture(...) in TextureStorage2D::TextureStorage2D around 
line 1649 in Texture.cpp failed where as 16x16 works.

I'm assuming there is no other way to tell directX "hey, make a 16x16 size 
texture but just use 15x15 of it)

Can anyone verify that's the case?

Original comment by g...@chromium.org on 16 Dec 2011 at 5:07

GoogleCodeExporter commented 9 years ago
Yes. It seems to be the case. I found the following snippet in the directx sdk 
documentation:
Direct3D 9 Graphics -> Reference for Direct3D 9 -> Direct3D Enumerations -> 
D3DFORMAT

"The runtime will not allow an application to create a surface using a DXTn 
format unless the surface dimensions are multiples of 4. This applies to 
offscreen-plain surfaces, render targets, 2D textures, cube textures, and 
volume textures."

Original comment by stu...@cs.au.dk on 16 Dec 2011 at 6:43

GoogleCodeExporter commented 9 years ago
Well the tricky bit is that you can't create a texture where the *top* level is 
not a multiple of 4. However if you bump the size up 1 or 2 levels (as 
necessary) to make it a multiple of 4, you can then use the smaller levels with 
a non-mult4 size (and just set D3DSAMP_MAXMIPLEVEL at run-time to ensure extra 
top levels aren't used)

For the 15x15 texture example you could instead create a 60x60 texture with 3 
levels and use the 3rd level which would be 15x15.

We have code to do this in Image::createSurface since this is also needed for 
the 1x1 and 2x2 systemmem surfaces.

We prototyped this when we implemented DXT textures in the first place, but we 
couldn't actually find any dxt compression tools that would allow us to create 
non-multiple of 4 textures so we couldn't test it.

Greg: do you actually have a 15x15 DXT image, or you were just trying it out as 
a pathological case?

Original comment by dan...@transgaming.com on 16 Dec 2011 at 9:26

GoogleCodeExporter commented 9 years ago
The problem with this approach is that you can't support all non-mult4 textures 
that are greater than MAX_TEXTURE_SIZE/4.

Original comment by dan...@transgaming.com on 16 Dec 2011 at 9:35

GoogleCodeExporter commented 9 years ago
You don't generate a 15x15 DXT. You Generate a 16x16 and then at runtime tell 
it to only use 15x15 of it.

I'm discussing on the WebGL side if it's okay just to include the DirectX 
limits in WebGL. Given that DirectX is the number 1 API on desktops it's 
unlikely there is much if any content that doesn't meet these rules.

We'll have to expose these as GL_ANGLE_texture_compression_dxt1 etc or 
something but I'm sure that's fine.

Original comment by g...@chromium.org on 16 Dec 2011 at 10:22

GoogleCodeExporter commented 9 years ago
one suggestion I got back, if the texture doesn't fit the DX limits uncompress 
it behind the scenes.  What do you think?

Original comment by g...@chromium.org on 4 Jan 2012 at 9:03

GoogleCodeExporter commented 9 years ago
The problem with decompressing it in ANGLE is that compressed vs non-compressed 
textures tend to be handled a little differently in places.  In particular 
there are various things you *can't* do with compressed textures that we'd have 
to make sure still held over with uncompressed-compressed textures.  You also 
need to have a software decompressor (which has other implications).

Original comment by dan...@transgaming.com on 4 Jan 2012 at 9:34

GoogleCodeExporter commented 9 years ago
Even if we can't support all non-mult4 textures that are greater than 
MAX_TEXTURE_SIZE/4 with this method (doubling the size until it's a multiple of 
4), we can still support a lot more textures, and that's helpful for NaCl. I'll 
try and fix this to at least that extent.

Original comment by jbauman@chromium.org on 4 Jun 2012 at 10:59

GoogleCodeExporter commented 9 years ago
currently we are only guaranteeing support for compressed textures that are a 
multiple of 4.  And, the smallest texture you can create is 4x4 (although that 
means you can provide a 2x2 and 1x1 mips)

Original comment by g...@chromium.org on 5 Jun 2012 at 6:49

GoogleCodeExporter commented 9 years ago
John: are there really that many DXT textures that are hitting this case in 
NaCl?

Original comment by dan...@transgaming.com on 7 Jun 2012 at 3:31

GoogleCodeExporter commented 9 years ago
John says this is fixed for M22.

John is going to request a merge for this to M21. This bug is triggered by a 
feature which is new in Flash 11.3, which isn't used by many developers yet, 
but will at some point. (The bug causes the content to not work, AFAIK.) We'd 
like to merge the fix to M21 if it's not too complex, otherwise we will live 
with M22.

Can someone who has edit access on this bug cc kareng@? (I can't)

Original comment by jeffr...@google.com on 16 Jul 2012 at 8:20

GoogleCodeExporter commented 9 years ago
Ignore comment #15, I think I pasted in the wrong bug

Original comment by jeffr...@google.com on 16 Jul 2012 at 10:45

GoogleCodeExporter commented 9 years ago

Original comment by c...@chromium.org on 7 Dec 2013 at 4:08

GoogleCodeExporter commented 9 years ago
Has been fixed since M22.

Original comment by c...@chromium.org on 21 May 2014 at 4:26