Remove sound default constructor

[kimci86]

Description

This PR is related to the issue #2507.

By removing Sound default constructor, we make sure it is always bound to some SoundBuffer.

Doing some operations on an OpenAL sound buffer while OpenAL Sources are attached to it triggers error messages, so we have to keep track of Sound instances which are bound to a given SoundBuffer so that we can detach and attach sounds when needed.

Tasks

• Tested on Linux
• Tested on Windows
• Tested on macOS
• Tested on iOS
• Tested on Android

How to test this PR?

• Running SFML audio examples voip, sound, sound_capture and tennis.
• Playing around with the following program:

#include <SFML/Graphics.hpp>
#include <SFML/Audio.hpp>

int main()
{
    auto window = sf::RenderWindow{sf::VideoMode{{200, 200}}, "Test"};
    window.setFramerateLimit(60);

    auto buffer1 = sf::SoundBuffer{};
    if (!buffer1.loadFromFile("resources/jingles_SAX02.ogg"))
        return EXIT_FAILURE;

    auto buffer2 = sf::SoundBuffer{};
    if (!buffer2.loadFromFile("resources/jingles_SAX07.ogg"))
        return EXIT_FAILURE;

    auto buffer = buffer1;

    auto sound1 = sf::Sound{buffer};
    auto sound2 = sound1;
    sound2.setPitch(0.5f);

    while (window.isOpen())
    {
        for (auto event = sf::Event{}; window.pollEvent(event);)
        {
            switch (event.type)
            {
                case sf::Event::Closed:
                    window.close();
                    break;
                case sf::Event::KeyPressed:
                    switch (event.key.scancode)
                    {
                        case sf::Keyboard::Scan::Num1:
                            sound1.play();
                            break;
                        case sf::Keyboard::Scan::Num2:
                            sound2.play();
                            break;
                        case sf::Keyboard::Scan::Q:
                            buffer = buffer1;
                            break;
                        case sf::Keyboard::Scan::W:
                            buffer = buffer2;
                            break;
                        case sf::Keyboard::Scan::E:
                        {
                            auto buffer3 = sf::SoundBuffer{};
                            if (!buffer3.loadFromFile("resources/jingles_SAX03.ogg"))
                                return EXIT_FAILURE;
                            buffer = buffer3;
                            break;
                        }
                        case sf::Keyboard::Scan::A:
                            sound1.setBuffer(buffer1);
                            sound2 = sound1;
                            sound2.setPitch(0.5f);
                            break;
                        case sf::Keyboard::Scan::S:
                            sound1.setBuffer(buffer);
                            sound2 = sound1;
                            sound2.setPitch(0.5f);
                            break;
                        default:
                            break;
                    }
                    break;
                default:
                    break;
            }
        }

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

resources.zip
Sounds are from kenney.nl

[codecov]

Codecov Report

Merging #2640 (6ef894e) into master (60dbed7) will increase coverage by 0.43%.
Report is 24 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2640      +/-   ##
==========================================
+ Coverage   30.26%   30.69%   +0.43%     
==========================================
  Files         229      229              
  Lines       19748    19740       -8     
  Branches     4723     4718       -5     
==========================================
+ Hits         5976     6060      +84     
- Misses      13046    13055       +9     
+ Partials      726      625     -101     

Files Changed	Coverage Δ
src/SFML/Audio/Sound.cpp	0.00% <0.00%> (ø)
src/SFML/Audio/SoundBuffer.cpp	0.00% <0.00%> (ø)

... and 55 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60dbed7...6ef894e. Read the comment docs.

[kimci86]

I added a test program. I found no issue playing around with it.

[ChrisThrasher]

cmake_minimum_required(VERSION 3.22)
project(audio-test CXX)

include(FetchContent)
FetchContent_Declare(SFML
    GIT_REPOSITORY https://github.com/kimci86/SFML.git
    GIT_TAG feature/remove_sound_default_constructor)
FetchContent_MakeAvailable(SFML)

add_executable(audio-test main.cpp)
target_link_libraries(audio-test PRIVATE SFML::Graphics SFML::Audio)

Here's how you can build this branch to run the example program.

I ran this and couldn't notice any oddities so this change appears to hold up to testing.

[eXpl0it3r]

I just inserted this code to the testing example from above:

                case sf::Keyboard::Scan::Num3:
                {
                    auto bufferTemp = sf::SoundBuffer{};
                    if (!bufferTemp.loadFromFile("resources/jingles_SAX03.ogg"))
                        return EXIT_FAILURE;
                    sound2.setBuffer(bufferTemp);
                    sound2.play();
                    break;
                }

In debug mode this fails in the added assert(), but in release mode it plays just fine. 🤔
I wonder whether there shouldn't be harder fail also in release mode, otherwise people might just not care, or we get a lot of "but it works in release".

[ChrisThrasher]

I wonder whether there shouldn't be harder fail also in release mode, otherwise people might just not care, or we get a lot of "but it works in release".

This is getting at how sf::Sound and sf::SoundBuffer properly ensure correct lifetimes are maintained whereas something like sf::Text and sf::Font just let those lifetime issues persist undetected. This is an inconsistency worth digging into at some point in the future before SFML 3's release.

The only way to turn this into a hard failure in a destructor in release mode is to throw an exception or call some variety of program-ending function like std::terminate(). I think a good improvement would be to elevate that assertion into an sf::err message so that those using release builds at least get some indication that something weird is going on. After the log we can assert(false). We already use this pattern elsewhere.

[kimci86]

In debug mode this fails in the added assert(), but in release mode it plays just fine.

I guess in that case, OpenAL detects that the OpenAL buffer is still in use and refuses to delete it, so we are leaking the resource then.
I will implement a stronger fail, thank you for the feedback.

[kimci86]

Here we go. Now debug and release are the same.
I choose to call std::terminate instead of assert to avoid undefined behavior in release mode.
FYI, throwing an exception from a desctructor also calls std::terminate.

Changes have been addressed

[eXpl0it3r]

Not really a deal breaker, but we've duplicated these two lines in three places now.

[kimci86]

I agree this duplication it is not extremely elegant. The motivation here is that only constructors are responsible for setting up the invariant (i.e. sound it attached to a sound buffer), and methods (especially setBuffer) can be simpler by assuming the invariant is true.