xbmc icon indicating copy to clipboard operation
xbmc copied to clipboard

Enables use of single/dual channel textures

Open sarbes opened this issue 3 years ago • 10 comments

Description

This PR enables skinners to use single/dual channel textures by naming them "filename.[channelusage].png".

[channelusage] can be the following values:

  • "alpha" is a single channel texture, which is equivalent to [FF,FF,FF,channel1]
  • "luma" is a single channel texture, which is equivalent to [channel1,channel1,channel1,FF]
  • "intensity" is a single channel texture, which is equivalent to [channel1,channel1,channel1,channel1]
  • "lumaalpha" is a dual channel texture, which is equivalent to [channel1,channel1,channel1,channel2]

Motivation and context

Currently, GPU textures in Kodi are often (always?) stored as BGRA, irregardless of the actual content. This wastes a lot of resources, as a lot of assets are only greyscale. For Estuary, this is the case for >90% of all assets.

Replacing the colored textures would save a large amount of memory bandwidth. The background textures of Estuary are ~10MB large. This occupies >600MB/s memory bandwidth at 1080p60(might be lower with compression), which is a lot on low end systems. Single channel textures would save 75% of the BW.

Also, compression (disk memory) might be a bit better on when using single channel textures.

For OpenGL >=3.3 (or suitable extensions), both "luma" and "alpha" hints are supported. Same goes for OpenGL ES 3.0. OpenGL ES 2.0 supports just the "luma" hint.

In every other case, the texture gets converted into a full GBRA texture.

DX would need its own implementation. GLES 2.0 and lower OpenGL versions could support the "alpha" hint with additional shaders, but I don't think it is worth the time do implement this.

How has this been tested?

Compiles and runs fine with GL and GLES.

DX is a shot in the blue, but fortunately it is not complex.

What is the effect on users?

Moar speed on lower end systems.

Screenshots (if appropriate):

Here is what Renderdoc reported before the change: before

And after the change for GL: after_gl

...and for GLES 2.0: after_gles

Types of change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • [ ] Improvement (non-breaking change which improves existing functionality)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that will cause existing functionality to change)
  • [ ] Cosmetic change (non-breaking change that doesn't touch code)
  • [ ] None of the above (please explain below)

Checklist:

  • [x] My code follows the Code Guidelines of this project
  • [x] My change requires a change to the documentation, either Doxygen or wiki
  • [x] I have updated the documentation accordingly
  • [x] I have read the Contributing document
  • [ ] I have added tests to cover my change
  • [x] All new and existing tests passed

sarbes avatar Jun 15 '22 00:06 sarbes

On unsupported systems, the texture gets converted via ConvertToBGRA(). For older GL systems without swizzling, I would need to add a dozen or so shaders (I'm certain Lukas would rip my head off if I would do that :D). And I'm staying away from DX. From what I've gathered, there is no swizzling, so everything would have to be done in shaders.

I'll have to add some defines for systems without newer GL headers (i.e. TVOS). Together with your suggestions.

Edit: and I would wager that the "luma" effect would be the most used one, so OpenGL ES 2.0 does not miss out too much.

And GL_EXT_texture_swizzle stems from 2008. Even the goold old ION would have had support for it: https://feedback.wildfiregames.com/report/opengl/device/GeForce%209400M https://feedback.wildfiregames.com/report/opengl/feature/GL_ARB_texture_swizzle

sarbes avatar Jun 15 '22 23:06 sarbes

Sorry @garbear, I was not happy with the previous approach. I rewrote the selection logic behind it.

Now, the file extension is used to determine the texture property. This is a better approach as it will avoid situations where the skin might want to have different versions of the textures. Because it is not a skin attribute anymore, there is no need to transition all assets as it might be the case before. I can also integrate it into the texture packer in the future. A file is identified by naming it "texturename.luma.png" or equivalent.

Also on the plus side, we do have more options. Besides the "alpha" and "luma" qualifier, there is also "intensity" and "lumaalpha" The latter is a dual channel texture.

New for GLES 2.0 is the option to use dual channel textures if the extension "GL_EXT_texture_rg" is available. In this case, "alpha" and "intensity" will be converted to a dual channel texture, saving 50% resources.

I've renamed the original format XB_FMT_A8 to XB_FMT_R8, as this reflects its usage far more accurate.

sarbes avatar Jun 16 '22 18:06 sarbes

GL_GREEN is missing for iOS, I'll add the define when Jenkins is done.

sarbes avatar Jun 16 '22 19:06 sarbes

I've renamed the original format XB_FMT_A8 to XB_FMT_R8, as this reflects its usage far more accurate.

Can you explain this as if I don't know what any of those letters are abbreviations for?

garbear avatar Jun 16 '22 20:06 garbear

I think I see an area of improvement. We can currently convert between texture formats on the fly, back and forth. It would improve performance to not free old pixels and then having to regenerate them.

If freeing old pixels is still desired, then I would argue to move the mutation out of the texture. Let the calling object create a new texture of a different format and free the old texture, moving old/new memory management out of CTexture.

garbear avatar Jun 16 '22 20:06 garbear

I've renamed the original format XB_FMT_A8 to XB_FMT_R8, as this reflects its usage far more accurate.

Can you explain this as if I don't know what any of those letters are abbreviations for?

Previously, XB_FMT_A8 stand for a generic single channel texture. On OpenGL and DX, it was used as the red component for font. I've changed it to reflect this aspect. I would say it stands for XBMC_FORMAT_APLHA8{bit].

While it is possible to use the identifier for other kinds of texture representations, the actual intent of what the texture is has to be stored somewhere. So the new identifiers were born, reflecting what the texture is representing.

I think I see an area of improvement. We can currently convert between texture formats on the fly, back and forth. It would improve performance to not free old pixels and then having to regenerate them.

If freeing old pixels is still desired, then I would argue to move the mutation out of the texture. Let the calling object create a new texture of a different format and free the old texture, moving old/new memory management out of CTexture.

I could allocate memory for a whole four channel texture all the time and work in this space without an extra alloc. Is that what you mean?

sarbes avatar Jun 16 '22 22:06 sarbes

I've made some formatting changes to meet the current code style. The diffs are available in the following links:

For more information please see our current code style guidelines.

jenkins4kodi avatar Jun 22 '22 17:06 jenkins4kodi

Huh, seems to me that Mali-450 GPU's do support GL_INTENSITY and swizzling in hardware. But GLES 2.0 does not expose this function, and there is no suitable extension. What a mess.

Lima supports those with OpenGL 2.1.

sarbes avatar Jun 24 '22 15:06 sarbes

Maybe time for a state-of-the OpenGLES 3.1+ path?

fritsch avatar Jun 24 '22 17:06 fritsch

Na, Not for this. I'm totally fine writing the fallback for this minor instance. It is not restrictive.

sarbes avatar Jun 25 '22 17:06 sarbes

I've rewritten some parts and pushed a new version. The highlights:

  • GL_INTENSITY type textures have been dropped. I don't see much use.
  • OpenGL ES 2.0 supports luma and luma/alpha textures "normally", and pure alpha variants via shaders.
  • OpenGL >=3.3 supports all via swizzling.
  • All other platforms decode the texture to full BGRA.
  • The texture packer is now able to pack such textures from PNG files. Extensions like "filename.luma.png" will be shortened to "filename.png", so that the hard coded default textures can be supplied.

Now, I have to figure out how to resolve the conflict (:.

sarbes avatar Nov 28 '22 22:11 sarbes

jenkins build this please

garbear avatar Dec 20 '22 14:12 garbear

It would be nice if you could squash the last two commits into the relevant prior commits.

This is probably easiest to reset the commit, do a partial add, then commit as a fix up, and autosquash in a rebase.

Something like

git reset HEAD^

git add -p
git commit --fixup=<sha-of-relevant-commit>

Repeat last two commands till all changes are committed.

git rebase -i --autosquash upstream/master

lrusak avatar Dec 20 '22 22:12 lrusak

Hi @sarbes

from what I understand, to make the change effective you need to apply changes to the skin (e.g. estuary). It's correct?! If yes, the rule for renaming files can be

  • jpg --> luma
  • png b/w --> alpha
  • others --> lumaalpha

or it always depends on the skin and not on the file (if it's in color or not)!?!

edit: From my understanding, the texture packer tool checks the png type and packs the textures accordingly :)

ilmich avatar Mar 14 '23 07:03 ilmich

Hi @sarbes

building kodi (nexus 20.1) with this patch and #22919 I can say that the fluidity of my rk3229/ddr2 box has improved a lot and I haven't noticed any problems of any kind.

Thank you very much

ilmich avatar Mar 14 '23 17:03 ilmich

In this iteration, the packer is looking at the format of the PNG and for special "markers" in the file name. This needs proper file handling on the skinner side (by providing grescale textures).

But I've redone the texture packer again. Now, it scans through all the pixels, and chooses a proper format based on its findings. With it, even a full RGBA PNG will be stored as an alpha only texture, if the color channels are all 0xff. No skin/texture changes needed, provided that textures are properly done. I'll commit it soon.

In practice, this means that Estuary reduces its texture footprint by over 60%, as it is composed mainly out of alpha only textures. Some textures (around 10%) are broken (having varying colour information) and are stored as luma/alpha textures, which has to be fixed separately.

For Estuary, the most performance uplift with this patch would come from providing greyscale background textures. Either replace the JPGs in skin.estuary/extras/backgrounds/ with greyscale versions (I have not found a way to strip the chroma channels without reencoding) or attach the "_luma.jpg" hint (+update the XML). At 60fps, this saves over 540MB/s.

sarbes avatar Mar 15 '23 09:03 sarbes

The texture packer now reduces textures to a suitable format.

sarbes avatar Mar 16 '23 00:03 sarbes

Hi @sarbes,

I did some tests renaming the files but didn't notice the expected improvements. Surely I'm doing something wrong, or the jpegs should be treated differently. But you hit the 'problem' because by mistake in one of my tests, I completely removed the background(typo in filename) and estuary became ultra fast.

ilmich avatar Mar 18 '23 23:03 ilmich

@ilmich I've double checked it with Renderdoc. Skin (XBT) textures are uploaded as GL_LUMINANCE/GL_ALPHA​/GL_LUMINANCE_ALPHA​ on GLES.

I forgot to update my top post. Textures should be marked with an underscore (e.g. "primary_luma.jpg"). This might be your problem.

Another way to get "automatic" texture format handling is by providing compatible image formats. Greyscale JPEGs (without any chroma channels) will decode to GL_LUMINANCE textures. Greyscale PNGs will decode to GL_LUMINANCE or GL_LUMINANCE_ALPHA​, depending on the present of an alpha channel.

sarbes avatar Mar 19 '23 11:03 sarbes

Hi @sarbes,

for my tests with libreelec I used this snippet for jpeg images

for file in ${INSTALL}/usr/share/kodi/addons/skin.estuary/extras/backgrounds/*.jpg; do 
        mogrify "$file" -colorspace Gray -quality 90
done

which should allow the use of luma textures. I don't know if I did everything correctly, I see a slight improvement. But keep in mind that I use them with Nexus (they still apply without problems) and that I probably approached hardware limitations, rather than software inefficiencies.

ilmich avatar Mar 20 '23 08:03 ilmich

which should allow the use of luma textures. I don't know if I did everything correctly, I see a slight improvement. But keep in mind that I use them with Nexus (they still apply without problems) and that I probably approached hardware limitations, rather than software inefficiencies.

This PR should relief the memory bus quite a bit. There are three possibilities:

  • Your device is not memory bandwidth limited
  • Your device is GPU bound (maybe)
  • The driver is converting the texture into an RGBA texture internally (shouldn't be)

You can test if your device is memory bandwidth limited by replacing those textures with 1x1 pixel textures. If the framerate is increasing, you are bandwidth bound.

sarbes avatar Mar 20 '23 17:03 sarbes

Hi @sarbes

I have tested this patch daily and have not seen any bugs.

This PR should relief the memory bus quite a bit. There are three possibilities:

  • Your device is not memory bandwidth limited
  • Your device is GPU bound (maybe)
  • The driver is converting the texture into an RGBA texture internally (shouldn't be)

You can test if your device is memory bandwidth limited by replacing those textures with 1x1 pixel textures. If the framerate is increasing, you are bandwidth bound.

I wrote wrong,

  • without your patch, my device has memory bandwidth limits
  • with your patch the memory limits disappear and therefore I get limits on the gpu
  • converting the background jpeg does not change anything having reached the limits of the GPU

However, it's an excellent result and I thank you for your work.

ilmich avatar Apr 03 '23 07:04 ilmich

Can you please rebase. I want to look at the D3D part but there have been quite a few build changes for Windows recently and switching backwards and forwards is painful.

CrystalP avatar Apr 12 '23 23:04 CrystalP

@sarbes this needs a rebase

jenkins4kodi avatar Apr 22 '23 02:04 jenkins4kodi