From 3f11d351f542e092191ee30c93b5f1c6be7bb837 Mon Sep 17 00:00:00 2001 From: Vlad G Date: Wed, 19 Jul 2023 00:37:38 +0300 Subject: [PATCH 1/9] Buffers are not always recycled automatically Change-Id: Ifc1b59535c34c12a1db11808b0ae6c59337dee20 --- private.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/private.cpp b/private.cpp index 4a6af00..985c898 100644 --- a/private.cpp +++ b/private.cpp @@ -173,6 +173,12 @@ void _DroidMediaBufferQueue::frameAvailable() { if (item.mGraphicBuffer != NULL) { static_cast(slot) = item; + if (slot.droidBuffer.get()) { + // It seems the buffers are not recycled. We're manually releasing this previous slot. + slot.droidBuffer->decStrong(0); + // clear() is left for seeking/video end from clients like gecko-camera to not crash. + } + slot.droidBuffer = new DroidMediaBuffer(slot, this); // Keep the original reference count for ourselves and give one to the buffer_created From 07d149864aa577a848ecc9026429734b347d0ab8 Mon Sep 17 00:00:00 2001 From: Vlad G Date: Mon, 3 Jul 2023 23:30:49 +0300 Subject: [PATCH 2/9] Keep track of pointers still referenced by the client, but release their resources when replaced. --- droidmediabuffer.cpp | 8 +++++++- droidmediacodec.cpp | 2 +- private.cpp | 39 +++++++++++++++++++++++++++++---------- private.h | 8 +++++++- 4 files changed, 44 insertions(+), 13 deletions(-) diff --git a/droidmediabuffer.cpp b/droidmediabuffer.cpp index 64a94eb..f64ae58 100644 --- a/droidmediabuffer.cpp +++ b/droidmediabuffer.cpp @@ -17,6 +17,7 @@ * Authored by: Mohammed Hassan */ +#include #include "droidmediabuffer.h" #include "private.h" @@ -127,7 +128,12 @@ DroidMediaBuffer *droid_media_buffer_create(uint32_t w, uint32_t h, void droid_media_buffer_destroy(DroidMediaBuffer *buffer) { - buffer->decStrong(0); + if (buffer->m_queue) { + buffer->m_queue->releaseBufferResources(buffer); + } else { + ALOGW("Decrementing buffer refs without a queue %" PRIxPTR, (uintptr_t)buffer); + buffer->decStrong(0); + } } void droid_media_buffer_set_user_data(DroidMediaBuffer *buffer, void *data) diff --git a/droidmediacodec.cpp b/droidmediacodec.cpp index 9d75d22..4eec795 100644 --- a/droidmediacodec.cpp +++ b/droidmediacodec.cpp @@ -181,7 +181,7 @@ class Source : public android::MediaSource { for (android::List::iterator iter = m_framesBeingProcessed.buffers.begin(); iter != m_framesBeingProcessed.buffers.end(); iter++) { if (*iter == buffer) { - m_framesBeingProcessed.buffers.erase(iter); + m_framesBeingProcessed.buffers.erase(iter); m_framesBeingProcessed.cond.signal(); m_framesBeingProcessed.lock.unlock(); return; diff --git a/private.cpp b/private.cpp index 985c898..3df48ca 100644 --- a/private.cpp +++ b/private.cpp @@ -16,7 +16,7 @@ * * Authored by: Mohammed Hassan */ - +#include #include "private.h" #include "droidmediabuffer.h" #if (ANDROID_MAJOR == 4 && ANDROID_MINOR < 4) @@ -63,6 +63,7 @@ void DroidMediaBufferQueueListener::onBuffersReleased() } _DroidMediaBufferQueue::_DroidMediaBufferQueue(const char *name) : + m_unslotted({}), m_listener(new DroidMediaBufferQueueListener(this)), m_data(0) { @@ -171,14 +172,17 @@ void _DroidMediaBufferQueue::frameAvailable() { DroidMediaBufferSlot &slot = m_slots[slotIndex(item)]; if (item.mGraphicBuffer != NULL) { - static_cast(slot) = item; - if (slot.droidBuffer.get()) { // It seems the buffers are not recycled. We're manually releasing this previous slot. - slot.droidBuffer->decStrong(0); - // clear() is left for seeking/video end from clients like gecko-camera to not crash. + ALOGW("Releasing only resources, keeping memory %" PRIxPTR, (uintptr_t)slot.droidBuffer.get()); + slot.mGraphicBuffer = 0; + // instead of clear-ing the entire memory which leads to droid_media_buffer_destroy crashing. + releaseBufferResources(slot.droidBuffer.get()); + // Add the buffer to the list of unslotted buffers that droid_media_buffer_destroy will check and don't do anything. + m_unslotted.emplace(slot.droidBuffer.get(), slot.droidBuffer); } + static_cast(slot) = item; slot.droidBuffer = new DroidMediaBuffer(slot, this); // Keep the original reference count for ourselves and give one to the buffer_created @@ -224,18 +228,33 @@ void _DroidMediaBufferQueue::frameAvailable() { } } -void _DroidMediaBufferQueue::buffersReleased() { - for (int i = 0; i < android::BufferQueue::NUM_BUFFER_SLOTS; ++i) { - DroidMediaBufferSlot &slot = m_slots[i]; - slot.droidBuffer.clear(); - slot.mGraphicBuffer = 0; +void _DroidMediaBufferQueue::releaseBufferResources(DroidMediaBuffer *buffer) { + if (m_unslotted.find(buffer) != m_unslotted.end()) { + ALOGI("Unslotted buffer, resources already released %" PRIxPTR, (uintptr_t)buffer); + } else { + buffer->decStrong(0); + buffer->m_buffer.clear(); } +} +void _DroidMediaBufferQueue::buffersReleased() { android::AutoMutex locker(&m_lock); if (m_data) { m_cb.buffers_released(m_data); } + + // Releasing DroidMediaBuffer memory for slots + for (int i = 0; i < android::BufferQueue::NUM_BUFFER_SLOTS; ++i) { + DroidMediaBufferSlot &slot = m_slots[i]; + slot.droidBuffer.clear(); + slot.mGraphicBuffer = 0; + } + // Releasing DroidMediaBuffer memory for unslotted buffers + for (auto& droidBufferPair : m_unslotted) { + droidBufferPair.second.clear(); + } + m_unslotted.clear(); } int _DroidMediaBufferQueue::releaseMediaBuffer(int index, EGLDisplay dpy, EGLSyncKHR fence) { diff --git a/private.h b/private.h index 6cd4881..8a5f4a3 100644 --- a/private.h +++ b/private.h @@ -81,6 +81,8 @@ struct _DroidMediaBufferQueue : public android::RefBase { void buffersReleased(); + void releaseBufferResources(DroidMediaBuffer *buffer); + private: friend class DroidMediaBufferQueueListener; @@ -97,6 +99,10 @@ struct _DroidMediaBufferQueue : public android::RefBase { DroidMediaBufferSlot m_slots[android::BufferQueue::NUM_BUFFER_SLOTS]; + // Storage for buffers that have their resources cleaned up, but the client still holds a raw reference. + // The first part is used to identify the raw pointer from client, the second to actually clean the memory. + std::unordered_map> m_unslotted; + android::sp m_listener; android::Mutex m_lock; @@ -111,5 +117,5 @@ android::sp droid_media_codec_create_encoder_raw(DroidMedi android::sp looper, #endif android::sp src); - + #endif /* DROID_MEDIA_PRIVATE_H */ From ea775e97eba775417d3f91f7fffb0625ff2b94d0 Mon Sep 17 00:00:00 2001 From: Vlad G Date: Thu, 17 Aug 2023 15:54:45 +0300 Subject: [PATCH 3/9] fix missing include Change-Id: I323b20177b4298cf5848266e84bf9d73d9084c6f --- private.cpp | 6 +++--- private.h | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/private.cpp b/private.cpp index 3df48ca..d748684 100644 --- a/private.cpp +++ b/private.cpp @@ -246,9 +246,9 @@ void _DroidMediaBufferQueue::buffersReleased() { // Releasing DroidMediaBuffer memory for slots for (int i = 0; i < android::BufferQueue::NUM_BUFFER_SLOTS; ++i) { - DroidMediaBufferSlot &slot = m_slots[i]; - slot.droidBuffer.clear(); - slot.mGraphicBuffer = 0; + DroidMediaBufferSlot &slot = m_slots[i]; + slot.droidBuffer.clear(); + slot.mGraphicBuffer = 0; } // Releasing DroidMediaBuffer memory for unslotted buffers for (auto& droidBufferPair : m_unslotted) { diff --git a/private.h b/private.h index 8a5f4a3..faaceef 100644 --- a/private.h +++ b/private.h @@ -33,6 +33,7 @@ #if ANDROID_MAJOR >=5 #include #endif +#include struct _DroidMediaBufferQueue; From 4e757ea9b381a7708ba2c1414e808b340e0d868d Mon Sep 17 00:00:00 2001 From: Vlad G Date: Thu, 17 Aug 2023 16:36:13 +0300 Subject: [PATCH 4/9] trying to fix Android 5 and below build Change-Id: I4c576c5d1b44b014a69aa0ab131491cd52e5c678 --- Android.mk | 4 ++++ droidmediabuffer.cpp | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Android.mk b/Android.mk index da69d47..e7f4573 100644 --- a/Android.mk +++ b/Android.mk @@ -118,6 +118,10 @@ ifeq ($(ANDROID_MAJOR),$(filter $(ANDROID_MAJOR),9)) LOCAL_C_INCLUDES += frameworks/av/media/libmediaextractor/include endif +ifeq ($(shell test $(ANDROID_MAJOR) -le 5 && echo true),true) +LOCAL_CPPFLAGS += --std=c++11 +endif + include $(BUILD_SHARED_LIBRARY) include $(CLEAR_VARS) diff --git a/droidmediabuffer.cpp b/droidmediabuffer.cpp index f64ae58..4f9525c 100644 --- a/droidmediabuffer.cpp +++ b/droidmediabuffer.cpp @@ -128,7 +128,7 @@ DroidMediaBuffer *droid_media_buffer_create(uint32_t w, uint32_t h, void droid_media_buffer_destroy(DroidMediaBuffer *buffer) { - if (buffer->m_queue) { + if (buffer->m_queue != NULL) { buffer->m_queue->releaseBufferResources(buffer); } else { ALOGW("Decrementing buffer refs without a queue %" PRIxPTR, (uintptr_t)buffer); From 2b700044f434399172754c23fffa55e831a96407 Mon Sep 17 00:00:00 2001 From: Vlad G Date: Thu, 17 Aug 2023 16:55:57 +0300 Subject: [PATCH 5/9] Fix std parameter Change-Id: I8492f91eca7c8709faae42a2f1be69108c932a3a --- Android.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Android.mk b/Android.mk index e7f4573..d6de225 100644 --- a/Android.mk +++ b/Android.mk @@ -119,7 +119,7 @@ LOCAL_C_INCLUDES += frameworks/av/media/libmediaextractor/include endif ifeq ($(shell test $(ANDROID_MAJOR) -le 5 && echo true),true) -LOCAL_CPPFLAGS += --std=c++11 +LOCAL_CPPFLAGS += -std=c++11 endif include $(BUILD_SHARED_LIBRARY) From 905afccf0668955ea497e8d71ff98b8f0960b835 Mon Sep 17 00:00:00 2001 From: Vlad G Date: Thu, 17 Aug 2023 17:42:28 +0300 Subject: [PATCH 6/9] Additional libs/includes from c++11 on Android 5 and lower Change-Id: Ia25af46b92b6b53a6fa0d262bf89cd10e73add94 --- Android.mk | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Android.mk b/Android.mk index d6de225..99a21c3 100644 --- a/Android.mk +++ b/Android.mk @@ -120,6 +120,8 @@ endif ifeq ($(shell test $(ANDROID_MAJOR) -le 5 && echo true),true) LOCAL_CPPFLAGS += -std=c++11 +LOCAL_SHARED_LIBRARIES += libc++ +LOCAL_C_INCLUDES += external/libcxx/include endif include $(BUILD_SHARED_LIBRARY) From 41fef42812a2ca2eb8fcac7770cb7ccaa008c0a9 Mon Sep 17 00:00:00 2001 From: Vlad G Date: Thu, 17 Aug 2023 18:09:10 +0300 Subject: [PATCH 7/9] ifdef ANDROID_MAJOR > 5 Change-Id: Ife94eb380e2d9488a1dfdec1a2526d926e284c3e --- Android.mk | 6 ------ droidmediabuffer.cpp | 8 +++++--- private.cpp | 9 ++++++++- private.h | 9 ++++++--- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/Android.mk b/Android.mk index 99a21c3..da69d47 100644 --- a/Android.mk +++ b/Android.mk @@ -118,12 +118,6 @@ ifeq ($(ANDROID_MAJOR),$(filter $(ANDROID_MAJOR),9)) LOCAL_C_INCLUDES += frameworks/av/media/libmediaextractor/include endif -ifeq ($(shell test $(ANDROID_MAJOR) -le 5 && echo true),true) -LOCAL_CPPFLAGS += -std=c++11 -LOCAL_SHARED_LIBRARIES += libc++ -LOCAL_C_INCLUDES += external/libcxx/include -endif - include $(BUILD_SHARED_LIBRARY) include $(CLEAR_VARS) diff --git a/droidmediabuffer.cpp b/droidmediabuffer.cpp index 4f9525c..4181f58 100644 --- a/droidmediabuffer.cpp +++ b/droidmediabuffer.cpp @@ -128,12 +128,14 @@ DroidMediaBuffer *droid_media_buffer_create(uint32_t w, uint32_t h, void droid_media_buffer_destroy(DroidMediaBuffer *buffer) { +#if ANDROID_MAJOR > 5 if (buffer->m_queue != NULL) { buffer->m_queue->releaseBufferResources(buffer); - } else { - ALOGW("Decrementing buffer refs without a queue %" PRIxPTR, (uintptr_t)buffer); - buffer->decStrong(0); + return; } + ALOGW("Decrementing buffer refs without a queue %" PRIxPTR, (uintptr_t)buffer); +#endif + buffer->decStrong(0); } void droid_media_buffer_set_user_data(DroidMediaBuffer *buffer, void *data) diff --git a/private.cpp b/private.cpp index d748684..ee3f56a 100644 --- a/private.cpp +++ b/private.cpp @@ -63,7 +63,9 @@ void DroidMediaBufferQueueListener::onBuffersReleased() } _DroidMediaBufferQueue::_DroidMediaBufferQueue(const char *name) : +#if ANDROID_MAJOR > 5 m_unslotted({}), +#endif m_listener(new DroidMediaBufferQueueListener(this)), m_data(0) { @@ -172,6 +174,7 @@ void _DroidMediaBufferQueue::frameAvailable() { DroidMediaBufferSlot &slot = m_slots[slotIndex(item)]; if (item.mGraphicBuffer != NULL) { +#if ANDROID_MAJOR > 5 if (slot.droidBuffer.get()) { // It seems the buffers are not recycled. We're manually releasing this previous slot. ALOGW("Releasing only resources, keeping memory %" PRIxPTR, (uintptr_t)slot.droidBuffer.get()); @@ -181,7 +184,7 @@ void _DroidMediaBufferQueue::frameAvailable() { // Add the buffer to the list of unslotted buffers that droid_media_buffer_destroy will check and don't do anything. m_unslotted.emplace(slot.droidBuffer.get(), slot.droidBuffer); } - +#endif static_cast(slot) = item; slot.droidBuffer = new DroidMediaBuffer(slot, this); @@ -228,6 +231,7 @@ void _DroidMediaBufferQueue::frameAvailable() { } } +#if ANDROID_MAJOR > 5 void _DroidMediaBufferQueue::releaseBufferResources(DroidMediaBuffer *buffer) { if (m_unslotted.find(buffer) != m_unslotted.end()) { ALOGI("Unslotted buffer, resources already released %" PRIxPTR, (uintptr_t)buffer); @@ -236,6 +240,7 @@ void _DroidMediaBufferQueue::releaseBufferResources(DroidMediaBuffer *buffer) { buffer->m_buffer.clear(); } } +#endif void _DroidMediaBufferQueue::buffersReleased() { android::AutoMutex locker(&m_lock); @@ -250,11 +255,13 @@ void _DroidMediaBufferQueue::buffersReleased() { slot.droidBuffer.clear(); slot.mGraphicBuffer = 0; } +#if ANDROID_MAJOR > 5 // Releasing DroidMediaBuffer memory for unslotted buffers for (auto& droidBufferPair : m_unslotted) { droidBufferPair.second.clear(); } m_unslotted.clear(); +#endif } int _DroidMediaBufferQueue::releaseMediaBuffer(int index, EGLDisplay dpy, EGLSyncKHR fence) { diff --git a/private.h b/private.h index faaceef..fdde7f7 100644 --- a/private.h +++ b/private.h @@ -33,8 +33,9 @@ #if ANDROID_MAJOR >=5 #include #endif +#if ANDROID_MAJOR > 5 #include - +#endif struct _DroidMediaBufferQueue; class DroidMediaBufferQueueListener : @@ -82,8 +83,9 @@ struct _DroidMediaBufferQueue : public android::RefBase { void buffersReleased(); +#if ANDROID_MAJOR > 5 void releaseBufferResources(DroidMediaBuffer *buffer); - +#endif private: friend class DroidMediaBufferQueueListener; @@ -100,10 +102,11 @@ struct _DroidMediaBufferQueue : public android::RefBase { DroidMediaBufferSlot m_slots[android::BufferQueue::NUM_BUFFER_SLOTS]; +#if ANDROID_MAJOR > 5 // Storage for buffers that have their resources cleaned up, but the client still holds a raw reference. // The first part is used to identify the raw pointer from client, the second to actually clean the memory. std::unordered_map> m_unslotted; - +#endif android::sp m_listener; android::Mutex m_lock; From 7809bed32aee6f92d85c665fa83e156bc0254bc4 Mon Sep 17 00:00:00 2001 From: Vlad G Date: Thu, 17 Aug 2023 23:08:13 +0300 Subject: [PATCH 8/9] m_queue might be null at end of playback Change-Id: I7b5f6c1b2f142ef46a9cf73c3ccaa33e691b306f --- droidmediabuffer.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/droidmediabuffer.cpp b/droidmediabuffer.cpp index 4181f58..947d601 100644 --- a/droidmediabuffer.cpp +++ b/droidmediabuffer.cpp @@ -133,9 +133,10 @@ void droid_media_buffer_destroy(DroidMediaBuffer *buffer) buffer->m_queue->releaseBufferResources(buffer); return; } - ALOGW("Decrementing buffer refs without a queue %" PRIxPTR, (uintptr_t)buffer); -#endif + ALOGW("Destroying buffer refs but no queue %" PRIxPTR, (uintptr_t)buffer); +#else buffer->decStrong(0); +#endif } void droid_media_buffer_set_user_data(DroidMediaBuffer *buffer, void *data) From f67d7be6210d382beb1bbb8b2c2f23d63f7dc40f Mon Sep 17 00:00:00 2001 From: Vlad G Date: Sun, 20 Aug 2023 00:32:01 +0300 Subject: [PATCH 9/9] Fix playback on ACodec Change-Id: I594c5cecfbf582e15cb20a7e79230897d4882f12 --- private.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private.cpp b/private.cpp index ee3f56a..8a8a8dd 100644 --- a/private.cpp +++ b/private.cpp @@ -179,6 +179,7 @@ void _DroidMediaBufferQueue::frameAvailable() { // It seems the buffers are not recycled. We're manually releasing this previous slot. ALOGW("Releasing only resources, keeping memory %" PRIxPTR, (uintptr_t)slot.droidBuffer.get()); slot.mGraphicBuffer = 0; + slot.droidBuffer->m_buffer.clear(); // instead of clear-ing the entire memory which leads to droid_media_buffer_destroy crashing. releaseBufferResources(slot.droidBuffer.get()); // Add the buffer to the list of unslotted buffers that droid_media_buffer_destroy will check and don't do anything. @@ -237,7 +238,6 @@ void _DroidMediaBufferQueue::releaseBufferResources(DroidMediaBuffer *buffer) { ALOGI("Unslotted buffer, resources already released %" PRIxPTR, (uintptr_t)buffer); } else { buffer->decStrong(0); - buffer->m_buffer.clear(); } } #endif