complete glCopyImageSubData

[qiankanglai]

Description

When serializing glCopyImageSubData, we could try glGetTexImage when srcData.compressedData doesn't exist.
Also I reformat here to make code more readable.

TODO: need find a elegant way to skip copying here

srcCd = srcData.compressedData[srcLevel];

[baldurk]

I think this looks good though I've left a couple of small comments.

I must admit I'm shocked that any Android driver actually successfully implements a feature like this, but it seems to be working for you since I think you are the one who implemented this compressed copy in this function in the first place!

[baldurk]

You can change srcCd to be a pointer like bytebuf *srcCd, and have it either point to this compressed data, or to a new temporary variable bytebuf tempReadback or similar.

[baldurk]

Is it worth adding an error message if the source texture is not 2D or 2D array? it could be a cubemap or 3D texture as well, copying a single slice.

[qiankanglai]

I'm a bit unsure about supporting 3D or Cubemap here (just remove line 1405 and use TextureBinding), which is not used in the game.

[baldurk]

Yes that's totally fine to not support it, I just mean to add an error message for that case. It looks like at the moment the call will be silently dropped.

[baldurk]

I think you also need to push & pop the pixel pack state here too, to ensure tightly packed data that doesn't go to a buffer.

[baldurk]

I don't think this check is accurate anymore, since unless I'm mistaken the dimensions of the uncompressed texture will be off by a factor of the block size from the compressed texture - each uncompressed 1x1 texel maps to a compressed 4x4 block.

[qiankanglai]

It could be like 1x1 RGBA32UI -> 4x4 ETC2, which is in fact the case we use (virtual texture like and runtime gpu compression). So I just leave this fast code path when perform full texture copy of the same size.

[baldurk]

Yeh that's what I mean. In fact I didn't notice that the check had been changed, and it's now comparing srcLevelWidth to srcWidth which doesn't seem correct. I think the check for a full copy should be something like comparing src dimensions divided by src block size to dst dimensions divided by dst block size. Then it does the same kind of check as before (where it just compared src to dst) but with handling for compressed textures only being on one side.

[qiankanglai]

This fast path is ensured under these situations:

• copy full srcTex (srcX == 0 && srcY == 0 && srcZ == 0 and srcLevelWidth == srcWidth && srcLevelHeight == srcHeight && srcLevelDepth == srcDepth
• copy full dstTex (dstX == 0 && dstY == 0 && dstZ == 0 and srcSize == dstSize)

src dimensions divided by src block size to dst dimensions divided by dst block size

I didn't check way because it is implied by srcSize == dstSize (Please correct me if I misunderstand it)

[baldurk]

I think it is correct, but I had to think quite carefully to be sure there wasn't a scenario where this would break. Normally the size could be same for different dimensions, but together with the constraint that the whole source texture is copied there's no valid way to copy to anything but the same dimension.

As I say though, I did have to think through it carefully as it's not trivially obvious to me and my first instinct was that it could still break in some cases. For that reason I would still rather do an explicit check that the source dimension is equal to the destination dimension after normalising for blocks/texels. This is not a high performance case so there is no need to minimise the number of comparisons, and keeping the same "src == dest" logic as before is clearer to read.

[qiankanglai]

Just a screenshot of capture&replay on my device(ETC2 on XiaoMi13). We rely on this API to copy into compressed formats, and tests show many devices nowdays are able to work in this way.

[baldurk]

There is a helper PixelPackState which does this push/pop for you, as well as a helper ResetPixelPackState, since alignment is not the only pack value that could interfere here.