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

Rewrite RenderWindow::capture #1001

Merged
merged 1 commit into from Feb 19, 2016

Conversation

Projects
None yet
7 participants
@binary1248
Member

binary1248 commented Nov 12, 2015

Implements what was discussed here: http://en.sfml-dev.org/forums/index.php?topic=19260.0

@binary1248 binary1248 self-assigned this Nov 12, 2015

@binary1248 binary1248 added this to the 2.4 milestone Nov 12, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Nov 12, 2015

Member

It makes this function look rather useless now. It would be more efficient for users to have their texture and just call update + copyToImage whenever they want to capture the window 😛

More seriously, is the performance improvement something that we're sure of? On all kind of platforms/drivers?

Member

LaurentGomila commented Nov 12, 2015

It makes this function look rather useless now. It would be more efficient for users to have their texture and just call update + copyToImage whenever they want to capture the window 😛

More seriously, is the performance improvement something that we're sure of? On all kind of platforms/drivers?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 16, 2015

Member

👍


Old (Release) [µs]:

Old

New (Release) [µs]:

New

Speed up: 1383%

Code used:

#include <SFML/Graphics.hpp>
#include <iostream>
#include <vector>

int main()
{
    sf::RenderWindow window({1024, 768}, "Test");
    window.setFramerateLimit(60);

    sf::Clock cl;
    std::vector<sf::Time> times;

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
            {
                window.close();
            }
            else if(event.type == sf::Event::KeyPressed)
            {
                if(event.key.code == sf::Keyboard::Escape)
                {
                    double average = 0.;
                    for(auto t : times)
                        average += t.asMicroseconds();
                    average /= times.size();
                    std::cout << "AVG: " << average << "\n";
                }
                else
                {
                    cl.restart();
                    sf::Image img = window.capture();
                    sf::Time time = cl.getElapsedTime();
                    times.push_back(time);
                    std::cout << time.asMicroseconds() << "\n";
                }
            }
        }

        window.clear();
        window.display();
    }
}

On all kind of platforms/drivers?

Feel free to test on all the other platforms and drivers...

Member

eXpl0it3r commented Nov 16, 2015

👍


Old (Release) [µs]:

Old

New (Release) [µs]:

New

Speed up: 1383%

Code used:

#include <SFML/Graphics.hpp>
#include <iostream>
#include <vector>

int main()
{
    sf::RenderWindow window({1024, 768}, "Test");
    window.setFramerateLimit(60);

    sf::Clock cl;
    std::vector<sf::Time> times;

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
            {
                window.close();
            }
            else if(event.type == sf::Event::KeyPressed)
            {
                if(event.key.code == sf::Keyboard::Escape)
                {
                    double average = 0.;
                    for(auto t : times)
                        average += t.asMicroseconds();
                    average /= times.size();
                    std::cout << "AVG: " << average << "\n";
                }
                else
                {
                    cl.restart();
                    sf::Image img = window.capture();
                    sf::Time time = cl.getElapsedTime();
                    times.push_back(time);
                    std::cout << time.asMicroseconds() << "\n";
                }
            }
        }

        window.clear();
        window.display();
    }
}

On all kind of platforms/drivers?

Feel free to test on all the other platforms and drivers...

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Nov 16, 2015

Member

Feel free to test on all the other platforms and drivers...

That's not what I meant. The question was more "is this a known technique that we can rely on, or something that we're trying".

Speed up: 1383%

That's huge! 😃
Would be interesting to run the same test, but with a pre-allocated texture and just calling update + copyToImage in the loop. If there is a significant difference, maybe we could just deprecate the function and advise to use Texture::update directly?

Also, little reminder: we should not merge this until it is tested properly on mobile platforms, where Texture::copyToImage is implemented differently.

Member

LaurentGomila commented Nov 16, 2015

Feel free to test on all the other platforms and drivers...

That's not what I meant. The question was more "is this a known technique that we can rely on, or something that we're trying".

Speed up: 1383%

That's huge! 😃
Would be interesting to run the same test, but with a pre-allocated texture and just calling update + copyToImage in the loop. If there is a significant difference, maybe we could just deprecate the function and advise to use Texture::update directly?

Also, little reminder: we should not merge this until it is tested properly on mobile platforms, where Texture::copyToImage is implemented differently.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 16, 2015

Member

is this a known technique that we can rely on, or something that we're trying

Why shouldn't it be?

maybe we could just deprecate the function and advise to use Texture::update directly?

If we deprecate the function, I'd still change the implementation, nobody will lose anything.

we should not merge this until it is tested properly on mobile platforms, where Texture::copyToImage is implemented differently.

The mobile platforms are still experimental. Sure it's nice if we can test the change as soon as possible, but it should not be a reason to hold back the merge.

Member

eXpl0it3r commented Nov 16, 2015

is this a known technique that we can rely on, or something that we're trying

Why shouldn't it be?

maybe we could just deprecate the function and advise to use Texture::update directly?

If we deprecate the function, I'd still change the implementation, nobody will lose anything.

we should not merge this until it is tested properly on mobile platforms, where Texture::copyToImage is implemented differently.

The mobile platforms are still experimental. Sure it's nice if we can test the change as soon as possible, but it should not be a reason to hold back the merge.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Nov 16, 2015

Member

If we deprecate the function, I'd still change the implementation, nobody will lose anything.

👍

Member

zsbzsb commented Nov 16, 2015

If we deprecate the function, I'd still change the implementation, nobody will lose anything.

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 20, 2015

Member

Mark it as deprecated?

  • Yes
  • No
Member

eXpl0it3r commented Nov 20, 2015

Mark it as deprecated?

  • Yes
  • No
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Nov 20, 2015

Member
  • Yes
  • No
Member

binary1248 commented Nov 20, 2015

  • Yes
  • No
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila
Member

LaurentGomila commented Nov 20, 2015

👍

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Nov 20, 2015

Member

Rebased onto master and added SFML_DEPRECATED.

Member

binary1248 commented Nov 20, 2015

Rebased onto master and added SFML_DEPRECATED.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 21, 2015

Member

You should add the \deprecated to the header documentation and briefly explain what to use instead.

Member

eXpl0it3r commented Nov 21, 2015

You should add the \deprecated to the header documentation and briefly explain what to use instead.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Nov 21, 2015

Member

Fixed.

Member

binary1248 commented Nov 21, 2015

Fixed.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Nov 21, 2015

Member

Android 6.0 - armeabi-v7a - Nexus 5

Avg. master: 718866
Avg. feature/capture: 173961

Member

MarioLiebisch commented Nov 21, 2015

Android 6.0 - armeabi-v7a - Nexus 5

Avg. master: 718866
Avg. feature/capture: 173961

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 21, 2015

Member

Has anybody checked whether compilers are able to apply RVO here?

I'm asking because there are still quite a few lines of code necessary to achieve the same behavior as RenderWindow::capture(), and quite a few error sources (like forgetting to set the size before calling update(); what about setActive()?). Additional time needs to be invested into reading documentation.

In my opinion, if we reduce the usability of taking screenshots, we should not deprecate this function.

Member

Bromeon commented Nov 21, 2015

Has anybody checked whether compilers are able to apply RVO here?

I'm asking because there are still quite a few lines of code necessary to achieve the same behavior as RenderWindow::capture(), and quite a few error sources (like forgetting to set the size before calling update(); what about setActive()?). Additional time needs to be invested into reading documentation.

In my opinion, if we reduce the usability of taking screenshots, we should not deprecate this function.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Nov 22, 2015

Member

Has anybody checked whether compilers are able to apply RVO here?

Even if they do, there's still the unnecessary creation of a texture every time the function is called.
And if they don't, it will be "fixed" in SFML 3 when sf::Image supports move semantics.

there are still quite a few lines of code necessary to achieve the same behavior as RenderWindow::capture()

Two lines of code, plus one other in the Resized event handler. If someone requested a new functon to replace 3 lines of user code, I think we all know what the answer would be ;)

If we keep this function, people will just use it and never wonder if they can do something better themselves. And everytime someone asks on the forum about the most efficient way to take a screenshot, we'll say "don't use capture(), use this code instead".

On the other hand, if we deprecate it, the most efficient code will be clearly mentioned in the doc and tutorials and FAQ, and people will quickly get used to it without being confused by this capture() function.

Member

LaurentGomila commented Nov 22, 2015

Has anybody checked whether compilers are able to apply RVO here?

Even if they do, there's still the unnecessary creation of a texture every time the function is called.
And if they don't, it will be "fixed" in SFML 3 when sf::Image supports move semantics.

there are still quite a few lines of code necessary to achieve the same behavior as RenderWindow::capture()

Two lines of code, plus one other in the Resized event handler. If someone requested a new functon to replace 3 lines of user code, I think we all know what the answer would be ;)

If we keep this function, people will just use it and never wonder if they can do something better themselves. And everytime someone asks on the forum about the most efficient way to take a screenshot, we'll say "don't use capture(), use this code instead".

On the other hand, if we deprecate it, the most efficient code will be clearly mentioned in the doc and tutorials and FAQ, and people will quickly get used to it without being confused by this capture() function.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 22, 2015

Member

Two lines of code, plus one other in the Resized event handler. If someone requested a new functon to replace 3 lines of user code, I think we all know what the answer would be ;)

It's more than that. The user has to keep both a sf::Image and sf::Texture instance around, react to Resized events (which is again explained elsewhere) and know how 3 classes interact. It's not a big deal, but still rather complicated for somebody who may only be interested in taking a screenshot once 😉

And I still don't know about the setActive() call. Is it needed for correctness? If not, why does capture() have it?

On the other hand, if we deprecate it, the most efficient code will be clearly mentioned in the doc and tutorials and FAQ, and people will quickly get used to it without being confused by this capture() function.

If we clearly document it, okay. I think we should even include such a code in the RenderWindow::capture() documentation, to make the transition easier.

Member

Bromeon commented Nov 22, 2015

Two lines of code, plus one other in the Resized event handler. If someone requested a new functon to replace 3 lines of user code, I think we all know what the answer would be ;)

It's more than that. The user has to keep both a sf::Image and sf::Texture instance around, react to Resized events (which is again explained elsewhere) and know how 3 classes interact. It's not a big deal, but still rather complicated for somebody who may only be interested in taking a screenshot once 😉

And I still don't know about the setActive() call. Is it needed for correctness? If not, why does capture() have it?

On the other hand, if we deprecate it, the most efficient code will be clearly mentioned in the doc and tutorials and FAQ, and people will quickly get used to it without being confused by this capture() function.

If we clearly document it, okay. I think we should even include such a code in the RenderWindow::capture() documentation, to make the transition easier.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Nov 22, 2015

Member

The user has to keep both a sf::Image and sf::Texture instance around

Just the texture.

react to Resized events

He can also compare the size every time he takes a screenshot, so all the code is kept at the same place and doesn't involve events.

and know how 3 classes interact

He just needs to know 2 functions, which are (or will be, if not) properly documented and explained in the tutorials.

And I still don't know about the setActive() call. Is it needed for correctness? If not, why does capture() have it?

setActive is only needed for raw OpenGL calls. So it's not needed anymore with the new implementation (Texture::update correctly calls it).

If we clearly document it, okay. I think we should even include such a code in the RenderWindow::capture() documentation, to make the transition easier.

👍

Member

LaurentGomila commented Nov 22, 2015

The user has to keep both a sf::Image and sf::Texture instance around

Just the texture.

react to Resized events

He can also compare the size every time he takes a screenshot, so all the code is kept at the same place and doesn't involve events.

and know how 3 classes interact

He just needs to know 2 functions, which are (or will be, if not) properly documented and explained in the tutorials.

And I still don't know about the setActive() call. Is it needed for correctness? If not, why does capture() have it?

setActive is only needed for raw OpenGL calls. So it's not needed anymore with the new implementation (Texture::update correctly calls it).

If we clearly document it, okay. I think we should even include such a code in the RenderWindow::capture() documentation, to make the transition easier.

👍

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 22, 2015

Member

👍 if we add some transition code to the deprecated function's documentation. And so we can remove setActive() from this PR, right?

Member

Bromeon commented Nov 22, 2015

👍 if we add some transition code to the deprecated function's documentation. And so we can remove setActive() from this PR, right?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Nov 22, 2015

Member

And so we can remove setActive() from this PR, right?

Yes, unless I'm missing something.

Member

LaurentGomila commented Nov 22, 2015

And so we can remove setActive() from this PR, right?

Yes, unless I'm missing something.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 15, 2016

Member

Fixed.

Member

binary1248 commented Jan 15, 2016

Fixed.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 16, 2016

Member

Looks good!

docs

Member

eXpl0it3r commented Jan 16, 2016

Looks good!

docs

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 16, 2016

Member

Ignore the build failure for OS X... I'll have to look into why it fails but it's most certainly not related to this PR.

👍

Member

mantognini commented Jan 16, 2016

Ignore the build failure for OS X... I'll have to look into why it fails but it's most certainly not related to this PR.

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 16, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Jan 16, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Rewrite RenderWindow::capture to make use of a single texture transfe…
…r instead of transferring each row of the framebuffer individually.

@eXpl0it3r eXpl0it3r merged commit 043fb83 into master Feb 19, 2016

16 checks passed

debian-gcc-64 Build #75 done.
Details
freebsd-gcc-64 Build #75 done.
Details
osx-clang-universal Build #75 done.
Details
static-analysis Build #75 done.
Details
windows-gcc-471-tdm-32 Build #77 done.
Details
windows-gcc-471-tdm-64 Build #77 done.
Details
windows-gcc-481-tdm-32 Build #77 done.
Details
windows-gcc-481-tdm-64 Build #77 done.
Details
windows-gcc-520-mingw-32 Build #75 done.
Details
windows-gcc-520-mingw-64 Build #77 done.
Details
windows-vc11-32 Build #76 done.
Details
windows-vc11-64 Build #77 done.
Details
windows-vc12-32 Build #76 done.
Details
windows-vc12-64 Build #75 done.
Details
windows-vc14-32 Build #75 done.
Details
windows-vc14-64 Build #77 done.
Details

@eXpl0it3r eXpl0it3r deleted the feature/capture branch Feb 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment