Skip to content

supporting full glCopyImageSubData #2129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

qiankanglai
Copy link
Contributor

Description

Current version's glCopyImageSubData only supports full texture copy (with the same size). Copy with region raises assertions like this.
image

I implemented a line by line version when sub region is provided, and fast copy path still works with full copy.

Here is a example frame with two continuous copies (512x512 texture -> 1024x1024 texture)
image
image

BTW GetCompressedBlockSize is a bit ugly compared with GetCompressedByteSize. Any suggestion?

@qiankanglai qiankanglai force-pushed the glCopyImageSubData branch 2 times, most recently from 2294b45 to 1173cfd Compare December 24, 2020 09:06
Copy link
Owner

@baldurk baldurk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, I've left review notes about the things I'd like you to change but overall I think this is fine.

You could simplify GetCompressedBlockSize a bit by e.g. returning 4,4 block size as default and skipping all the 4x4 format types, since you already have an early-out for non-compressed formats. I don't think that's necessary though and it's fine to have the function be explicit like this.

RDCASSERT(srcWidth % std::get<0>(srcBlockSize) == 0);
RDCASSERT(srcWidth % std::get<0>(dstBlockSize) == 0);
RDCASSERT(srcHeight % std::get<1>(srcBlockSize) == 0);
RDCASSERT(srcHeight % std::get<1>(dstBlockSize) == 0);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw glCopyImageSubData also allows copying between uncompressed and compressed textures, as long as the uncompressed texel size in bytes and compressed block size in bytes are the same. It's fine if you don't want to support this (after all that was the reason that sub-image copies was never implemented before), but I'd like you to add a check for this case and output an explicit warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a typo in fact. It should be like RDCASSERT(dstWidth % std::get<0>(dstBlockSize) == 0);
In fact I'm working with compression ETC2 on GPU, which relies on copying between uncompressed and compressed textures. And this is the reason why I worked this patch out.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no dstWidth though at the moment, the function only takes a single width/height because it doesn't do any scaling. In the case of copying between compressed and uncompressed the destination width/height either gets divided by or multiplied by the block size depending on the order to get an implicit dstWidth / dstHeight.

If you do intend to support copies between compressed and uncompressed then that will require more work. For example the current code checks only that the source texture is compressed and goes into this path. If the destination texture is compressed and the source is not, this code won't execute at all. Also uncompressed textures do not have entries in compressedData - since they are not compressed! So you will need to do a GPU readback of the uncompressed source texture data temporarily to be able to then copy into a compressed texture.

@qiankanglai
Copy link
Contributor Author

Got the feedback, I'll take the details and refactor them in weekend 😃

@qiankanglai
Copy link
Contributor Author

Hi, I've included the suggestions above.

For supporting copies between compressed and uncompressed, I'll open another PR later since I think it would be more clear to avoid modifying too much in one commit. (That's why I just leave RDCASSERT(srcHeight % srcBlockSize.second == 0) temporarily here unchanged)

@qiankanglai qiankanglai force-pushed the glCopyImageSubData branch 3 times, most recently from cb5f97e to 6d09832 Compare December 27, 2020 08:04
@qiankanglai qiankanglai requested a review from baldurk January 4, 2021 01:33
Copy link
Owner

@baldurk baldurk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I was on holiday for the end of the year until today. I've left a couple of minor notes but otherwise I think this looks good.

@qiankanglai
Copy link
Contributor Author

Hi, I've improved dimension related codes with the help from https://github.com/baldurk/renderdoc/blob/v1.x/renderdoc/driver/gl/gl_driver.cpp#L2880 but not sure whether it's enough. Thanks

@qiankanglai qiankanglai requested a review from baldurk January 5, 2021 13:41
@baldurk baldurk merged commit 47c7b38 into baldurk:v1.x Jan 5, 2021
@qiankanglai qiankanglai deleted the glCopyImageSubData branch January 6, 2021 01:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants