Fixed text rendering artifacts on Radeon cards.

[oomek]

Padding set to 1.0 was causing adjacent glyphs leaking on Radeon cards. The initial padding value for glyphs was too high anyway, the proper value should be 0.5 on each side.

Example video: https://youtu.be/sN44kuPA1Jk

[eliasdaler]

Can you please post before-after screenshots, so we can compare two padding values directly?

[oomek]

It's in the linked video, I can only post a screenshot now zoomed in of the text rendering before the fix:

[eliasdaler]

I see, so there's some tearing. Does the distance between glyphs change? Or is it still the same?
If so, I think that this PR should be approved. At the worst case we should have padding configurable for sf::Font (not sure if other values would be meaningful for it)

[oomek]

I tested different padding sizes and compared screenshots before my initial commit with padding 1.0. Since I apply the padding to the size and uv coords the glyph size and positioning stay the same, so no worries with that. Adding an additional parameter would not serve any useful purpose.

[eliasdaler]

Am I correct that setting padding of 0.5 makes vertices have non-integer coordinates? This makes me a bit anxious, because from experience I've seen non-integer coordinates cause a lot of tearing.

Otherwise we need to test:

• For different GPUs
• For different fonts

Right now it looks like text rendering is really simplistic in SFML and it doesn't take kerning into account, as far as I see. If you render text with 1.0 padding to sf::RenderTexture, does tearing go away?

[oomek]

Don't be.
UV and coord set to -0.5 null each other out.
Without padding the glyphs were clipping during sub pixel scrolling.
With padding 1.0 on nVidia there is no clipping
With padding 1.0 on AMD there is no clipping, but adjacent glyphs are leaking from the glyph texture.
With padding 0.5 everything is working nicely.

Btw it's the same as half texel offset used for textures by @LaurentGomila several years ago.

[oomek]

I've tested it myself on Win10 with AMD, nVidia and integrated Intel and also on the Raspberry PI

[oomek]

There is an alternative route, but requires modifying glyph texture generation. You leave the padding as 1.0 and add 2 pixel padding between glyphs on the glyph sheet instead of 1.

[eliasdaler]

I think this looks pretty solid then, thanks for testing and explanations.
Now it's to @LaurentGomila and @eXpl0it3r to decide. :)

[eliasdaler]

@binary1248 - any thoughts as well?

[binary1248]

I wouldn't call this phenomenon "leaking" as it can occur even when there is a lot of space between "set" texels, and it should be consistent every time a glyph is rendered, which doesn't look to be the case.

I'm guessing this is one of those cases where the OpenGL specification didn't 100% define how texture filtering should be performed along the edges of the source rectangle where a part of the sampling area basically consists of "undefined" data. I would have to guess how big the sampling area is, but it would be at least 3x3 texels around the centre, maybe even larger.

Sampling is always done at texel centres, so adding 0.5 would be more correct in that sense and leave less room for interpretation/interpolation by the implementation.

I would still also bump the padding between glyphs up to 2 just to be extra safe. I don't think requiring that extra space is going to make any difference in real world scenarios. It is amortized when dealing with larger glyphs anyway which is the only scenario where you would have to worry about texture size.

[oomek]

So, does it mean that you approve my commit?
As I mentioned earlier, my tests has proven 0.5 padding to be effective, the "leaking" is gone and the text is pixel perfect for integer positions as it's without any padding or padding set to 1.0

[binary1248]

Implementations might change. Remembering my work from years ago when broken Intel IGPs were still prevalent, I remember that a padding of 1 was not enough in some rare scenarios. That's why I always used a value of at least 2 in my own code. This might no longer be necessary now with less broken implementations around, so we have the choice of increasing the padding now as a precautionary measure or waiting until a real bug report comes in and increasing it then.

[eXpl0it3r]

so we have the choice of increasing the padding now as a precautionary measure or waiting until a real bug report comes in and increasing it then

I'd vote for merging it as is and wait & see.

@oomek Do you still have the example code? I want to give this a run on my system.

[oomek]

@ eXpl0it3r sure, but tomorrow if you don't mind, as my daughter is occupying my PC atm.

[oomek]

I'm sorry, I forgot to post a sample code, but because I couldn't find it I wrote a new one:

#include <SFML/Graphics.hpp>

int main(int argc, char *argv[])
{
	sf::VideoMode desktop_mode = sf::VideoMode::getDesktopMode();
	sf::RenderWindow window(desktop_mode, "FONT TEST", sf::Style::Fullscreen);

	sf::Font font;
	font.loadFromFile("c:/windows/fonts/arial.ttf");

	sf::Text text;
	text.setFont(font);
	text.setString("Lorem ipsum dolor sit amet,\nconsectetur adipiscing elit,\nsed do eiusmod tempor incididunt\nut labore et dolore magna aliqua.\nUt enim ad minim veniam,\nquis nostrud exercitation ullamco laboris\nnisi ut aliquip ex ea commodo consequat.\n");
	text.setCharacterSize(80);
	text.setFillColor(sf::Color::White);
	sf::Sprite font_sprite;

	window.setVerticalSyncEnabled(true);

	while (window.isOpen())
	{
		sf::Event event;

		while (window.pollEvent(event))
		{
			if (event.type == sf::Event::Closed)
				window.close();

			if (event.type == sf::Event::KeyPressed)
			{
				if (event.key.code == sf::Keyboard::Escape)
					window.close();

				if (event.key.code == sf::Keyboard::Down)
					text.setCharacterSize(text.getCharacterSize() - 1);

				if (event.key.code == sf::Keyboard::Up)
					text.setCharacterSize(text.getCharacterSize() + 1);
			}
		}
		text.move(0.05, 0.05);
		window.clear();
		const sf::Texture& font_tex = font.getTexture(text.getCharacterSize());
		font_sprite.setTexture(font_tex, true);
		font_sprite.setPosition(window.getSize().x - font_tex.getSize().x, 0);
		window.draw(font_sprite);
		window.draw(text);
		window.display();
	}
	return 0;
}

Use up and down arrows.
I've noticed that the "leaks" occur only when I change the size of the text, maybe the reused glyph buffer isn't cleared properly?

Update: displaying glyph texture

[eXpl0it3r]

Thanks for the code! 🙂

It's interesting. If I use SFML-master on the integrated Intel GPU, I notice the artifacts. If I use SFML-master on the discrete Nvidia GPU, I don't see any artifacts.
If I use this PR then I don't see any artifacts on the integrated Intel GPU nor on the discrete Nvidia GPU.

In all cases the letters wobble quite a bit when slowly enlarging the font, but I guess that's to be expected.

Will try my AMD GPU at home this evening.

[oomek]

What's very weird, when you don't change the size of the characters there aren't any artifacts, and it does not matter what setCharacterSize() you have set in the beginning. It only manifests when you change the font size at runtime, so it made me thinking, maybe reused textures for glyphs have old uncleared glyph data still in the padding area? what do you think?

[LaurentGomila]

maybe reused textures for glyphs have old uncleared glyph data still in the padding area? what do you think?

Glyph textures are never reused. A sf::Font instance does not know what glyphs and sizes will be requested by sf::Text objects, so once it has generated something, it keeps it until its destruction. Internally, sf::Font keeps one separate set of textures per requested character size.

[oomek]

Sorry wrong wording, not glyph data, the textures reused by the driver. Please try my revised code. It's clearly showing the old glyphs from previous sizes that were generated before.

When you analyze the screenshot you will see that the distance between new glyphs is 2 pixels, but between an old data is just 1 pixel. I believe this is where the artifacts are coming from. Changing the padding made filtering not catching the old glyph data, but now I think the root of the problem should be addressed. A simple clearing the texture after it's generated should fix the problem.

[LaurentGomila]

A simple clearing the texture after it's generated should fix the problem.

Would be easy to try, the clearing should be made here: https://github.com/SFML/SFML/blob/master/src/SFML/Graphics/Font.cpp#L729

[oomek]

Is it ok to change the newTexture to RenderTexture? Clearing with update() from an image or a buffer may add some unneccessary overhead.

Update: I'm answering to myself. With RenderTexture I can't use update() or swap() without an intermediary sprite.

[oomek]

Would this be acceptable? It works, but requires an Image.

// Make the texture 2 times bigger
Texture newTexture;
newTexture.create(textureWidth * 2, textureHeight * 2);
Image image;  //added
image.create(textureWidth * 2, textureHeight * 2, Color::Transparent);  //added
newTexture.loadFromImage(image);  //added
newTexture.setSmooth(true);
newTexture.update(page.texture);
page.texture.swap(newTexture);

It would be handy to have an additional texture.create() parameter that takes a color to fill it after initialization, or a dedicated clear() function.

[LaurentGomila]

Before talking about the best way to do it... does it solve the problem? Did you make sure to revert your previous changes (padding) before testing this solution?

[oomek]

Yes, it works with padding 1.0

[LaurentGomila]

Ok great.

The best way to clear the texture would be to use a dedicated OpenGL feature (attaching to a FBO + glClear, or the GL_ARB_texture_clear extension, ... found on stackoverflow). No idea how hard it would be to integrate to SFML, or if we can even do it, according to our requirements on OpenGL version.

The other solution is to allocate a vector of width*height pixels and call texture.update(...) with that. With the possibility to reuse a single vector to avoid big allocations.

[oomek]

Would you rather have a dedicated function for it, or mixed code in font.cpp?

glClearTexImage() seems like the most elegant solution to me. This extension is OpenGL 1.3 complaint. Is this enough of backward compatibility?

[oomek]

Ok, how about this:

RenderTexture renderTexture;
renderTexture.create(textureWidth * 2, textureHeight * 2);
renderTexture.clear(sf::Color::Transparent);
Texture newTexture(renderTexture.getTexture());
newTexture.setSmooth(true);
newTexture.update(page.texture);
page.texture.swap(newTexture);

It's actually working.

[LaurentGomila]

Seems like we need to summon @binary1248 😉

[binary1248]

There are a few points to discuss here.

Is clearing the whole texture necessary when resizing it to accommodate for new glyphs? If e.g. we resized the texture to 4x the size as is done in the current implementation, and only wanted to insert a single glyph, wouldn't it be wasteful to initialize the area that we never intended on using anyway? If uninitialized texture data is the root cause of the problem (and it seems likely), I would focus my effort on initializing only the regions we intend on using, i.e. the area around glyphs that will actually be considered for filtering.

When resizing an already generated texture object, OpenGL essentially does a "free" and then a "malloc". Because of the proximity of the operations, it would almost always lead to the old block being reused, maybe slightly offset from before in some way. As such, trying to access this data, even through sampling, would be considered as "undefined behaviour" in C++ land.

Quoting the OpenGL specification:

If the data argument of TexImage1D, TexImage2D, orTexImage3D is NULL, and the pixel unpack buffer object is zero, a one-, two-, or three-dimensional texture image is created with the specified target, level, internalformat, border, width, height, and depth, but with unspecified image contents.

As I said in my previous comment, if OpenGL is factoring in a bigger texture area than we initialized, we will get these artifacts. The simplest solution would be to increase the padding and see what happens. Clearing out the whole texture is overkill if you ask me.

When considering performance, which as I understand is what is currently being discussed, we have to look at the actual access patterns before ending up micro-optimizing or even pessimizing the implementation in ways that don't correspond to real-world usage.

How often will an sf::Font texture realistically be resized during the lifetime of an application?
Would the clearing of the texture really dominate this time? Or would some other operation, e.g. FreeType-related consume measureably more time than the actual texture manipulations?

Bear in mind that for quite a while, GPUs and iGPUs have been attached to the system via PCIe or some other form of PCIe-like interface. These interfaces don't like small transfers. Any decent driver will batch a big chunk of "data" together before being sent to the GPU. This increases average latency but also significantly increases throughput as well.

As such, if I had to guess, expanding the rectangle around a glyph to initialize a bigger padding area to proper values wouldn't even have a performance penalty, unless of course the glyphs were really really huge which doesn't happen every day.

This would be the variant I favour.

Because I too find that in some situations clearing a texture to a predefined colour can be useful when the user knows that they would otherwise have to do it "manually", I propose adding a sf::Texture::clear(const sf::Color&) method to sf::Texture. Initially this can be implemented using the naive OpenGL 1.1 compatible implementation. Building in optimizations that take into account extensions and even multiple platforms can get complicated really fast.

---

Edit: I tested the test code on my desktop with an AMD RX Vega 64 and I can see constant periodic "flickering"/"blinking" of the sf::Text around the edges of the glyphs at certain font sizes. Looks like this is some kind of implementation-specific behaviour. Simply changing the padding constant at https://github.com/SFML/SFML/blob/master/src/SFML/Graphics/Font.cpp#L604 from 1 to 2 eliminated all artifacts for me.

[oomek]

Oh god, I somehow knew I'm not gonna get a simple yes or no answer, but I agree with everything you said. I was also thinking about increasing the padding to 2.0 for font generation and using a smaller padding 1.0 for drawing. The only thing that was stopping me was a hope that we will get a clear() function for textures which I was hoping to get as a bonus, but since you've mentioned it I'm all up for it. I was having the same artifacts on the bottom and right edges while moving textures after updating them dynamically with png's with various sizes. I'm glad we can agree on that. I'm updating the PR then.

[binary1248]

My final analysis would be:

PR #1452 introduced a 1 pixel border of "overdraw" around each glyph in order to reduce motion artifacts. This border started to place texel centres within the padding region of the glyph textures. Since this padding was only 1 pixel wide, when OpenGL is forced to interpolate the texture data due to vertices having non-integer coordinates, linear filtering is applied (SFML always set font textures to smooth). Linear filtering samples a 2x2 texel area whose centre lies closest to the texture coordinate in question. Because this 2x2 area would include texels beyond the 1 pixel padding area, regions of undefined texel data would be sampled as well leading to the artifacts along the edges of the glyphs.

The solution was to adjust the padding in accordance with the overdraw introduced in PR #1452 to make sure that any potentially sampled texels within the padding area are initialized to transparent.

The padding in Font.cpp should therefore always be 1 more than the padding in Text.cpp in order to prevent artifacts from appearing.

As an example: setting padding to 2 in both files will cause artifacts as well.

[LaurentGomila]

Agreed 100% 👍

[naezith]

This fixed Nintendo Switch port too.