BinomialLLC / basis_universal

Basis Universal GPU Texture Codec
Apache License 2.0
2.66k stars 260 forks source link

Alpha artifacts in BC7 output #278

Open kg opened 2 years ago

kg commented 2 years ago

When I use BC7 in my test application, it seems like the basis transcoder is generating semitransparency artifacts in the alpha channel that aren't in the source texture or in the DXT5 transcode. I pulled the latest rev of the repository, compiled basisu from scratch, and re-generated all my ktx2/uastc files. The RGB channels look fine to my eye, I think only A is messed up. Comparison screenshots (the last one is BC7 and obviously broken) (EDIT: Also note that I used renderdoc's range slider to make the artifacts more obvious, it's alpha values in the like 230-250 range that should be 255):

source-rgba basis-dxt5 basis-bc7

Also, here are two ktx2 uastc files that exhibit this problem when transcoded. basis-bc7-alpha-bug.zip

richgel999 commented 2 years ago

Got it - investigating. Could you please provide the source image too?

It is possible (and known behavior) for the transcoder to output BC7 textures with alpha values=254 on opaque blocks, due to the way it uses some of the alpha modes and p-bits. It appears (just by examining your provided screenshot in Paint Shop Pro, and getting a histogram) that's the issue you're seeing. I believe I can provide either a flag or modify the transcoder to not do this. So far it's not been an issue, but I can see it causing issues. When it does happen, it'll only be like 1 LSB in alpha.

image

Thanks! -Rich

kg commented 2 years ago

red This is the source image, but I transcode it to TGA before feeding it to basisu to work around deficiencies in PNG loading. I'll try to generate that tga and upload it when I get a chance (the compressor deletes it when it's done)

richgel999 commented 2 years ago

This is the source image, but I transcode it to TGA before feeding it to basisu to work around deficiencies in PNG loading. Curious - what PNG issues have you encountered? We want to fix them.

PNG loading is currently handled using lodepng, but we're thinking of switching to Wuffs. We've upgraded to the latest lodepng version in v1.16 (the next major release of basisu), but we're in a holding pattern on that release until given approval.

kg commented 2 years ago

It's less issues and more missing features necessary to handle certain things right - I mentioned some of it in https://github.com/BinomialLLC/basis_universal/issues/249. I have to do certain things to ensure that alpha and sRGB/linear stuff is handled correctly and the default behavior for PNGs created by various image software is not "right" in this case, though it is probably textbook correct. So I do various things to make sure the rgb and a channels have the content I want, then save that to a tga and compress it with basisu. At runtime I take the dxt textures and multiply the rgb on the fly, since if I encoded them premultiplied the discontinuities between RGB and A would produce horrible artifacts.

richgel999 commented 2 years ago

OK - this is definitely the artifact I thought it was. The UASTC->BC7 transcoder will sometimes output mode 6 blocks that have p-bits that are 0 on opaque blocks. BC7 mode 6 only has 7 bit components, so when the p-bit is 0 you'll get alpha values of 254 instead of 255. This is a known issue with BC7. It results in less RGB error overall, which is why it does that. (Our offline BC7 encoders do this too.)

Is it possible for you to work around this? When it happens, you'll either get alpha values of 254 or 255, instead of 255 in alpha blocks. I will see if it's possible for us to provide a workaround, but it may be tricky.

image

image

richgel999 commented 2 years ago

If you want to be adventurous and experiment - you can slam both p-bits to 1 at line 13083 in function transcode_uastc_to_bc7() in transcoder/basisu_transcoder.cpp. The problem goes away, however overall RGB error will go up. (However I doubt the added error would be visually noticeable.)

It should be possible for us to add an encoder/transcoder option that forbids the system from utilizing the p-bits in this way on opaque blocks. We're going to rev the UASTC encoder this quarter to support ASTC sRGB sampling better, which would be the perfect time to do that.

image

kg commented 2 years ago

If the range on the artifacts will be known and easily detected (253-254 for example) I can probably just bias those in my pixel shader at the cost of a couple opcodes. Awkward but reasonable. It sounds like there isn't a good alternative. I'm guessing I'll also need to adjust my RGB values in some way to fix it? Do I want to multiply all values by (255 / 254)?

Is this in the docs somewhere and I missed it?

EDIT: If you are already considering making changes, though, I would advise at least making it the default to eat the slight RGB issues in favor of producing alpha without this artifact. The alpha artifact is comparatively severe in terms of consequences and not everyone will be able to modify all their shaders to fix it. People who have issues with the resulting RGB artifacts could opt in to the current behavior, I think?

richgel999 commented 2 years ago

It sounds like there isn't a good alternative. I'm guessing I'll also need to adjust my RGB values in some way to fix it? Do I want to multiply all values by (255 / 254)?

Yes, I can modify the system to not do this, or make it an option. There's no need to adjust RGB values. On fully opaque blocks, the transcoder will still use 0/1-pbits on mode 6 and you'll sometimes get alpha values of 254.

The alpha artifact is comparatively severe in terms of consequences

I'll see what I can do. It's totally fixable, but we want to make sure all of our users are happy because RGB PSNR is typically #1. Not utilizing these p-bits to minimize RGB error will make others less happy. (It's a whack a mole game for us.)

richgel999 commented 2 years ago

Just compressed your PNG file with AMD Compressonator, using its built-in BC7 encoder. It has the same problem: some alpha values are 254 because it utilizes p-bits too:

image

"the last one is BC7 and obviously broken"

So does this mean AMD Compressonator is broken too? The generated DDS file is attached. So it's clear, you are asking us to do something that even the highest quality offline BC7 encoders don't do. Note that we can do something, it's just going to take us some time. :)

image

bc7_artifact_PNG_BC7_1.zip

kg commented 2 years ago

Well, if everyone has the same problem - it sounds like a limitation in the BC7 format - then it's not really your obligation to fix it! Certainly appreciate it if you do, but if this is just an artifact inherent to BC7 I'll have to change my shaders in order to use BC7. The workaround is at least fairly simple so I don't think it's a big burden to do it. It's interesting that compressonator produces artifacts in different places.

You're right that RGB error is bad, but doesn't this produce the equivalent of RGB error at display time since in most cases the RGB values are going to be multiplied by the A channel? I can imagine it being very bad for cases where the RGBA channels aren't color + alpha data, though, because in that case you'd be slightly increasing the accuracy of one channel at the cost of the other three.

kg commented 2 years ago

I updated my shaders to generalize support for handling format quirks like this, and multiplying by (255 / 254) seems to make the problems go away. Considering that other encoders will produce bad data like this, I think it's probably reasonable not to fix it - though I'd certainly love to have a flag that does work around it, since it would make complex shaders that sample textures a lot (like a blur) much cheaper.

richgel999 commented 2 years ago

We'll get a fix in. It's not that hard, but we want to do it right (i.e. not regress anything).

kg commented 2 years ago

I learned today that bc7 can also produce a=1 instead of a=0. Should I be compensating for this too, or does basis's transcoder work in a way that avoids generating erroneous 1s? Thanks!