Separate sound buffer pool from sound manager

[elsid]

Also:

1. Track used buffers.
2. Encapsulate usage counter by a custom type. It's possible to use std::shared_ptr but it will add overhead.
3. Different types for a reference to a used and unused sound buffer.
4. Store sound buffers in the object pool.

[kcat]

What's the point of this class? Sound_Buffer objects should remain valid for the lifetime of the sound manager, and are automatically cleared with it. It's the internal handle, which stores the actual sample data for the output backend, that gets loaded and unloaded as needed.

[elsid]

Sound_Buffer doesn't know what pool does it belong to. This type provides this information. Also it represents a state of Sound_Buffer when it's already allocated but sound is not playing yet. Between this and this line.

And I don't understand what do you suggest instead. I can move this into Sound_Buffer since it already knows about iterator from the pool.

and are automatically cleared with it

Before this change it's handled by SoundManager with code like:

if (sfx->mUses-- == 1)
    mUnusedBuffers.push_front(sfx);

and

if (sfx->mUses++ == 0)
{
    SoundList::iterator iter = std::find(mUnusedBuffers.begin(), mUnusedBuffers.end(), sfx);
    if(iter != mUnusedBuffers.end())
        mUnusedBuffers.erase(iter);
}

that is copy-pasted over multiple places. I don't see why RAII can't be used for internal handles to avoid code duplication and make a usage of Sound_Buffer objects safer.

[kcat]

Sound_Buffer doesn't know what pool does it belong to.

It doesn't need to, since it exists for the life of its pool/allocating container. And when the pool is destroyed, nothing should have a Sound_Buffer.

Before this change it's handled by SoundManager with code like:
...
and
...
that is copy-pasted over multiple places. I don't see why RAII can't be used for internal handles to avoid code duplication and make a usage of Sound_Buffer objects safer.

Private methods can be added to SoundManagerImpl to avoid the duplication. As it is, mUnusedBuffers only exists to expediate unloading of least-recently-used samples buffers by tracking the total sample buffer size and calling to the output backend when needed. And managing which sample buffers to unload, when, and how, seems like the job of the sound manager to me, rather than the pool for Sound_Buffers (which holds an opaque handle to the sample buffers). SoundBufferPool, SoundBufferRef, and SoundBufferSharedRef seems way too over-designed to me and is putting management work where it shouldn't be.

[elsid]

SoundManagerImpl is overwhelmed with logic from a selection of the next music track from a playlist to play to a reference counting. I don't know what are the benefits of doing this. Reference counting for a Sound_Handle and playing/stopping sounds are different responsibilities. Sound manager still manages when to unload the sound by SoundBufferSharedRef destruction.

[kcat]

SoundManagerImpl is overwhelmed with logic from a selection of the next music track from a playlist to play to a reference counting.

The next music track/playlist handling can certainly go into a sub-manager of sorts. I believe the intention is to eventually allow for mod-defined playlists. The sound manager just needs to make sure the right playlist is being used, and ensure a track from that playlist plays. What's currently there is an attempt to randomize tracks while avoiding the same track playing twice, basically a fallback dynamic playlist creator.

The reference counting in Sound_Buffer is so the sound manager knows when a sound buffer is no longer used, and becomes the next candidate for unloading after any previously-made-unused sounds when the cache fills up. On its own, it doesn't do anything except to indicate 'this is now unused' or 'this is now used'. Since the sound manager already handles starting and stopping sounds, and handles the output backend, it's the most appropriate place to say when a sound is no longer in use or is now in use, and to load and unload sounds from the backend as needed.

[elsid]

Removed SoundBufferRef by moving the logic into Sound_Buffer. Also had to move SoundBufferPool and SoundBufferSharedRef to sound_buffer.hpp to save benefits from inlining without putting Sound_Buffer::use into pool header.

[kcat]

I do not think this is a good change. It's moving logic out of where it makes sense into a separate class that complicates the code by creating extra references and inter-dependencies, spreading it out and making it harder to track what's going on. The pool itself shouldn't need to maintain the status and state of opaque data contained in the objects in the pool, nor should it need to be aware of the output and decoder backends.

Additionally, the change of the buffers storage from std::deque to std::list is not good. std::list is not an efficient storage container, particularly for small objects like a single pointer since each one is an independent allocation (a deque handles multiple objects in a single allocation, improving memory efficiency, while still allowing new elements to be pushed onto the back or front without invalidating references to the other pre-existing elements).

[akortunov]

I do not think this is a good change

Is there a conclusion? Should we merge this PR or close it?

[elsid]

I don't agree that a single class should mix low level and high level logic. The idea of type system is to divide and encapsulate logic. So when there is a new unrelated logic or a refactoring no need to care about unrelated level stuff. Like in #2928 I had to be careful with this reference counting stuff. OpenMW is a C++ project and it should take advantage of this. RAII is not just about memory. Sometimes non trivial object is a resource and is needed to be treated accordingly. Anyway I don't want to push this because some parts of this pr I don't like myself, but without any ideas how to make it better I don't want to see it in the master.

[psi29a]

It is something that we can come back to later when a light-bulb goes off.