From 9c9a810c4152ec6acf3d2720e45f7466c38ef590 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Duval?= Date: Fri, 14 Feb 2020 22:03:32 +0100 Subject: [PATCH] Media Kit: BBufferCache: if not reclaimed, only mark the buffer for deletion hrev53379 clears the buffer cache for disconnected clients, and also delete buffers. This is too early (see #15263, media_addon_server crash), and should only happen after the buffer is recycled. This can be resolved by abusing the fFlags field of BBuffer to mark the buffer for deletion, and mark the buffer to be reclaimed. Some BBuffers don't reside in the SharedBufferList, so we have to mark them as to be reclaimed. For those in the SharedBufferList, call a new RemoveBuffer(), which can check whether the buffer is still to be reclaimed. For reclaimed BBuffers, delete them right away, others can be marked for deletion. fixes #15606 #15263, possibly #15433 Change-Id: I66e94138e7e10a40d4c48e2ac042f816c79f5aab Reviewed-on: https://review.haiku-os.org/c/haiku/+/2245 Reviewed-by: Adrien Destugues Reviewed-by: X512 Reviewed-by: Ryan Leavengood --- headers/private/media/MediaMisc.h | 4 +++ headers/private/media/SharedBufferList.h | 1 + src/kits/media/Buffer.cpp | 7 +++- src/kits/media/BufferCache.cpp | 20 +++++++++-- src/kits/media/SharedBufferList.cpp | 42 ++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 4 deletions(-) diff --git a/headers/private/media/MediaMisc.h b/headers/private/media/MediaMisc.h index b22944c50b..b422b03b4f 100644 --- a/headers/private/media/MediaMisc.h +++ b/headers/private/media/MediaMisc.h @@ -44,6 +44,10 @@ progress_shutdown(int stage, #define MEDIA_SERVER_PORT_NAME "__media_server_port" #define MEDIA_ADDON_SERVER_PORT_NAME "__media_addon_server_port" +#define BUFFER_TO_RECLAIM 0x20000000 +#define BUFFER_MARKED_FOR_DELETION 0x40000000 + + extern const char *B_MEDIA_ADDON_SERVER_SIGNATURE; namespace BPrivate { namespace media { diff --git a/headers/private/media/SharedBufferList.h b/headers/private/media/SharedBufferList.h index 000165db2f..9006491cf3 100644 --- a/headers/private/media/SharedBufferList.h +++ b/headers/private/media/SharedBufferList.h @@ -30,6 +30,7 @@ public: status_t AddBuffer(sem_id groupReclaimSem, const buffer_clone_info& info, BBuffer** buffer); + status_t RemoveBuffer(BBuffer* buffer); // Call AddBuffer and CheckID locked status_t AddBuffer(sem_id groupReclaimSem, diff --git a/src/kits/media/Buffer.cpp b/src/kits/media/Buffer.cpp index 6d1988cb06..1d17a9cfba 100644 --- a/src/kits/media/Buffer.cpp +++ b/src/kits/media/Buffer.cpp @@ -38,6 +38,7 @@ #include #include "MediaDebug.h" +#include "MediaMisc.h" #include "DataExchange.h" #include "SharedBufferList.h" @@ -114,7 +115,11 @@ BBuffer::Recycle() CALLED(); if (fBufferList == NULL) return; - fBufferList->RecycleBuffer(this); + fFlags &= ~BUFFER_TO_RECLAIM; + if ((fFlags & BUFFER_MARKED_FOR_DELETION) != 0) + delete this; + else + fBufferList->RecycleBuffer(this); } diff --git a/src/kits/media/BufferCache.cpp b/src/kits/media/BufferCache.cpp index cbcc6ac7c7..9ec71d6372 100644 --- a/src/kits/media/BufferCache.cpp +++ b/src/kits/media/BufferCache.cpp @@ -14,6 +14,8 @@ #include #include "MediaDebug.h" +#include "MediaMisc.h" +#include "SharedBufferList.h" namespace BPrivate { @@ -41,8 +43,10 @@ BufferCache::GetBuffer(media_buffer_id id, port_id port) return NULL; buffer_cache_entry* existing; - if (fMap.Get(id, existing)) + if (fMap.Get(id, existing)) { + existing->buffer->fFlags |= BUFFER_TO_RECLAIM; return existing->buffer; + } buffer_clone_info info; info.buffer = id; @@ -65,6 +69,7 @@ BufferCache::GetBuffer(media_buffer_id id, port_id port) return NULL; } + buffer->fFlags |= BUFFER_TO_RECLAIM; return buffer; } @@ -76,8 +81,17 @@ BufferCache::FlushCacheForPort(port_id port) while (iterator.HasNext()) { BufferMap::Entry entry = iterator.Next(); if (entry.value.port == port) { - // Delete the buffer - delete entry.value.buffer; + BBuffer* buffer = entry.value.buffer; + bool isReclaimed = (buffer->fFlags & BUFFER_TO_RECLAIM) == 0; + if (isReclaimed && buffer->fBufferList != NULL) + isReclaimed = buffer->fBufferList->RemoveBuffer(buffer) == B_OK; + + if (isReclaimed) + delete buffer; + else { + // mark the buffer for deletion + buffer->fFlags |= BUFFER_MARKED_FOR_DELETION; + } // Then remove it from the map fMap.Remove(iterator); } diff --git a/src/kits/media/SharedBufferList.cpp b/src/kits/media/SharedBufferList.cpp index fed865bbc5..5673874f83 100644 --- a/src/kits/media/SharedBufferList.cpp +++ b/src/kits/media/SharedBufferList.cpp @@ -378,6 +378,48 @@ SharedBufferList::RecycleBuffer(BBuffer* buffer) } +status_t +SharedBufferList::RemoveBuffer(BBuffer* buffer) +{ + CALLED(); + + media_buffer_id id = buffer->ID(); + + if (Lock() != B_OK) + return B_ERROR; + + int32 notRemovedCount = 0; + + for (int32 i = 0; i < fCount; i++) { + // find the buffer id, and remove it in all groups it belongs to + if (fInfos[i].id == id) { + if (!fInfos[i].reclaimed) { + notRemovedCount++; + ERROR("SharedBufferList::RequestBuffer, BBuffer %p, id = %" + B_PRId32 " not reclaimed\n", buffer, id); + DEBUG_ONLY(debugger("buffer not reclaimed")); + continue; + } + fInfos[i].buffer = NULL; + fInfos[i].id = -1; + fInfos[i].reclaim_sem = -1; + } + } + + if (Unlock() != B_OK) + return B_ERROR; + + if (notRemovedCount != 0) { + ERROR("SharedBufferList::RemoveBuffer, BBuffer %p, id = %" B_PRId32 + " not reclaimed\n", buffer, id); + return B_ERROR; + } + + return B_OK; +} + + + /*! Returns exactly \a bufferCount buffers from the group specified via its \a groupReclaimSem if successful. */