Implement async callback for Android mediacodec #4105

trengginas · 2024-10-14T05:37:31Z

This is the current pattern for encoding/decoding using Android media codec:

check the input buffer availability
once an input buffer is available, input the data to encode/decode
check the output buffer availability
once the output buffer is available, it is the encoded/decoded data

Issues related to this pattern/when using polling:

We are using polling when checking the input/output buffer availability. This is necessary because the API won't block to wait for the buffer to be available.
A supply to input buffer doesn't always produce from the output buffer. Sometimes it needs to be fed more than one input buffer before the output is available, so it might waste some polling time/no guarantee of an output.
When the poll returns error, the codec might need to be restarted (ref). However, there are instances where this doesn't resolve the issue, and the codec fails to recover even after being re-created.

This patch will use an async callback to the implementation.

Set new_input_buffer_avail() and new_output_buffer_avail() callback.
The callback will notify the index of the input/output buffer available to use. The buffer information will be stored in an atomic queue.
The encode/decode process will only use the buffer information provided from the callback on the queue.
This will eliminate the polling requirements.

Notes:

The patch will use the atomic queue from oboe_dev.cpp
The async callback is supported for Android API level 28 and later.
There's noticeable lag/delay when using the Android MediaCodec backend. Unfortunately, this patch doesn't address the issue.
To completely avoid the lag/delay issue, you can use an alternative video codec backend, such as OpenH264. This solution is also applicable to older devices.

nanangizz · 2024-10-14T05:53:09Z

pjmedia/include/pjmedia/atomic_queue.hpp

+    AtomicQueue() {}
+};
+
+#endif


Headers in include dir are meant to be public API, so by convention it should use a specific prefix such as pj or pjmedia.

If this is public API, IMO, the interface better uses C (as used by the rest), e.g: pj_atomic_queue, and perhaps part of PJLIB instead of PJMEDIA, note that the implementation can still use C++.

nanangizz · 2024-10-15T02:03:44Z

pjlib/include/pj/atomic_queue.h

+    pj_atomic_queue() {}
+};
+
+#endif


This is definitely still C++ (with C naming style :)
Please check ioqueue_symbian.cpp which use ioqueue.h.

nanangizz · 2024-10-15T02:06:11Z

pjlib/src/pj/atomic_queue.cpp

+
+#if defined(PJ_ANDROID) && PJ_ANDROID != 0
+
+//#include <android/log.h>


For tracing purpose, perhaps it can use the usual approach, e.g: using PJLIB log (instead of Android), tracing is by default disabled.

Approving by accident

nanangizz · 2024-10-16T00:24:15Z

pjlib/include/pj/atomic_queue.h

+/* Atomic queue (ring buffer) for single consumer & single producer.
+ *
+ * Producer invokes 'pj_atomic_queue_put(item)' to put an item to the back of
+ * the queue.
+ * Consumer invokes 'pj_atomic_queue_get(item)' to get an item from the head of
+ * the queue.
+ */


This does not seem to be part of the docs?
Note that the "single consumer & single producer" condition needs to be explicitly specified in the docs.

nanangizz · 2024-10-16T00:26:43Z

pjlib/src/pj/atomic_queue.cpp

+PJ_DEF(pj_status_t) pj_atomic_queue_put(pj_atomic_queue_t *atomic_queue,
+                                        void *item)
+{
+    PJ_ASSERT_RETURN(atomic_queue && atomic_queue->aQ, PJ_EINVAL);


Check item too?
Also below.

nanangizz · 2024-10-16T00:27:32Z

pjlib/src/pj/atomic_queue.cpp

+                                           const char* name,
+                                           pj_atomic_queue_t **atomic_queue)
+{
+    pj_atomic_queue_t *aqueue;


Add pre-verification/PJ_ASSERT_RETURN()?
Also below.

sauwming

Also add in the PR description as to why this PR is needed, e.g. is there any performance issue with the current polling approach?
If yes, does using atomic queue manage to improve performance/reduce video lags (especially when compared to using other codec backend such as OpenH264)?

sauwming · 2024-10-16T03:06:31Z

pjlib/include/pj/atomic_queue.h

+PJ_DECL(pj_status_t) pj_atomic_queue_destroy(pj_atomic_queue_t *atomic_queue);
+
+/**
+ * Put a item to the back of the queue. If the queue is almost full


an item.

What does "almost" full mean?

sauwming · 2024-10-16T03:09:10Z

pjlib/src/pj/atomic_queue.cpp

+    }
+
+    /**
+     * Get a item from the head of the queue


an item.
Also below.

sauwming · 2024-10-16T03:12:39Z

pjlib/src/pj/atomic_queue.cpp

+        /* Surpress warning when debugging log is disabled */
+        PJ_UNUSED_ARG(name_);
+
+        // PJ_LOG(5, (name_, "Created AtomicQueue: maxItemCnt=%d itemSize=%d",


Rather than comment it, better disable it as @nanangizz suggested, such as using macro TRACE, so we can easily enable it when logging is needed.

trengginas · 2024-10-18T14:00:10Z

Also add in the PR description as to why this PR is needed, e.g. is there any performance issue with the current polling approach? If yes, does using atomic queue manage to improve performance/reduce video lags (especially when compared to using other codec backend such as OpenH264)?

Unfortunately, the lag/delay issue still noticeable.
Tested scenario:

Using the configuration specified here
Using hw codec

Implement async callback for Android mediacodec

ee60ba1

trengginas added the component: pjmedia label Oct 14, 2024

trengginas requested review from sauwming and nanangizz October 14, 2024 05:37

trengginas self-assigned this Oct 14, 2024

nanangizz reviewed Oct 14, 2024

View reviewed changes

move atomic_queue to pjlib

8b97e99

nanangizz previously approved these changes Oct 15, 2024

View reviewed changes

nanangizz self-requested a review October 15, 2024 02:10

Change the interface

42b8d75

nanangizz reviewed Oct 16, 2024

View reviewed changes

Add checks and doc.

0672e06

nanangizz approved these changes Oct 16, 2024

View reviewed changes

sauwming reviewed Oct 16, 2024

View reviewed changes

Modification based on comments

d7b56f6

sauwming approved these changes Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement async callback for Android mediacodec #4105

Implement async callback for Android mediacodec #4105

trengginas commented Oct 14, 2024 •

edited

Loading

nanangizz Oct 14, 2024

nanangizz Oct 15, 2024

nanangizz Oct 15, 2024

nanangizz Oct 16, 2024

nanangizz Oct 16, 2024

nanangizz Oct 16, 2024

sauwming left a comment

sauwming Oct 16, 2024

sauwming Oct 16, 2024

sauwming Oct 16, 2024

trengginas commented Oct 18, 2024


		#if defined(PJ_ANDROID) && PJ_ANDROID != 0

		//#include <android/log.h>

Implement async callback for Android mediacodec #4105

Are you sure you want to change the base?

Implement async callback for Android mediacodec #4105

Conversation

trengginas commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauwming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trengginas commented Oct 18, 2024

trengginas commented Oct 14, 2024 •

edited

Loading