From 91cd907c4e30a44a240eb0d428a89bd81d77a26e Mon Sep 17 00:00:00 2001 From: krcroft Date: Sun, 23 Feb 2020 20:23:22 -0800 Subject: [PATCH] Migrate some members of the Player class to smart pointers 1. Moves the mutex and mixer channel members to unique pointers 2. Moves the trackFile to a weak pointer 3. Move member initialization to the class definition This class still retains a raw *cd member, however fixing this ripples up to the array of [26] CD images, and across more code that generically deals with mount points; so this work remains to do. --- include/mixer.h | 12 ++++++ include/support.h | 16 +++++++ src/dos/cdrom.h | 25 +++++------ src/dos/cdrom_image.cpp | 93 +++++++++++++---------------------------- 4 files changed, 70 insertions(+), 76 deletions(-) diff --git a/include/mixer.h b/include/mixer.h index d352a804..a2aff129 100644 --- a/include/mixer.h +++ b/include/mixer.h @@ -106,6 +106,18 @@ MixerChannel * MIXER_FindChannel(const char * name); /* Find the device you want to delete with findchannel "delchan gets deleted" */ void MIXER_DelChannel(MixerChannel* delchan); +// A smart pointer deleter for MixerChannel objects +// Use-case: +// std::unique_ptr myChannel; +// myChannel.reset(MIXER_AddChannel(...)); +// +struct MixerChannelDeleter { + void operator()(MixerChannel *channel) { + if (channel) + MIXER_DelChannel(channel); + } +}; + /* Object to maintain a mixerchannel; As all objects it registers itself with create * and removes itself when destroyed. */ class MixerObject{ diff --git a/include/support.h b/include/support.h index 65a33c4a..e5c69cd7 100644 --- a/include/support.h +++ b/include/support.h @@ -27,11 +27,27 @@ #include #include +#include + #ifdef _MSC_VER #define strcasecmp(a, b) _stricmp(a, b) #define strncasecmp(a, b, n) _strnicmp(a, b, n) #endif +// A smart-pointer deleter for SDL-Mutex objects. +// Use-case example: +// std::unique_ptr myMutex; +// myMutex.reset(SDL_CreateMutex()); +// +struct SdlMutexDeleter { + void operator()(SDL_mutex* mutex) { + if (mutex) + SDL_DestroyMutex(mutex); + } +}; + +// Returns the filename with the prior path stripped. +// Works with both \ and / directory delimeters. std::string get_basename(const std::string& filename); // Include a message in assert, similar to static_assert: diff --git a/src/dos/cdrom.h b/src/dos/cdrom.h index 32d2f3e6..6e823be8 100644 --- a/src/dos/cdrom.h +++ b/src/dos/cdrom.h @@ -32,6 +32,7 @@ #include #include "dosbox.h" +#include "support.h" #include "mem.h" #include "mixer.h" #include "../libs/decoders/SDL_sound.h" @@ -220,18 +221,18 @@ public: private: static struct imagePlayer { - Bit16s buffer[MIXER_BUFSIZE * 2]; // 2 channels (max) - SDL_mutex *mutex; - TrackFile *trackFile; - MixerChannel *channel; - CDROM_Interface_Image *cd; - void (MixerChannel::*addFrames) (Bitu, const Bit16s*); - uint32_t startSector; - uint32_t totalRedbookFrames; - uint64_t playedTrackFrames; - uint64_t totalTrackFrames; - bool isPlaying; - bool isPaused; + Bit16s buffer[MIXER_BUFSIZE * 2] = {0}; + std::unique_ptr mutex = nullptr; + std::unique_ptr channel = nullptr; + std::weak_ptr trackFile; + CDROM_Interface_Image *cd = nullptr; + void (MixerChannel::*addFrames) (Bitu, const Bit16s*) = nullptr; + uint64_t playedTrackFrames = 0; + uint64_t totalTrackFrames = 0; + uint32_t startSector = 0; + uint32_t totalRedbookFrames = 0; + bool isPlaying = false; + bool isPaused = false; } player; // Private utility functions diff --git a/src/dos/cdrom_image.cpp b/src/dos/cdrom_image.cpp index dd1eca7c..1ccd220a 100644 --- a/src/dos/cdrom_image.cpp +++ b/src/dos/cdrom_image.cpp @@ -271,20 +271,7 @@ int CDROM_Interface_Image::AudioFile::getLength() // initialize static members int CDROM_Interface_Image::refCount = 0; CDROM_Interface_Image* CDROM_Interface_Image::images[26] = {}; -CDROM_Interface_Image::imagePlayer CDROM_Interface_Image::player = { - {0}, // buffer[] - nullptr, // SDL_Mutex* - nullptr, // TrackFile* - nullptr, // MixerChannel* - nullptr, // CDROM_Interface_Image* - nullptr, // addFrames - 0, // startSector - 0, // totalRedbookFrames - 0, // playedTrackFrames - 0, // totalTrackFrames - false, // isPlaying - false // isPaused -}; +CDROM_Interface_Image::imagePlayer CDROM_Interface_Image::player; CDROM_Interface_Image::CDROM_Interface_Image(Bit8u _subUnit) : tracks({}), @@ -293,17 +280,13 @@ CDROM_Interface_Image::CDROM_Interface_Image(Bit8u _subUnit) { images[subUnit] = this; if (refCount == 0) { - if (player.mutex == nullptr) { - player.mutex = SDL_CreateMutex(); - } - if (player.channel == nullptr) { - // channel is kept dormant except during cdrom playback periods - player.channel = MIXER_AddChannel(&CDAudioCallBack, 0, "CDAUDIO"); - player.channel->Enable(false); + player.mutex.reset(SDL_CreateMutex()); + // channel is kept dormant except during cdrom playback periods + player.channel.reset(MIXER_AddChannel(&CDAudioCallBack, 0, "CDAUDIO")); + player.channel->Enable(false); #ifdef DEBUG LOG_MSG("CDROM: Initialized the CDAUDIO mixer channel and mutex"); #endif - } } refCount++; } @@ -311,34 +294,18 @@ CDROM_Interface_Image::CDROM_Interface_Image(Bit8u _subUnit) CDROM_Interface_Image::~CDROM_Interface_Image() { refCount--; + + // Ensure the global player and mixer states are stopped + // before we drop our last CD drive. + if (refCount == 0 && player.cd) { + StopAudio(); + } if (player.cd == this) { player.cd = nullptr; } - - // Empty our tracks and the now-invalid player pointer - tracks.clear(); - player.trackFile = nullptr; - - // Note: the player's channel and mutex objects are - // useable for all CDROM drives, therefore we leave - // them until we have no more CDROM drives left (when - // refCount falls to zero), at which point we finally - // purge them. - // - if (refCount == 0) { - StopAudio(); - if (player.channel != nullptr) { - MIXER_DelChannel(player.channel); - player.channel = nullptr; - } - if (player.mutex != nullptr) { - SDL_DestroyMutex(player.mutex); - player.mutex = nullptr; - } #ifdef DEBUG - LOG_MSG("CDROM: Released the CDAUDIO mixer channel and mutex"); + LOG_MSG("CDROM: Released and cleared resources"); #endif - } } void CDROM_Interface_Image::InitNewMedia() @@ -445,8 +412,8 @@ bool CDROM_Interface_Image::GetAudioSub(unsigned char& attr, if (!tracks.empty()) { // We have a useable CD; get a valid play-position track_iter track = tracks.begin(); // the CD's current track is valid - if (player.trackFile) { - const uint32_t sample_rate = player.trackFile->getRate(); + if (player.trackFile.lock()) { + const uint32_t sample_rate = player.trackFile.lock()->getRate(); const uint32_t played_frames = (player.playedTrackFrames * REDBOOK_FRAMES_PER_SECOND + sample_rate - 1) / sample_rate; @@ -552,7 +519,7 @@ bool CDROM_Interface_Image::PlayAudioSector(uint64_t start, uint64_t len) + clamp(relative_start, 0, static_cast(track->length - 1)) * track->sectorSize); - TrackFile* trackFile = track->file.get(); + std::shared_ptr trackFile(track->file); // Guard: Bail if our track could not be seeked if (!trackFile->seek(offset)) { @@ -571,7 +538,7 @@ bool CDROM_Interface_Image::PlayAudioSector(uint64_t start, uint64_t len) // Before we update our player object with new track details, we lock access // to it to prevent the Callback (which runs in a separate thread) from // getting inconsistent or partial values. - if (SDL_LockMutex(player.mutex) < 0) { + if (SDL_LockMutex(player.mutex.get()) < 0) { LOG_MSG("CDROM: PlayAudioSector couldn't lock our player for exclusive access"); StopAudio(); return false; @@ -632,7 +599,7 @@ bool CDROM_Interface_Image::PlayAudioSector(uint64_t start, uint64_t len) player.channel->Enable(true); // Guard: release the lock in this data - if (SDL_UnlockMutex(player.mutex) < 0) { + if (SDL_UnlockMutex(player.mutex.get()) < 0) { LOG_MSG("CDROM: PlayAudioSector couldn't unlock this thread"); StopAudio(); return false; @@ -667,7 +634,7 @@ bool CDROM_Interface_Image::StopAudio(void) void CDROM_Interface_Image::ChannelControl(TCtrl ctrl) { // Guard: Bail if our mixer channel hasn't been allocated - if (player.channel == nullptr) { + if (!player.channel) { #ifdef DEBUG LOG_MSG("CDROM: ChannelControl => game tried applying channel controls " "before playing audio"); @@ -811,34 +778,36 @@ void CDROM_Interface_Image::CDAudioCallBack(Bitu desired_track_frames) if (desired_track_frames == 0 || !player.cd || !player.mutex - || !player.trackFile) { + || player.trackFile.expired()) { #ifdef DEBUG LOG_MSG("CDROM: CDAudioCallBack called with one more empty dependencies:\n" "\t - frames to play (%" PRIuPTR ")\n" "\t - pointer to the CD object (%p)\n" "\t - pointer to the mutex object (%p)\n" - "\t - pointer to the track's file (%p)\n", + "\t - pointer to the track's file (%s)\n", desired_track_frames, player.cd, - player.mutex, - player.trackFile); + player.mutex.get(), + player.trackFile.expired() ? "nullptr" : "valid" ); #endif + if (player.cd) + player.cd->StopAudio(); return; } // Ensure we have exclusive access to update our player members - if (SDL_LockMutex(player.mutex) < 0) { + if (SDL_LockMutex(player.mutex.get()) < 0) { LOG_MSG("CDROM: CDAudioCallBack couldn't lock this thread"); return; } const uint64_t decoded_track_frames = - player.trackFile->decode(player.buffer, desired_track_frames); + player.trackFile.lock()->decode(player.buffer, desired_track_frames); player.playedTrackFrames += decoded_track_frames; // uses either the stereo or mono and native or // nonnative AddSamples call assigned during construction - (player.channel->*player.addFrames)(decoded_track_frames, player.buffer); + (player.channel.get()->*player.addFrames)(decoded_track_frames, player.buffer); if (player.playedTrackFrames >= player.totalTrackFrames) { #ifdef DEBUG @@ -860,23 +829,21 @@ void CDROM_Interface_Image::CDAudioCallBack(Bitu desired_track_frames) + played_redbook_frames; const Bit32u remaining_redbook_frames = player.totalRedbookFrames - played_redbook_frames; - if (SDL_UnlockMutex(player.mutex) < 0) { + if (SDL_UnlockMutex(player.mutex.get()) < 0) { LOG_MSG("CDROM: CDAudioCallBack couldn't unlock to move to next track"); return; } player.cd->PlayAudioSector(new_redbook_start_frame, remaining_redbook_frames); return; } - if (SDL_UnlockMutex(player.mutex) < 0) { + if (SDL_UnlockMutex(player.mutex.get()) < 0) { LOG_MSG("CDROM: CDAudioCallBack couldn't unlock our player before returning"); } } bool CDROM_Interface_Image::LoadIsoFile(char* filename) { - // Empty our tracks and the now-invalid player pointer tracks.clear(); - player.trackFile = nullptr; // data track (track 1) Track track; @@ -959,9 +926,7 @@ static string dirname(char * file) { bool CDROM_Interface_Image::LoadCueSheet(char *cuefile) { - // Empty our tracks and the now-invalid player pointer tracks.clear(); - player.trackFile = nullptr; Track track; int shift = 0;