SFML community forums

General => General discussions => Topic started by: binary1248 on October 09, 2015, 04:10:26 am

Title: Experimental internal-context-less branch
Post by: binary1248 on October 09, 2015, 04:10:26 am
Hi guys, I recently came up with a way to completely get rid of the need for those infamous "internal contexts" that have plagued SFML for so long. This solves many problems/annoyances without actually changing the behaviour of the library for anybody who follows the official usage guidelines/documentation and doesn't rely on undocumented/undefined behaviour.

With internal contexts gone, you might have guessed, SFML has to deal with less contexts. Currently, even if you are working with a single window, SFML will create and hold on to at least 3 contexts although only one of them is really ever used. This branch gets rid of one of the unused contexts, the other one is another story. Less contexts means smaller memory footprint and less work for the OS/driver if that means anything.

The biggest change however, would be that people can actually start creating RenderTextures and other OpenGL resources in threads without having to worry about their system blowing up at some point. The only reason this was an issue previously was because of the whole internal context story.

For the time being I've pushed the branch to my own repository. After enough people have tested it and made sure I didn't utterly break too much stuff, I'll push it to the main repository and open a formal pull request. It might get merged one day.

Here it is for your testing pleasure: https://github.com/SFML/SFML/tree/feature/no_internal_context
Title: Re: Experimental internal-context-less branch
Post by: Ceylo on October 09, 2015, 08:16:59 am
Can you explain why these contexts are an issue?
A few more MB is no trouble for today's computers. So if it breaks usages (can you states which ones?) I'm not convinced it's a good idea.
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on October 09, 2015, 10:45:40 am
Quote from: Ceylo on October 09, 2015, 08:16:59 am
Can you explain why these contexts are an issue?
Quote from: binary1248 on October 09, 2015, 04:10:26 am
The biggest change however, would be that people can actually start creating RenderTextures and other OpenGL resources in threads without having to worry about their system blowing up at some point. The only reason this was an issue previously was because of the whole internal context story.
Quote from: Ceylo on October 09, 2015, 08:16:59 am
So if it breaks usages (can you states which ones?) I'm not convinced it's a good idea.
Quote from: binary1248 on October 09, 2015, 04:10:26 am
This solves many problems/annoyances without actually changing the behaviour of the library for anybody who follows the official usage guidelines/documentation and doesn't rely on undocumented/undefined behaviour.
I guess I wasn't clear enough in my original post.
Title: Re: Experimental internal-context-less branch
Post by: Arcade on October 09, 2015, 05:36:48 pm
I believe this thread (http://en.sfml-dev.org/forums/index.php?topic=16448.0) from last year may be relevant to give some background on how SFML uses contexts.
Title: Re: Experimental internal-context-less branch
Post by: Jesper Juhl on October 09, 2015, 07:27:16 pm
Looks like it has potential. Nice work binary1248.
Title: Re: Experimental internal-context-less branch
Post by: Nexus on October 09, 2015, 07:58:36 pm
Great work indeed! 8)

There are many threads about this limitation... I hope the feature branch gets enough exposure and is actually tested by some people!
Title: Re: Experimental internal-context-less branch
Post by: Gambit on October 10, 2015, 05:41:18 am
I'd like to help with testing but I'm unsure how to actually test this. Can someone offer some suggestions like unit tests or something?
Title: Re: Experimental internal-context-less branch
Post by: Nexus on October 10, 2015, 10:44:21 am
I think the most promising way is to download binary1248's repository, build it and use the installed SFML library in your projects. Especially if you're actively developing one, you'll see relatively quickly whether things function :)
Title: Re: Experimental internal-context-less branch
Post by: Satus on October 10, 2015, 01:00:48 pm
I want to test it, but I'm getting an error when building graphics module.

MinGW w64 5.0, Cmake 3.3, Windows 10, building from Clion IDE, all targets.

Full compiler output:

(click to show/hide)
Title: Re: Experimental internal-context-less branch
Post by: Laurent on October 10, 2015, 02:57:59 pm
TransientContextLock should be declared as

class SFML_WINDOW_API TransientContextLock

(in GlResource.hpp)

Simple enough to add yourself, if you want to test before binary1248 fixes it ;)
Title: Re: Experimental internal-context-less branch
Post by: Satus on October 10, 2015, 03:43:54 pm
Yep, now it compiles and works with well for me  :)
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on October 10, 2015, 04:30:25 pm
Fixed. ;D
Title: Re: Experimental internal-context-less branch
Post by: Ceylo on October 11, 2015, 07:56:01 pm
Quote from: binary1248 on October 09, 2015, 10:45:40 am
I guess I wasn't clear enough in my original post.
I guess I should really not post right after waking up ::)
Going to test your branch.

Edit: no trouble with sfeMovie. It only uses an OpenGL context on main thread though, so it doesn't test the annoying case.
Title: Re: Experimental internal-context-less branch
Post by: Jabberwocky on October 11, 2015, 09:27:02 pm
Cool.
I recall being bit once by some SFML context-related surprises.  Whether or not this change fixes that particular problem, I'm all for this change.  Especially if it makes the library more useful to advanced users, as suggested on this thread (http://en.sfml-dev.org/forums/index.php?topic=16448.0).  Even more impressive if it doesn't affect current users or the API at all.
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on October 13, 2015, 03:50:33 pm
I just moved the branch to the main repository and updated/fixed a few things. The new version should minimize the number of temporary contexts created during the lifetime of an application as well. If you make sure to create an OpenGL resource (Texture, Shader, Window, etc.) before calling any of Shader's or Texture's static member functions, the total number of contexts created should be (number of RenderTargets or Windows) + 1. In a typical single window application that doesn't use RenderTextures, this means that only 2 contexts will ever be created.

As before, it would be nice if people could test this branch in their projects and report any behaviour that is different from the current master revision or release of SFML.

https://github.com/SFML/SFML/tree/feature/no_internal_context
Title: Re: Experimental internal-context-less branch
Post by: Laurent on October 13, 2015, 04:32:24 pm
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?
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on October 13, 2015, 05:01:10 pm
Quote from: Laurent on October 13, 2015, 04:32:24 pm
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.

Quote from: Laurent on October 13, 2015, 04:32:24 pm
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.

Quote from: Laurent on October 13, 2015, 04:32:24 pm
Why don't you create a PR?
All in good time. ;)
Title: Re: Experimental internal-context-less branch
Post by: Laurent on October 13, 2015, 06:02:48 pm
Quote
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?
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on October 13, 2015, 09:30:04 pm
Quote from: Laurent on October 13, 2015, 06:02:48 pm
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 (https://github.com/SFML/SFML/commit/6b50691551da7c76b69f626b86056bb40a1fbf9a), so you should know best. Although to me, the locations look pretty random. ;D
Title: Experimental internal-context-less branch
Post by: eXpl0it3r on October 13, 2015, 10:04:31 pm
The last thing I remember with glFlush was about font rendering from a separate thread... ;)
Title: Re: Experimental internal-context-less branch
Post by: Laurent on October 13, 2015, 10:13:40 pm
Quote
Well... you introduced glFlush() in multiple places in 6b50691, so you should know best.
Don't overestimate me 8)

Quote
Although to me, the locations look pretty random
Quote
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 :-[
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on October 13, 2015, 10:59:39 pm
Maybe #411 (https://github.com/SFML/SFML/issues/411) and this thread (http://en.sfml-dev.org/forums/index.php?topic=10344.15) 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. ;D

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.
Title: Re: Experimental internal-context-less branch
Post by: Laurent on October 13, 2015, 11:23:13 pm
Quote
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.
Title: Re: Experimental internal-context-less branch
Post by: Elias Daler on October 14, 2015, 06:40:20 am
Sometimes I just imagine binary1248 to be Dexter sitting in his secret lab coming up with great things... :D

This is really great and creating RenderTextures in a separate thread is something which I wanted to do for a long time, he-he.
Title: Re: Experimental internal-context-less branch
Post by: Nexus on October 14, 2015, 09:32:20 am
Quote from: Laurent on October 13, 2015, 06:02:48 pm
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 :D
Title: Re: Experimental internal-context-less branch
Post by: BlueCobold on October 21, 2015, 12:19:07 pm
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
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on November 14, 2015, 03:13:12 pm
Rebased onto current master and created pull request #1002 (https://github.com/SFML/SFML/pull/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.
Title: Re: Experimental internal-context-less branch
Post by: eXpl0it3r on January 15, 2016, 12:00:27 pm
Any updates on this? BlueCobold do you have an example for binary1248?
Title: Re: Experimental internal-context-less branch
Post by: BlueCobold on January 15, 2016, 12:33:31 pm
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.
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on January 15, 2016, 12:37:01 pm
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.
Title: Re: Experimental internal-context-less branch
Post by: ekun on April 18, 2016, 10:50:50 pm
I tried out this branch, and I've run into a deadlock when creating two RenderTextures in different threads. Here's my attempt of a minimal example that recreates the issue on my machine:

#include <SFML/Graphics.hpp>
void createSomeTextures()
{
    for (unsigned i = 0; i < 100; i++)
    {
        sf::RenderTexture texture;
        texture.create(100, 100);
        printf("Thread: %d\n", i);
    }
}
int main(int argc, char** argv)
{
    sf::Thread thread(&createSomeTextures);
    thread.launch();

    for (unsigned i = 0; i < 100; i++)
    {
        sf::RenderTexture texture;
        texture.create(100, 100);
        printf("Main: %d\n", i);
    }
    thread.wait();
    return 0;
}

Apologies if this has already been addressed elsewhere.
Title: Re: Experimental internal-context-less branch
Post by: eXpl0it3r on April 18, 2016, 11:46:08 pm
Can not reproduce here. What's your specs etc?
Title: Re: Experimental internal-context-less branch
Post by: ekun on April 18, 2016, 11:53:24 pm
Compiled using MSVC 2015 on Windows 10.

Main call stack:
Code: [Select]
> sfml-system-d-2.dll!sf::priv::MutexImpl::lock() Line 52 C++
  sfml-system-d-2.dll!sf::Mutex::lock() Line 57 C++
  sfml-system-d-2.dll!sf::Lock::Lock(sf::Mutex & mutex) Line 39 C++
  sfml-window-d-2.dll!sf::priv::GlContext::setActive(bool active) Line 427 C++
  sfml-window-d-2.dll!sf::priv::GlContext::releaseTransientContext() Line 253 C++
  sfml-window-d-2.dll!sf::GlResource::TransientContextLock::~TransientContextLock() Line 101 C++
  sfml-graphics-d-2.dll!sf::Texture::~Texture() Line 100 C++
  sfml-graphics-d-2.dll!sf::RenderTexture::~RenderTexture() Line 48 C++
  stand.exe!main(int argc, char * * argv) Line 24 C++

Other thread callstack:
Code: [Select]
> sfml-system-d-2.dll!sf::priv::MutexImpl::lock() Line 52 C++
  sfml-system-d-2.dll!sf::Mutex::lock() Line 57 C++
  sfml-system-d-2.dll!sf::Lock::Lock(sf::Mutex & mutex) Line 39 C++
  sfml-window-d-2.dll!sf::priv::GlContext::create() Line 272 C++
  sfml-window-d-2.dll!sf::Context::Context() Line 44 C++
  sfml-graphics-d-2.dll!sf::priv::RenderTextureImplFBO::create(unsigned int width, unsigned int height, unsigned int textureId, bool depthBuffer) Line 88 C++
  sfml-graphics-d-2.dll!sf::RenderTexture::create(unsigned int width, unsigned int height, bool depthBuffer) Line 81 C++
  stand.exe!createSomeTextures() Line 9 C++
  stand.exe!sf::priv::ThreadFunctor<void (__cdecl*)(void)>::run() Line 39 C++
  sfml-system-d-2.dll!sf::Thread::run() Line 83 C++
  sfml-system-d-2.dll!sf::priv::ThreadImpl::entryPoint(void * userData) Line 86 C++

The problematic sections of code seem to be:
GlContext.cpp: Line 264

    Lock lock(mutex);

    GlContext* context = NULL;

    {
        Lock sharedContextLock(sharedContextMutex); // The thread waits until sharedContextMutex.unlock() is called.
 

GlContext.cpp: Line 252
    sharedContext->setActive(false); // This calls Lock lock(mutex), which blocks because other thread has it locked already
    sharedContextLocked = false;
    sharedContextMutex.unlock();
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on April 20, 2016, 02:55:37 am
After going over the code with the thread sanitizer, I modified the implementation to make the locking more order independent. Build the latest version of the branch and see if it solves your issue.
Title: Re: Experimental internal-context-less branch
Post by: ekun on April 20, 2016, 03:11:57 am
Thanks for the update. Trying out the updated branch, I still run into a deadlock, but in a different part of the code.

Here's an updated minimal example that reproduces the issue on my machine:
#include <SFML/Graphics.hpp>
void createSomeTextures()
{
    for (unsigned i = 0; i < 100; i++)
    {
        sf::RenderTexture texture;
        texture.create(100, 100);
        sf::RectangleShape shape({ 50, 50 });
        texture.draw(shape);
        printf("Thread: %d\n", i);
    }
}
int main(int argc, char** argv)
{
    sf::Thread thread(&createSomeTextures);
    thread.launch();

    for (unsigned i = 0; i < 100; i++)
    {
        sf::RenderTexture texture;
        texture.create(100, 100);
        sf::RectangleShape shape({ 50, 50 });
        texture.draw(shape);
        printf("Main: %d\n", i);
    }
    thread.wait();
    return 0;
}

And the locked callstack:
Code: [Select]
> sfml-system-d-2.dll!sf::priv::MutexImpl::lock() Line 52 C++
  sfml-system-d-2.dll!sf::Mutex::lock() Line 57 C++
  sfml-system-d-2.dll!sf::Lock::Lock(sf::Mutex & mutex) Line 39 C++
  sfml-window-d-2.dll!sf::priv::GlContext::setActive(bool active) Line 423 C++
  sfml-window-d-2.dll!sf::Context::setActive(bool active) Line 60 C++
  sfml-window-d-2.dll!sf::Context::~Context() Line 53 C++
  [External Code]
  sfml-graphics-d-2.dll!sf::priv::RenderTextureImplFBO::~RenderTextureImplFBO() Line 68 C++
  [External Code]
  sfml-graphics-d-2.dll!sf::RenderTexture::~RenderTexture() Line 47 C++
  stand.exe!createSomeTextures() Line 11 C++
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on April 24, 2016, 12:07:05 pm
Try out the latest version I pushed, should be fixed there.
Title: Re: Experimental internal-context-less branch
Post by: ekun on April 25, 2016, 11:00:14 pm
I'm not able to reproduce any deadlocks with the latest version, nice work.

I'll be doing some more testing of this branch on some non-trivial code today, but it looks good so far for the cases I think of at least.
Title: Re: Experimental internal-context-less branch
Post by: ekun on April 26, 2016, 08:30:49 pm
I ran into another deadlock with this branch. It didn't occur on my machine so I can't reproduce it unfortunately, but if/when I have more details I'll add them.
Title: Re: Experimental internal-context-less branch
Post by: BlueCobold on October 10, 2016, 08:30:30 am
OK, so, since this is live now, I'm getting the following log-output when creating a simple RenderWindow on Android:

E/libEGL(2065): call to OpenGL ES API with no current context (logged once per thread)
    sf::RenderWindow m_screen;
    m_screen.create(sf::VideoMode(width, height, 32), "...", sf::Style::Fullscreen);

If I afterwards try to use glLoadGen, I get no valid return values from the calls (glGetError always returns 0, glGetString(GL_VERSION) returns 0, glGetIntegerv(GL_MAJOR_VERSION, &v) writes nothing to v, etc):
    sf::RenderWindow m_screen;
    m_screen.create(sf::VideoMode(width, height, 32), "...", sf::Style::Fullscreen);
    m_screen.setActive(true);
    gl::sys::LoadFunctions();
    int maxTextureSize = 0;
    gl::GetIntegerv(gl::MAX_TEXTURE_SIZE, &maxTextureSize);

If, however, I create a RenderTexture after creating the window and before using glLoadGen, glLoadGen works properly, but I'm still getting the error log about missing GL-context. Looks like there's something broken now.
Title: Re: Experimental internal-context-less branch
Post by: binary1248 on October 11, 2016, 01:08:32 am
I tried to reproduce the problem you described. I don't get any output in the debug console. An Android maintainer will have to look into this. It seems like there are still a few things bugged in the Android implementation that weren't obvious before.
Title: Re: Experimental internal-context-less branch
Post by: BlueCobold on November 01, 2016, 01:40:44 pm
Having the same issue on Windows too since this merge (the issue of glLoadGen being broken, not the issue of missing context). However, the workaround  of using a RenderTexture does not help. glLoadGen returns only 0 on Windows. That's true for gl::MAX_TEXTURE_SIZE, gl::VERSION and gl::EXTENSIONS.

I'm having the very same issues using the default gl-Implementations (meaning not glLoadGen, but simply importing SFML/OpenGL.hpp) like "glGetString" or "glGetIntegerv". Does the provided SFML-OpenGL-example still work?
I'm getting this on Windows:
Failed to activate the window's context
Failed to create texture, its internal size is too high (20x20, maximum is 0x0)
Failed to create texture, its internal size is too high (2048x2048, maximum is 0x0)
    sf::RenderWindow m_screen;
    m_screen.create(sf::VideoMode(width, height, 32), "...", sf::Style::Fullscreen);
    m_screen.setActive(true);

    sf::RenderTexture rt;
    rt.create(20, 20, false);
    sf::Texture t;
    t.loadFromFile(filePath);

Is there any minimal example how to successfully use glLoadGen after this feature had been implemented? Or is SFML only broken for me?