Experimental internal-context-less branch

[Laurent]

My questions so far:

1. Why did you remove a call to glFlush() from Font.cpp, and added one to Texture.cpp?

2. Threads with no active target no longer create their own context, but share the same one; won't it add too much overhead to constantly synchronize GL calls and activate/deactivate the shared context between these threads? Maybe we could run a quick benchmark on the corresponding use case (spawn threads in a loop, that do various GL operations through sfml-graphics classes).

3. Why don't you create a PR?

[binary1248]

Why did you remove a call to glFlush() from Font.cpp, and added one to Texture.cpp?

Strictly speaking, Font isn't a GlResource so it shouldn't make GL calls itself. It owns Textures, so it can rely on them calling glFlush() when their texture data actually gets modified. Also, there was a glFlush() missing from one of the update()s in Texture, so I added it for consistency.

Threads with no active target no longer create their own context, but share the same one; won't it add too much overhead to constantly synchronize GL calls and activate/deactivate the shared context between these threads?

In a realistic scenario, users either perform rendering on secondary threads (which I always recommend against anyway) or use them to offload some heavy resource loading. In the former case, there is already an active context on that thread, whereas in the latter case, most of the time will be spent "doing the other stuff".

Deactivating a context on a thread doesn't do anything else besides flush the context on that thread before being deactivated. Flushing a context is a relatively cheap operation. If we promise the driver that we try really hard to make sure GL operations are already synchronized in our own code, it will have less work to do itself, even if we constantly activate and deactivate contexts. This also allows users to optimize their own code more when they understand that parallel GL operations don't really benefit them in most cases.

Almost all the GL calls in SFML are asynchronous, meaning they return right away. I tried to minimize the time the shared context would be locked (if it even gets used in the first place), so I am fairly confident that there won't be much contention even when GL operations are being performed on multiple threads all using the shared context. We can run benchmarks, but I would be surprised if there was any noticeable performance degradation because of this.

Why don't you create a PR?

All in good time.

[Laurent]

Strictly speaking, Font isn't a GlResource so it shouldn't make GL calls itself. It owns Textures, so it can rely on them calling glFlush() when their texture data actually gets modified. Also, there was a glFlush() missing from one of the update()s in Texture, so I added it for consistency.

I'm pretty sure that there was an issue with calling glFlush in this specific function, which is the reason why it was like it was before.

Ceylo, did you notice a performance drop with sfeMovie?

[binary1248]

I'm pretty sure that there was an issue with calling glFlush in this specific function, which is the reason why it was like it was before.

Well... you introduced glFlush() in multiple places in 6b50691, so you should know best. Although to me, the locations look pretty random.

[eXpl0it3r]

The last thing I remember with glFlush was about font rendering from a separate thread...

[Laurent]

Well... you introduced glFlush() in multiple places in 6b50691, so you should know best.

Don't overestimate me

Although to me, the locations look pretty random

The last thing I remember with glFlush was about font rendering from a separate thread...

Still consistent with the fact that it couldn't be done directly in Texture::update, but was ok in non performance critical places that call it.

I really can't remember what the problem was with glFlush in Texture::update... sorry

[binary1248]

Maybe #411 and this thread help refresh your memory. The people reporting never even mentioned having problems with Shader or Font, only Texture. It was you who suggested adding glFlush() in all of them. Maybe you forgot that Font isn't really a GlResource back then.

But... from a theoretical standpoint, it kind of makes sense that flushing is needed when using internal contexts. With internal contexts, loading a texture or shader on a secondary thread would just queue up GL commands in the contexts on those threads. Depending on how hard the driver likes to optimize, it might try to buffer multiple of them so it can send them off in bigger batches. Obviously if loading a texture in a secondary thread doesn't produce enough operations to dispatch the batch, I'm guessing the driver will just wait until a timeout occurs and send them off anyway. In a multi-threaded application, the timeout would occur after the other threads try to access the resource leading to this problem. One of the solutions mentioned in the thread involves calling wglMakeCurrent(NULL, NULL), but as I said above, this implicitly flushes the context before deactivation as well.

Long story short: We only need to flush where we actually issue GL commands that affect the state of shared objects. Doing so elsewhere doesn't really make much sense and just adds unnecessary overhead.

[Laurent]

Long story short: We only need to flush where we actually issue GL commands that affect the state of shared objects. Doing so elsewhere doesn't really make much sense and just adds unnecessary overhead.

We all agree on that. The point here is to know whether doing it right in Texture::update is ok or not (most likely performance-wise); if not, calling it in higher-level non-OpenGL objects indeed made sense.

An answer from Ceylo about performances of sfeMovie should bring a final answer to this question.

[Elias Daler]

Sometimes I just imagine binary1248 to be Dexter sitting in his secret lab coming up with great things...

This is really great and creating RenderTextures in a separate thread is something which I wanted to do for a long time, he-he.

[Nexus]

I'm pretty sure that there was an issue with calling glFlush in this specific function, which is the reason why it was like it was before.

In the future, it would be good to write comments for such cases

[BlueCobold]

I tested the branch in android. It causes glLoadGen to receive NULL when asking for extension-strings which caused a segfault in glLoadGen-code. I bypassed the segfault, but still can't get any extension-string whatsoever. This used to work properly in the master-branch.
PS: gl::getIntegerv(GL_MAX_TEXTURE_SIZE) of glLoadGen also returns 0. That's said to happen without a valid context.

Apart of that, it fixes the following bug: http://en.sfml-dev.org/forums/index.php?topic=19180.0

[binary1248]

Rebased onto current master and created pull request #1002.

BlueCobold, you will have to provide a minimal example for me to test. As stated in the PR, this changes the way contexts are activated/deactivated slightly. Any prior assumptions on whether a context was active/inactive after certain calls into the library might no longer hold if they were undocumented. The documentation is still correct at this point, it's just the assumptions that "people just had to know" that have changed or are entirely gone. One of the goals of this PR is to make context behaviour and management consistent, a bit more explicit and therefore more predictable and less "random" as I consider it in the current implementation. It might break the code of users who have gotten used to the assumptions in the past, but it will make learning to use SFML (and OpenGL with SFML) much easier for newer users.

[eXpl0it3r]

Any updates on this? BlueCobold do you have an example for binary1248?

[BlueCobold]

No. Such an example would require external dependencies to glLoadGen, etc.. I still don't know to handle it on my side though, I just picked glLoadGen in the past and it used to work, whereas now it doesn't. There probably indeed isn't a proper context active or something, but I can't tell.

[binary1248]

Surely you can construct a simple example that demonstrates the problem when only using glLoadGen to load extensions. I'm not asking for manual extension loading, just a reduced version of your (now broken) code. Even just excerpts of critical code sections would be useful.