Skip to content
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

Astc normal maps #493

Merged
merged 36 commits into from
Feb 17, 2022
Merged

Astc normal maps #493

merged 36 commits into from
Feb 17, 2022

Conversation

wasimabbas-arm
Copy link
Contributor

First attempt at making 2-component normal mapping working.

@wasimabbas-arm wasimabbas-arm changed the title [WIP] Astc normal maps Astc normal maps Oct 19, 2021
Copy link
Collaborator

@MarkCallow MarkCallow left a comment

Choose a reason for hiding this comment

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

Please modify the documentation for normal mode so that it says something along the lines of

If the input file has 3 or 4 components, a 2 component normal map will first be generated. The 2-component normal map will be encoded with X in the RGB channels and Y in the alpha channel of the compressed texture. The encoding is optimized for normal maps by such things as disabling RDO, if RDO is available, and tuning for angular error rather than PSNR.

This allows us to support 2-component inputs without needing an option to invoke the 3 to 2 conversion and lets us extend the normal map support to the Basis encoders. The next release of Basis will have optimization for angular error. At some point I plan to add RDO for ASTC to get better results when zstd compression is used.

@wasimabbas-arm
Copy link
Contributor Author

wasimabbas-arm commented Nov 8, 2021

@MarkCallow I have now started another PR for this because my fork has changed, but before I create that one and close this one. I would like to resolve the documentation update you requests.

We already have the following regarding normal_mode.

          "  --normal_mode\n"
          "               For ASTC encoder '--encode astc' assumes the input texture is\n"
          "               a three component linear LDR normal map storing unit length\n"
          "               normals as (R=X, G=Y, B=Z). The output will be a two component\n"
          "               X+Y normal map stored as (RGB=X, A=Y), optimized for angular\n"
          "               error instead of simple PSNR. The Z component can be recovered\n"
          "               programmatically in shader code by using the equation:\n\n"
          "                   nml.xy = texture(...).ga;              // Load in [0,1]\n"
          "                   nml.xy = nml.xy * 2.0 - 1.0;           // Unpack to [-1,1]\n"
          "                   nml.z = sqrt(1 - dot(nml.xy, nml.xy)); // Compute Z\n\n"
          "               For ETC1S encoder '--encode etc1s' tunes codec parameters for \n"
          "               better quality on normal maps (no selector RDO, no endpoint RDO).\n"
          "               Only valid for linear textures.\n"

Thinking about your request:

Please modify the documentation for normal mode so that it says something along the lines of

If the input file has 3 or 4 components, a 2 component normal map will first be generated. The 2-component normal map will be encoded with X in the RGB channels and Y in the alpha channel of the compressed texture. The encoding is optimized for normal maps by such things as disabling RDO, if RDO is available, and tuning for angular error rather than PSNR.

I think, all the information is there. You have worded it differently. Is there still a need for wording it like you suggested or shall I leave it as is?

Or did you want me to add this documentation somewhere else?

@MarkCallow
Copy link
Collaborator

The main thing I want to change is the assumption that the input is a 3 component linear normal map. We need to allow both 2 and 3 component inputs. So I'd reword like this:

          "  --normal_mode\n"
          "               If the input texture has three linear components it is assumed to\n"
          "               be a three component linear normal map storing unit length\n"
          "               normals as (R=X, G=Y, B=Z). It will be converted to a two \n"
          "               component X+Y normal map stored as (RGB=X, A=Y) prior to\n"
          "               encoding.  If the input has 2 linear components it is assumed to\n
          "               be an X+Y normal map.
          "               The Z component can be recovered programmatically in shader\n"
          "               code by using the equations:\n\n"
          "                   nml.xy = texture(...).ga;              // Load in [0,1]\n"
          "                   nml.xy = nml.xy * 2.0 - 1.0;           // Unpack to [-1,1]\n"
          "                   nml.z = sqrt(1 - dot(nml.xy, nml.xy)); // Compute Z\n\n"
          "               Encoding is optimized for normal maps. For ASTC encoding,\n"
          "              '--encode astc', the encoder is directed to optimized for angular\n"
          "               error instead of simple PSNR.  For ETC1S encoding, '--encode etc1s',\n"
          "               RDO is disabled (no selector RDO, no endpoint RDO) to provide \n"
          "               better quality on normal maps.\n\n"
          "               Only valid for linear textures with two or three components.\n"

I've struck "LDR" as well. Wouldn't you want do the same things for HDR normal maps?

By the way does it give an error if the input is not linear? The document suggests it will.

We can probably strike the "linear"s that are in line in the text. The last validity statement is sufficient but I didn't notice it until I'd already written them.

@wasimabbas-arm
Copy link
Contributor Author

wasimabbas-arm commented Nov 15, 2021

I've struck "LDR" as well. Wouldn't you want do the same things for HDR normal maps?

Ok makes sense. I don't know much about HDR normal maps though not very common at least.

By the way does it give an error if the input is not linear? The document suggests it will.

It does in basis case, I will add it to astc section as well.
Was just having a look at this and I found that bopts.normalMap = options.normalMode; is called after its checked. Is this a bug?

if (bopts.normalMap && chosenOETF != KHR_DF_TRANSFER_LINEAR) {

Also I am moving this error message outside the if (options.etc1s || options.bopts.uastc) { and {else if (options.astc) { without checking for either of those options. Is that fine or should I be checking for all of these. Are there any other cases where a non-linear normal map would be acceptable? Again not very common though and probably wrong in some ways.

We can probably strike the "linear"s that are in line in the text. The last validity statement is sufficient but I didn't notice it until I'd already written them.

I would move it first statement. that was its more useful and if thats all what people read it will probably be enough without knowing what we do with these different component.

@MarkCallow
Copy link
Collaborator

Was just having a look at this and I found that bopts.normalMap = options.normalMode; is called after its checked. Is this a bug?

Yes, looks like a bug. Well caught! Please fix it as part of this PR.

utils/scapp.h Outdated Show resolved Hide resolved
@wasimabbas-arm
Copy link
Contributor Author

Are there any other cases where a non-linear normal map would be acceptable? Again not very common though and probably wrong in some ways.

Tests failed because the source reference image tests/srcimages/Iron_Bars/Iron_Bars_001_normal.jpg I am using sRGB 😮‍💨

@MarkCallow
Copy link
Collaborator

@wasimabbas-arm my mail server is having problems so my reply to your e-mailed question has not been sent. Here it is. You'll get this in e-mail too once the server comes back up.

toktx currently assumes .jpg files are sRGB because for the most part they are as the underlying YUV space is non-linear. At some point it should be updated to understand the various APPn tags in which color space information can be stored. toktx does not support ICC profiles. I was hoping to address both of these things by using OpenImageIO but I’m not sure that is going to work out. You can override the file’s OETF by using —assign_oetf so —assign_oetf linear should do the trick. But it is not a good idea to use .jpg for normal maps so we should try to find a .png version of Iron_Bars. Please see if you can find one.

@wasimabbas-arm
Copy link
Contributor Author

wasimabbas-arm commented Nov 17, 2021

toktx currently assumes .jpg files are sRGB because for the most part they are as the underlying YUV space is non-linear. At some point it should be updated to understand the various APPn tags in which color space information can be stored. toktx does not support ICC profiles. I was hoping to address both of these things by using OpenImageIO but I’m not sure that is going to work out. You can override the file’s OETF by using —assign_oetf so —assign_oetf linear should do the trick. But it is not a good idea to use .jpg for normal maps so we should try to find a .png version of Iron_Bars. Please see if you can find one.

Same errors with .png as well. I just saved the .jpg as .png. Is this not going to work? The following image show how it looks like in photoshop where it says "Untagged RGB" so I am assuming I have a .png in linear color space. Either my understanding of colorspace reported by photoshop is wrong or something else I am missing.

Screenshot 2021-11-17 at 12 06 52

Following up from the email the messages I am getting this time are:

toktx: --normal_map specified but input file(s) are not linear.
ASTC transfer function is (1=sRGB, 0=Linear) 1

Also same result with Adobe RGB (1998) and Apple RGB

Here is the image in .png if you would like to give it a go yourself.
Iron_Bars_001_normal

@MarkCallow
Copy link
Collaborator

The first image (the one where you said you converted the .jpg to a .png) has an ICC profile, which as I said are not yet supported. I do not know what is in the profile.

The second image has no color space information at all. In that case toktx assumes sRGB in common with many other software.

I suspect the first image is actually the Adobe RGB or Apple RGB image and the second one the "untagged" image.

You need to specify linear (may be you have to say gamma = 1.0) for the output when converting from the JPG image. But it is better to try to find an original. By the time it has been encoded to .jpg the damage to the original normal data has been done.

In the meantime you can use the --assign-oetf linear option to toktx to tell it to ignore any OETF information in the input file and assume it is linear. Or you can use --convert-oetf linear which tells it to convert from the file's OETF to linear.

@wasimabbas-arm
Copy link
Contributor Author

The first image (the one where you said you converted the .jpg to a .png) has an ICC profile, which as I said are not yet supported. I do not know what is in the profile.

I only have one image attached to the comment the .png. The first image is a screenshot from photoshop only to show its "Untagged RGB" its not to be used for testing.

The second image has no color space information at all.

This is the actual .png I created I really need some help from an artist here it seems. I think for now I will go with your advice --assign-oetf linear

@wasimabbas-arm
Copy link
Contributor Author

So looks like the tests pass now but Appveyor 1h limit is messing up some of them.

@MarkCallow
Copy link
Collaborator

MarkCallow commented Nov 27, 2021

So looks like the tests pass now but Appveyor 1h limit is messing up some of them.

They have passed now. I don't know what problem you were seeing. The limit for this repo has been set at 2 hours for a few weeks now. Perhaps the new limit doesn't apply to PR builds.

I need to verify the requested changes were made. I've forgotten what they were so I'll have to review the conversations and changes. It will likely be a couple of days before I can get to it.

If you find a non-JPEG normal map image, please let me know.

utils/scapp.h Outdated Show resolved Hide resolved
utils/scapp.h Outdated Show resolved Hide resolved
utils/scapp.h Outdated Show resolved Hide resolved
@MarkCallow
Copy link
Collaborator

@wasimabbas-arm I spotted a few typos in the documentation. Please fix them. Also the doxygen/html documentation for --normal_mode at line 304 needs to be updated to match the changes for the help documentation in 17799dd

I have received from the person who created Iron_Bars a PNG version but it is 4k x 4k. It needs resampling as it is too large to include in the tests (56Mb). Resampling normal maps requires renormalizing the normals after filtering. I need to find a tool to resample and renormalize this PNG to avoid adding 56Mb to tests/srcimages. Can you do that with Photoshop?

This also flags up the need to implement renormalizing when generating mipmaps for normal maps. There is a TODO for that at line 1240 in toktx.cc. I want to implement it and set the renormalize option when --normal_mode is specified. It should be part of this PR. Are you up for doing that? The actual renormalize function is not too complex. You can find one in basisu_enc.h. A template version of that needs to be added to image.hpp in toktx.

tools/toktx/toktx.cc Outdated Show resolved Hide resolved
@wasimabbas-arm
Copy link
Contributor Author

wasimabbas-arm commented Nov 29, 2021

@wasimabbas-arm I spotted a few typos in the documentation. Please fix them. Also the doxygen/html documentation for --normal_mode at line 304 needs to be updated to match the changes for the help documentation in 17799dd

I think I have fixed everything you have raised.

Can you do that with Photoshop?

I have photoshop and can do stuff with it but I don't know if it can do that with normal maps it also adds ICC profiles and what not that toktx didn't like. I asked our artist to understand better if this is possible. I think its not straight forward within Photoshop. We might have more luck with xNormal or Crazy Bump but I am not familiar with those. Also I did some manual investigations of the normal map we have, its pixels are not normalized normals either. So we are starting with unnormalized data. Usually people would normalize normals when they read it in shaders.

Are you up for doing that?

I don't mind but I am in training this whole week so will have to wait. Unless we can merge this PR and do that next.

utils/scapp.h Outdated Show resolved Hide resolved
@MarkCallow
Copy link
Collaborator

MarkCallow commented Feb 8, 2022

I've looked at the DFD channel id problem. I see the cause and I have a fix in progress.

@MarkCallow
Copy link
Collaborator

MarkCallow commented Feb 9, 2022

Here is a fixed basis_encode.cpp. @wasimabbas-arm please add it to your branch for this PR. (I had to zip it to be able to upload it here).

basis_encode.cpp.zip

The problem was that toktx is setting the inputSwizzle and the code was taking an externally set rrrg swizzle to indicate a luminance texture. It now only does that if normalMap is not set as we need to support users setting the swizzle themselves.

There is no actual need to set rrrg for the basis_encode.cpp as it sets that, when normalMap is set and inputSwizzle is not. We may want to do the same in the ASTC encoder.

The commit message should read

Allow inputSwizzle to be set for normal maps.

Generalize detection of luminance textures.
Do not use number of input components to determine how to rewrite DFDs.

You will need to regenerate the reference images. 3 will be changed after the rewrite: etc1s_Iron_Bars_001_normal.ktx2, uastc_Iron_Bars_001_normal.ktx2 and luminance_reference_uastc.ktx2. The last is a luminance texture so its DFD should be, and now is, KHR_DF_CHANNEL_UASTC_RGB. It was KHR_DF_CHANNEL_UASTC_RRR.

@wasimabbas-arm
Copy link
Contributor Author

e6d7458, b85772a and 04f9cb5 should answer the inputSwizzle, normal mode swizzle selection in encoder and gold images comment.

lib/astc_encode.cpp Outdated Show resolved Hide resolved
@MarkCallow
Copy link
Collaborator

@wasimabbas-arm, It looks like we may be suffering a case of non-determinism in the ETC1S encoder across platforms. Did you generate the golden image on macOS? I seem to recall that in some previous work you made a way to retrieve failing images from the CI build. Please remind me how that worked so I can investigate a failing image.

@wasimabbas-arm
Copy link
Contributor Author

wasimabbas-arm commented Feb 16, 2022

Did you generate the golden image on macOS?

Yes.

@MarkCallow I have had a hit and miss success with artefacts pulling. I remembering having tried all of the following with some success:

https://github.com/KhronosGroup/KTX-Software/blob/master/.travis.yml#L112
https://docs.travis-ci.com/user/uploading-artifacts/
https://www.appveyor.com/docs/how-to/rdp-to-build-worker/

However what have reliably worked was to reproduce the issue on Linux, which I have and managed to reproduce this one as well. For Appveyor I had to remote desktop to the machine from windows machine.

@MarkCallow
Copy link
Collaborator

managed to reproduce this one as well

Please attach the bad image here so I can compare it with the golden image. Have you already compared them?

@wasimabbas-arm
Copy link
Contributor Author

Yes I did compare. You probably missed the email I sent about it. Here it is again.

Trying to work out why the tests are failing both on Linux and Windows. The failing image (toktx-cmp-etc1s_Iron_Bars_001_normal) when compared with the golden image via ktxinfo I am getting the following diff:

-supercompressionGlobalData.byteLength: 6829
+supercompressionGlobalData.byteLength: 6838
 
-Level0.byteOffset: 0x1bed
-Level0.byteLength: 257480
+Level0.byteOffset: 0x1bf6
+Level0.byteLength: 257414
Level0.uncompressedByteLength: 0
-selectorCount: 2668
+selectorCount: 2666
-selectorsByteLength: 5820
-tablesByteLength: 789
+selectorsByteLength: 5828
+tablesByteLength: 790
 
-rgbSliceByteLength: 130493
+rgbSliceByteLength: 130461
-alphaSliceByteLength: 126987
-alphaSliceByteOffset: 0x1fdbd
+alphaSliceByteLength: 126953
+alphaSliceByteOffset: 0x1fd9d

Any ideas why this is? Why is it only on Linux and Windows? The Mac version works is fine and even more interesting is why is the etc1s version only.

Here is the failed test image.
toktx.etc1s_Iron_Bars_001_normal.ktx2.zip

@MarkCallow
Copy link
Collaborator

Any ideas why this is? Why is it only on Linux and Windows? The Mac version works is fine and even more interesting is why is the etc1s version only.

Yes. See BinomialLLC/basis_universal#60. It is an issue with the ETC1S encoder. Mac version works because that is where the golden image was generated. The image from the failed test looks fine so I am sure it is this same issue.

What we did for the other problem cases is only run them on APPLE. See

if(APPLE)
# Run only on macOS until we figure out the BasisLZ/ETC1S compressor non-determinancy.
gencmpktx( alpha_simple_basis alpha_simple_basis.ktx2 ../srcimages/alpha_simple.png "--test --bcmp" "" "" )
gencmpktx( kodim17_basis kodim17_basis.ktx2 ../srcimages/kodim17.png "--test --bcmp" "" "" )
gencmpktx( color_grid_basis color_grid_basis.ktx2 ../srcimages/color_grid.png "--test --bcmp" "" "" )
gencmpktx( 16bit_png_basis camera_camera_BaseColor_basis.ktx2 ../srcimages/camera_camera_BaseColor_16bit.png "--bcmp --test --nowarn" "" "" )
gencmpktx( paletted_png CesiumLogoFlat.ktx2 ../srcimages/CesiumLogoFlat_palette.png "--bcmp --test --nowarn" "" "" )
endif()
and other similar in toktx-tests.cmake.

Feel free to put an if(APPLE) around this test too to resolve this. Once I get Basis 1.16 integrated, which I'm working on now, I'll try the tests again on the other platforms and see if the changes reported in BinomialLLC/basis_universal#60 have had any effect.

@wasimabbas-arm
Copy link
Contributor Author

Any ideas why this is? Why is it only on Linux and Windows? The Mac version works is fine and even more interesting is why is the etc1s version only.

Yes. See BinomialLLC/basis_universal#60. It is an issue with the ETC1S encoder. Mac version works because that is where the golden image was generated. The image from the failed test looks fine so I am sure it is this same issue.

What we did for the other problem cases is only run them on APPLE. See

if(APPLE)
# Run only on macOS until we figure out the BasisLZ/ETC1S compressor non-determinancy.
gencmpktx( alpha_simple_basis alpha_simple_basis.ktx2 ../srcimages/alpha_simple.png "--test --bcmp" "" "" )
gencmpktx( kodim17_basis kodim17_basis.ktx2 ../srcimages/kodim17.png "--test --bcmp" "" "" )
gencmpktx( color_grid_basis color_grid_basis.ktx2 ../srcimages/color_grid.png "--test --bcmp" "" "" )
gencmpktx( 16bit_png_basis camera_camera_BaseColor_basis.ktx2 ../srcimages/camera_camera_BaseColor_16bit.png "--bcmp --test --nowarn" "" "" )
gencmpktx( paletted_png CesiumLogoFlat.ktx2 ../srcimages/CesiumLogoFlat_palette.png "--bcmp --test --nowarn" "" "" )
endif()

and other similar in toktx-tests.cmake.

Feel free to put an if(APPLE) around this test too to resolve this. Once I get Basis 1.16 integrated, which I'm working on now, I'll try the tests again on the other platforms and see if the changes reported in BinomialLLC/basis_universal#60 have had any effect.

Yea I have seen those if(APPLE) I didn't want to just limit it to APPLE before we could confirm what the issue is. Fix 2bba286

@wasimabbas-arm
Copy link
Contributor Author

Ok I think we are good to go. appveyor failed in debug builds due to 1h limit again. But usually for some reason it just clears up after a while or you do some magic, don't know.

@MarkCallow
Copy link
Collaborator

you do some magic, don't know.

I don't do anything. PR's seem to launch 2 builds, the first gets the standard 1 hour limit and fails, the second gets the extended 2 hour limit on the repo and succeeds. I haven't tried to investigate deeply what is going on and it always eventually clears up.

@MarkCallow MarkCallow merged commit 2d6ff94 into KhronosGroup:master Feb 17, 2022
@MarkCallow
Copy link
Collaborator

@wasimabbas-arm thank you for persevering with this. You have created a very useful addition to the functionality which makes it easier to create normal maps.

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 21, 2024
normalMap in ktxAstcParams and ktxBasisParams (`--normal_mode` in `toktx`), causes the
encoders to split the R & G components of the input into the RGB & alpha channels of the
encoded texture and to apply encoder-specific settings that improve quality for normal
maps.

Add `--normalize`  option to `toktx.

Add tests for normalMap and normalize functionality.

Deprecate `separateRGToRGB_A` and remove related code. Fixes KhronosGroup#455.

Co-authored-by: Wasim Abbas <abbas.wasim@gmail.com>
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
normalMap in ktxAstcParams and ktxBasisParams (`--normal_mode` in `toktx`), causes the
encoders to split the R & G components of the input into the RGB & alpha channels of the
encoded texture and to apply encoder-specific settings that improve quality for normal
maps.

Add `--normalize`  option to `toktx.

Add tests for normalMap and normalize functionality.

Deprecate `separateRGToRGB_A` and remove related code. Fixes KhronosGroup#455.

Co-authored-by: Wasim Abbas <abbas.wasim@gmail.com>
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
normalMap in ktxAstcParams and ktxBasisParams (`--normal_mode` in `toktx`), causes the
encoders to split the R & G components of the input into the RGB & alpha channels of the
encoded texture and to apply encoder-specific settings that improve quality for normal
maps.

Add `--normalize`  option to `toktx.

Add tests for normalMap and normalize functionality.

Deprecate `separateRGToRGB_A` and remove related code. Fixes KhronosGroup#455.

Co-authored-by: Wasim Abbas <abbas.wasim@gmail.com>
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
normalMap in ktxAstcParams and ktxBasisParams (`--normal_mode` in `toktx`), causes the
encoders to split the R & G components of the input into the RGB & alpha channels of the
encoded texture and to apply encoder-specific settings that improve quality for normal
maps.

Add `--normalize`  option to `toktx.

Add tests for normalMap and normalize functionality.

Deprecate `separateRGToRGB_A` and remove related code. Fixes KhronosGroup#455.

Co-authored-by: Wasim Abbas <abbas.wasim@gmail.com>
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
normalMap in ktxAstcParams and ktxBasisParams (`--normal_mode` in `toktx`), causes the
encoders to split the R & G components of the input into the RGB & alpha channels of the
encoded texture and to apply encoder-specific settings that improve quality for normal
maps.

Add `--normalize`  option to `toktx.

Add tests for normalMap and normalize functionality.

Deprecate `separateRGToRGB_A` and remove related code. Fixes KhronosGroup#455.

Co-authored-by: Wasim Abbas <abbas.wasim@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants