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

DDS: various improvements #3573

Merged
merged 1 commit into from
Oct 2, 2022
Merged

DDS: various improvements #3573

merged 1 commit into from
Oct 2, 2022

Conversation

aras-p
Copy link
Contributor

@aras-p aras-p commented Sep 28, 2022

Description

  • If a DDS pixel format header contains "this is a normal map" flag set, then handle that properly ([HELP] Converting a DDS normal map to jpeg #3525):
    • For a DXT5, this means it's a "DXT5nm" format. Normal X & Y components are in green and alpha channel respectively. Compute Z component from those, and the result is a 3-channel tangent space normal map.
    • For ATI2/BC5 format that also has a "normal map" flag set, compute the Z component automatically too, and result is a 3-channel image. However a bunch of ATI2/BC5 normal maps out there in the wild do not have that bit set, so also have a global "dds:bc5normal" attribute to optionally turn this on.
  • Fix support of R10G10B10A2 format - was producing garbage data due to code assumption that truncation down to 8 bits never needs to happen.
  • Fix bit expansion ("too dark") of very low-bit formats, e.g. R3G3B2. Code was simply shifting the bits, instead of properly repeating the high bits into new low bit positions (i.e. a 3-bit "111" needs to become an 8-bit "11111111" and not a "11100000").
  • Support "RXGB" (DXT5, with red & alpha swapped) as well as "BC4U" and "BC5U" formats (the latter two are just BC4 and BC5, produced by some older tools).
  • Support alpha-only (e.g. A8) formats.

All this mostly prompted by doing DDS improvements in Blender, and wanting to get Cycles renderer (which uses OIIO) to the same feature parity. See https://developer.blender.org/T101405 and https://developer.blender.org/D16087.

Tests

⚠️ Not yet! I should probably add a bunch of new images to oiio-images test suite, but first wanted to check if these suggested improvements even make sense.

Checklist:

  • I have read the contribution guidelines.
  • If this is more extensive than a small change to existing code, I
    have previously submitted a Contributor License Agreement
    (individual, and if there is any way my
    employers might think my programming belongs to them, then also
    corporate).
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM, other than my comment on how I think we already have an equivalent function to your bit scaling.

src/dds.imageio/ddsinput.cpp Outdated Show resolved Hide resolved
@aras-p aras-p requested a review from lgritz October 1, 2022 08:35
Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

Can I ask for one more thing? In the documentation file builtinplugins.rst, do you think you could add a brief note to the DDS section explaining what's up with normal maps in this format and advising use of the global "dds:bc5normal" attribute if they wish to change the default behavior of how these files are interpreted?

- If a DDS pixel format header contains "this is a normal map" flag
  set, then handle that properly (#3525):
  - For a DXT5, this means it's a "DXT5nm" format. Normal X & Y
    components are in green and alpha channel respectively. Compute Z
    component from those, and the result is a 3-channel tangent space
    normal map.
  - For ATI2/BC5 format that also has a "normal map" flag set, compute
    the Z component automatically too, and result is a 3-channel image.
    However a bunch of ATI2/BC5 normal maps out there in the wild do
    not have that bit set, so also have a global "dds:bc5normal"
    attribute to optionally turn this on.
- Fix support of R10G10B10A2 format - was producing garbage data due to
  code assumption that truncation down to 8 bits never needs to happen.
- Fix bit expansion ("too dark") of very low-bit formats, e.g. R3G3B2.
  Code was simply shifting the bits, instead of properly repeating
  the high bits into new low bit positions (i.e. a 3-bit "111" needs
  to become an 8-bit "11111111" and not a "11100000").
- Support "RXGB" (DXT5, with red & alpha swapped) as well as "BC4U"
  and "BC5U" formats (the latter two are just BC4 and BC5, produced by
  some older tools).
- Support alpha-only (e.g. A8) formats.

All this mostly prompted by doing DDS improvements in Blender, and
wanting to get Cycles renderer (which uses OIIO) to the same feature
parity. See https://developer.blender.org/T101405 and
https://developer.blender.org/D16087.
@aras-p
Copy link
Contributor Author

aras-p commented Oct 2, 2022

Can I ask for one more thing? In the documentation file builtinplugins.rst, do you think you could add a brief note to the DDS section

Done! I also took a liberty of replacing "this is an awful format" sentence with something that says "this is for direct-to-GPU usage". It's not awful; it's just very, very specific use case. For something you want to stuff directly into a GPU, DDS is an excellent format; but indeed it's not much of use for use cases outside of that.

@aras-p aras-p requested a review from lgritz October 2, 2022 15:59
Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

Perfect, and thanks for the cleanup of the intro. I must have been extra cranky when I wrote that, and it's definitely not the style I'm going for in the docs.

@lgritz lgritz merged commit 5dd9fee into AcademySoftwareFoundation:master Oct 2, 2022
@aras-p
Copy link
Contributor Author

aras-p commented Oct 2, 2022

@lgritz by the way would it be possible to get this into 2.4.x release? For Blender they are looking at updating to 2.4 "soon after it's out" and lack of feature parity wrt DDS (compared to internal non-OIIO code) was one of the things that would be nice to address.

@lgritz
Copy link
Collaborator

lgritz commented Oct 2, 2022

I was definitely going to backport it. Ordinarily, I would have waited until the next tagged release (just had a release yesterday!), but since this is important for Blender, I can do a patch right away if people are waiting for it.

lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request Oct 2, 2022
- If a DDS pixel format header contains "this is a normal map" flag
  set, then handle that properly (AcademySoftwareFoundation#3525):
  - For a DXT5, this means it's a "DXT5nm" format. Normal X & Y
    components are in green and alpha channel respectively. Compute Z
    component from those, and the result is a 3-channel tangent space
    normal map.
  - For ATI2/BC5 format that also has a "normal map" flag set, compute
    the Z component automatically too, and result is a 3-channel image.
    However a bunch of ATI2/BC5 normal maps out there in the wild do
    not have that bit set, so also have a global "dds:bc5normal"
    attribute to optionally turn this on.
- Fix support of R10G10B10A2 format - was producing garbage data due to
  code assumption that truncation down to 8 bits never needs to happen.
- Fix bit expansion ("too dark") of very low-bit formats, e.g. R3G3B2.
  Code was simply shifting the bits, instead of properly repeating
  the high bits into new low bit positions (i.e. a 3-bit "111" needs
  to become an 8-bit "11111111" and not a "11100000").
- Support "RXGB" (DXT5, with red & alpha swapped) as well as "BC4U"
  and "BC5U" formats (the latter two are just BC4 and BC5, produced by
  some older tools).
- Support alpha-only (e.g. A8) formats.

All this mostly prompted by doing DDS improvements in Blender, and
wanting to get Cycles renderer (which uses OIIO) to the same feature
parity. See https://developer.blender.org/T101405 and
https://developer.blender.org/D16087.
lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request Oct 2, 2022
- If a DDS pixel format header contains "this is a normal map" flag
  set, then handle that properly (AcademySoftwareFoundation#3525):
  - For a DXT5, this means it's a "DXT5nm" format. Normal X & Y
    components are in green and alpha channel respectively. Compute Z
    component from those, and the result is a 3-channel tangent space
    normal map.
  - For ATI2/BC5 format that also has a "normal map" flag set, compute
    the Z component automatically too, and result is a 3-channel image.
    However a bunch of ATI2/BC5 normal maps out there in the wild do
    not have that bit set, so also have a global "dds:bc5normal"
    attribute to optionally turn this on.
- Fix support of R10G10B10A2 format - was producing garbage data due to
  code assumption that truncation down to 8 bits never needs to happen.
- Fix bit expansion ("too dark") of very low-bit formats, e.g. R3G3B2.
  Code was simply shifting the bits, instead of properly repeating
  the high bits into new low bit positions (i.e. a 3-bit "111" needs
  to become an 8-bit "11111111" and not a "11100000").
- Support "RXGB" (DXT5, with red & alpha swapped) as well as "BC4U"
  and "BC5U" formats (the latter two are just BC4 and BC5, produced by
  some older tools).
- Support alpha-only (e.g. A8) formats.

All this mostly prompted by doing DDS improvements in Blender, and
wanting to get Cycles renderer (which uses OIIO) to the same feature
parity. See https://developer.blender.org/T101405 and
https://developer.blender.org/D16087.
@lgritz
Copy link
Collaborator

lgritz commented Oct 3, 2022

Released as v2.4.4.2

@aras-p aras-p deleted the dds branch October 3, 2022 18:09
lgritz pushed a commit that referenced this pull request Oct 5, 2022
* DDS: fixes for A8, L8, A8L8 formats

While developing #3573 at one time I had them working properly, but then
while fixing the address sanitizer failure for 3-channel formats I
botched them again. Note to self: never again do a "yeah I'll add tests
later", since if I had all of them running all the time this would
not have happened. 🤦

* DDS: extend test coverage for more formats & channel size cases

With more test images recently added (#3459),
start using them for DDS tests. This covers now:
- Alpha, Luminance and Alpha+Luminance formats,
- RGB formats with rarer channel sizes (rgb10 a2, r3g3b2),
- Several "broken" DDS file cases
- Removed usage of sample-DXT1 since it is well covered by others.
lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request Oct 6, 2022
…ftwareFoundation#3581)

* DDS: fixes for A8, L8, A8L8 formats

While developing AcademySoftwareFoundation#3573 at one time I had them working properly, but then
while fixing the address sanitizer failure for 3-channel formats I
botched them again. Note to self: never again do a "yeah I'll add tests
later", since if I had all of them running all the time this would
not have happened. 🤦

* DDS: extend test coverage for more formats & channel size cases

With more test images recently added (AcademySoftwareFoundation#3459),
start using them for DDS tests. This covers now:
- Alpha, Luminance and Alpha+Luminance formats,
- RGB formats with rarer channel sizes (rgb10 a2, r3g3b2),
- Several "broken" DDS file cases
- Removed usage of sample-DXT1 since it is well covered by others.
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.

None yet

2 participants