DiligentGraphics / DiligentCore

A modern cross-platform low-level graphics API
http://diligentgraphics.com/diligent-engine/
Apache License 2.0
594 stars 130 forks source link

OpenGL texture upload segfault (GLES Android) #251

Open cbauernhofer opened 3 years ago

cbauernhofer commented 3 years ago

I've found a bug in your texture upload mechanism for OpenGL on Android. Basically you set the packing alignment to a fixed value of 4 with glPixelStorei(GL_UNPACK_ALIGNMENT, 4);. This can be problematic for 1 or 3 channel textures if the stride of the image is not a multiple of 4 bytes. You should basically check if the stride is a multiple of 8, 4, 2 or 1 and set it accordingly.

I'm not sure if this is GL implementation dependent (as it works on my desktop using OpenGL but not with OpenGL ES on Android). Maybe GL_UNPACK_ROW_LENGTH that you already use is ignored by GLES and therefore it doesn't work on Android. AFAIK GL_UNPACK_ALIGNMENT is used by all implementations.

TheMostDiligent commented 3 years ago

Can you provide a reproducer? GL_UNPACK_ALIGNMENT only matters when unpack buffer is set, e.g. when data is copied from a buffer to a texture, which I don't think what you are doing. When updating texture data from CPU memory, it is irrelevant.

The row stride is set by glPixelStorei(GL_UNPACK_ROW_LENGTH, SubresData.Stride / PixelSize);

cbauernhofer commented 3 years ago

First I thought that I can easily reproduce it by simply changing your Tutorial03_Texturing.cpp to load a problematic texture (basically any 1channel image with a width that is not dividable by 4). But to my surprise it worked on my desktop and on Android.

So I digged deeper and found the reason. In our code we use OpenCV for all image related operations which include loading and saving. In your samples you use other third party libs (libjpeg, libpng etc). After loading a problematic image I had a look at its raw binary size and its row-stride. Here is the reason why it works on your end:

diligent: 1192288
ours    : 1186246
rows: 2014 cols: 589

(1192288 - 1186246) / 2014 = 3 --> adds 3 bytes to every line

when looking at the stride:
diligent: 592
ours    : 589
--> proofs my assumption as 592 is perfectly dividable by 4

Even though you set the row stride by glPixelStorei(GL_UNPACK_ROW_LENGTH, SubresData.Stride / PixelSize);

it still fails on Android (probably also iOS but I don't have a device here) with a segfault in glTexSubImage2D().

2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #00 pc 00000000002a0f30  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!4d0cdaba868e987aa070f5a6b168e2!85da404!+768) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #01 pc 000000000029cbb4  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!6fd1d11959478379873bee344e3720!85da404!+1140) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #02 pc 000000000026d180  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!e9a0267a4c3f12c4fb16e257d3a26e!85da404!+6192) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #03 pc 000000000027189c  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!9c0715a0352375a9ec27cf88ce6933!85da404!+468) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #04 pc 0000000000236898  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!e94336f9c3a8e90238c7c8557996da!85da404!+2776) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #05 pc 0000000000189910  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!0e6b00ab8c4b112f9f6effa6a8b2b5!85da404!+3720) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #06 pc 0000000000186504  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!4ecf3032464df959aad423cba1a73c!85da404!+1364) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #07 pc 00000000001a5164  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!141e50cb152287019aff218176d094!85da404!+268) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #08 pc 00000000001be678  /vendor/lib64/egl/libGLESv2_adreno.so (!!!0000!838e96e6042a39f699090106d8c25f!85da404!+208) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #09 pc 00000000000c95a0  /vendor/lib64/egl/libGLESv2_adreno.so (glTexSubImage2D+144) (BuildId: 3b3a133255381f2f53bde29433a1b4c2)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #10 pc 00000000000b0314  /data/app/~~Ybmy_wmJKz2vKR3QaMhrGg==/sandbox-chSFbiFTtLua33w8zZ6w8Q==/base.apk!libGraphicsEngineOpenGL.so (offset 0x4587000)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #11 pc 00000000000b007c  /data/app/~~Ybmy_wmJKz2vKR3QaMhrGg==/sandbox-chSFbiFTtLua33w8zZ6w8Q==/base.apk!libGraphicsEngineOpenGL.so (offset 0x4587000)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #12 pc 000000000009b16c  /data/app/~~Ybmy_wmJKz2vKR3QaMhrGg==/sandbox-chSFbiFTtLua33w8zZ6w8Q==/base.apk!libGraphicsEngineOpenGL.so (offset 0x4587000)
2021-07-01 12:52:20.368 8398-8398/? A/DEBUG:       #13 pc 000000000008f6f8  /data/app/~~Ybmy_wmJKz2vKR3QaMhrGg==/sandbox-chSFbiFTtLua33w8zZ6w8Q==/base.apk!libGraphicsEngineOpenGL.so (offset 0x4587000)

As I've had this issue already in the past on mobile platforms I knew that it has to be the GL_UNPACK_ALIGNMENT. So changing it to 1 with glPixelStorei(GL_UNPACK_ALIGNMENT, 1); solves the problem.

Still, this issue is not present on the desktop - that's why I assume that GL and GLES behaves differently in this regard.

cbauernhofer commented 3 years ago

One thing I should have added to my previous answer is the explanation why it's not enough in this case to set the GL_UNPACK_ROW_LENGTH. Looking at the definition (https://www.khronos.org/registry/OpenGL-Refpages/es3.0/html/glPixelStorei.xhtml) and my example we have:

s = 1 (8-bit --> 1 byte)
a = 4 (set by you)
n = 1 (single channel)

since s < a:
k = a/s * ⌈(snl)/a⌉
k = 4/1 * ⌈(1*1*589)/4⌉ = 4 * ⌈147.25⌉ = 4 * 148 = 592

So the row-length is set to 592 bytes but the image has a stride of 589 bytes and therefore it makes totally sense that it crashes with a segfault.

By setting a = 1 the other if in the specification gets active and therefore k is computed as:

k = n * l
k = 1 * 589 = 589

The thing I don't understand is why it doesn't happen on the desktop as well.

TheMostDiligent commented 3 years ago

Row stride must be aligned by 4 bytes to be consistent with other backends. A debug check that validates parameters is missing, I will add it.