diff --git a/include/support.h b/include/support.h index e5c69cd7..e75fc514 100644 --- a/include/support.h +++ b/include/support.h @@ -34,18 +34,6 @@ #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); diff --git a/src/dos/cdrom.h b/src/dos/cdrom.h index 94630e25..cf06b702 100644 --- a/src/dos/cdrom.h +++ b/src/dos/cdrom.h @@ -221,19 +221,20 @@ public: private: static struct imagePlayer { - std::unique_ptr mutex = nullptr; - std::weak_ptr trackFile; - MixerObject channelManager; - MixerChannel *channel = nullptr; - CDROM_Interface_Image *cd = nullptr; + // Objects, pointers, and then scalars; in descending size-order. + MixerObject mixerChannel = {}; + std::weak_ptr trackFile = {}; + SDL_mutex *mutex = nullptr; + MixerChannel *channel = nullptr; + 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; - int16_t buffer[MIXER_BUFSIZE * REDBOOK_CHANNELS] = {0}; - bool isPlaying = false; - bool isPaused = false; + uint64_t playedTrackFrames = 0; + uint64_t totalTrackFrames = 0; + uint32_t startSector = 0; + uint32_t totalRedbookFrames = 0; + int16_t buffer[MIXER_BUFSIZE * REDBOOK_CHANNELS] = {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 be28d86f..fe2388a8 100644 --- a/src/dos/cdrom_image.cpp +++ b/src/dos/cdrom_image.cpp @@ -280,9 +280,11 @@ CDROM_Interface_Image::CDROM_Interface_Image(Bit8u _subUnit) { images[subUnit] = this; if (refCount == 0) { - player.mutex.reset(SDL_CreateMutex()); + if (!player.mutex) + player.mutex = SDL_CreateMutex(); + if (!player.channel) { - player.channel = player.channelManager.Install(&CDAudioCallBack, 0, "CDAUDIO"); + player.channel = player.mixerChannel.Install(&CDAudioCallBack, 0, "CDAUDIO"); player.channel->Enable(false); // only enabled during playback periods } #ifdef DEBUG @@ -300,6 +302,8 @@ CDROM_Interface_Image::~CDROM_Interface_Image() // before we drop our last CD drive. if (refCount == 0 && player.cd) { StopAudio(); + SDL_DestroyMutex(player.mutex); + player.mutex = nullptr; } if (player.cd == this) { player.cd = nullptr; @@ -413,7 +417,9 @@ 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 - const auto track_file = player.trackFile.lock(); // lock() creates a shared_ptr! + + // reserve the track_file as a shared_ptr to avoid deletion in another thread + const auto track_file = player.trackFile.lock(); if (track_file) { const uint32_t sample_rate = track_file->getRate(); const uint32_t played_frames = (player.playedTrackFrames @@ -489,14 +495,14 @@ bool CDROM_Interface_Image::GetMediaTrayStatus(bool& mediaPresent, bool& mediaCh bool CDROM_Interface_Image::PlayAudioSector(uint64_t start, uint64_t len) { // Find the track that holds the requested sector - track_const_iter track(GetTrack(start)); - std::shared_ptr trackFile; - if (track != tracks.end() && track->file) - trackFile = track->file; + track_const_iter track = GetTrack(start); + std::shared_ptr track_file; + if (track != tracks.end()) + track_file = track->file; // Guard: sanity check the request beyond what GetTrack already checks if (len == 0 - || !trackFile + || !track_file || track->attr == 0x40 || !player.channel || !player.mutex) { @@ -525,7 +531,7 @@ bool CDROM_Interface_Image::PlayAudioSector(uint64_t start, uint64_t len) * track->sectorSize); // Guard: Bail if our track could not be seeked - if (!trackFile->seek(offset)) { + if (!track_file->seek(offset)) { LOG_MSG("CDROM: Track %d failed to seek to byte %u, so cancelling playback", track->number, offset); @@ -534,14 +540,14 @@ bool CDROM_Interface_Image::PlayAudioSector(uint64_t start, uint64_t len) } // Get properties about the current track - const Bit8u track_channels = trackFile->getChannels(); - const uint64_t track_rate = trackFile->getRate(); + const Bit8u track_channels = track_file->getChannels(); + const uint64_t track_rate = track_file->getRate(); // Guard: // 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.get()) < 0) { + if (SDL_LockMutex(player.mutex) < 0) { LOG_MSG("CDROM: PlayAudioSector couldn't lock our player for exclusive access"); StopAudio(); return false; @@ -549,14 +555,14 @@ bool CDROM_Interface_Image::PlayAudioSector(uint64_t start, uint64_t len) // Update our player with properties about this playback sequence player.cd = this; - player.trackFile = trackFile; + player.trackFile = track_file; player.startSector = start; player.totalRedbookFrames = len; player.isPlaying = true; player.isPaused = false; // Assign the mixer function associated with this track's content type - if (trackFile->getEndian() == AUDIO_S16SYS) { + if (track_file->getEndian() == AUDIO_S16SYS) { player.addFrames = track_channels == 2 ? &MixerChannel::AddSamples_s16 \ : &MixerChannel::AddSamples_m16; } else { @@ -602,7 +608,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.get()) < 0) { + if (SDL_UnlockMutex(player.mutex) < 0) { LOG_MSG("CDROM: PlayAudioSector couldn't unlock this thread"); StopAudio(); return false; @@ -777,21 +783,28 @@ bool CDROM_Interface_Image::ReadSector(Bit8u *buffer, bool raw, unsigned long se void CDROM_Interface_Image::CDAudioCallBack(Bitu desired_track_frames) { + /** + * This callback runs in SDL's mixer thread, so there's a risk + * our track_file pointer could be removed by the main thread. + * We reserve the track_file up-front for the scope of this call. + */ + std::shared_ptr track_file = player.trackFile.lock(); + // Guards: Bail if the request or our player is invalid if (desired_track_frames == 0 || !player.cd || !player.mutex - || player.trackFile.expired()) { + || !track_file) { #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 (%s)\n", - desired_track_frames, - player.cd, - player.mutex.get(), - player.trackFile.expired() ? "nullptr" : "valid" ); + "\t - pointer to the track's file (%p)\n", + desired_track_frames, + static_cast(player.cd), + static_cast(player.mutex), + static_cast(track_file.get()); #endif if (player.cd) player.cd->StopAudio(); @@ -799,13 +812,12 @@ void CDROM_Interface_Image::CDAudioCallBack(Bitu desired_track_frames) } // Ensure we have exclusive access to update our player members - if (SDL_LockMutex(player.mutex.get()) < 0) { + if (SDL_LockMutex(player.mutex) < 0) { LOG_MSG("CDROM: CDAudioCallBack couldn't lock this thread"); return; } - const uint64_t decoded_track_frames = - player.trackFile.lock()->decode(player.buffer, desired_track_frames); + const uint64_t decoded_track_frames = track_file->decode(player.buffer, desired_track_frames); player.playedTrackFrames += decoded_track_frames; // uses either the stereo or mono and native or @@ -832,14 +844,14 @@ 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.get()) < 0) { + if (SDL_UnlockMutex(player.mutex) < 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.get()) < 0) { + if (SDL_UnlockMutex(player.mutex) < 0) { LOG_MSG("CDROM: CDAudioCallBack couldn't unlock our player before returning"); } }