RDKEMW-15991 : gst_app_src_set_max_bytes to gst_app_src_set_max_buffers by narenr94 · Pull Request #479 · rdkcentral/rialto

narenr94 · 2026-04-15T03:42:00Z

Summary: Change gst_app_src_set_max_bytes to gst_app_src_set_max_buffers
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-15991

github-actions · 2026-04-15T03:42:10Z

Pull request must be merged with a description containing the required fields,

Summary:
Type: Feature/Fix/Cleanup
Test Plan:
Jira:

If there is no jira releated to this change, please put 'Jira: NO-JIRA'.

Description can be changed by editing the top comment on your pull request and making a new commit.

Copilot

Pull request overview

This PR updates Rialto’s GStreamer appsrc queue limiting to use gst_app_src_set_max_buffers on GStreamer >= 1.20 (while keeping max-bytes fallback for older versions), and centralizes queue limit configuration into a new GstPlayerConfig.h header. It also updates unit/component tests and introduces optional buffer-size logging.

Changes:

Add IGstWrapper::gstAppSrcSetMaxBuffers() and implement it in GstWrapper with GST_CHECK_VERSION(1,20,0) guards.
Centralize byte/buffer queue limits in media/server/gstplayer/include/GstPlayerConfig.h and update GstSrc / GstWebAudioPlayer to use them.
Update tests to expect gstAppSrcSetMaxBuffers() on GStreamer >= 1.20; add optional buffer-size logging in push paths.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
wrappers/interface/IGstWrapper.h	Adds new wrapper API for setting appsrc max buffers.
wrappers/include/GstWrapper.h	Implements `gstAppSrcSetMaxBuffers` with GStreamer version guard.
tests/common/externalLibraryMocks/GstWrapperMock.h	Extends mock to include new wrapper method.
media/server/gstplayer/include/GstPlayerConfig.h	New centralized constants for appsrc queue limits (bytes + buffers).
media/server/gstplayer/include/WebAudioPlayerContext.h	Switches WebAudio limits to be sourced from `GstPlayerConfig.h`.
media/server/gstplayer/source/GstWebAudioPlayer.cpp	Uses max-buffers on >=1.20 and max-bytes on older versions.
media/server/gstplayer/source/GstSrc.cpp	Uses version-dependent max-buffers/max-bytes per media type via config constants.
media/server/gstplayer/source/tasks/webAudio/WriteBuffer.cpp	Renames WebAudio max constant usage and adds optional buffer-size logging.
media/server/gstplayer/source/GstGenericPlayer.cpp	Adds optional buffer-size logging when pushing queued buffers.
tests/unittests/media/server/gstplayer/webAudioPlayer/common/WebAudioTasksTestsBase.cpp	Updates WebAudio byte-limit constant usage in one expectation.
tests/unittests/media/server/gstplayer/webAudioPlayer/common/GstWebAudioPlayerTestCommon.cpp	Expects max-buffers on >=1.20, else max-bytes.
tests/unittests/media/server/gstplayer/rialtoSrc/AppSrcTest.cpp	Expects max-buffers mapping on >=1.20, else max-bytes.
tests/componenttests/server/tests/webAudio/WebAudioTestMethods.cpp	Expects max-buffers on >=1.20, else max-bytes.
tests/componenttests/server/tests/mediaPipeline/DualVideoPlaybackTest.cpp	Expects max-buffers on >=1.20, else max-bytes.
tests/componenttests/server/fixtures/MediaPipelineTest.cpp	Expects max-buffers on >=1.20, else max-bytes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Koky2701 <90915184+Koky2701@users.noreply.github.com> Co-authored-by: Marcin Wojciechowski <105790697+skywojciechowskim@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 97 out of 97 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-29T07:40:25Z

+    if (m_currentId == std::numeric_limits<std::uint32_t>::max())
+    {
+        m_currentId = 1;
+    }
+
+    auto [it, inserted] = m_requestMap.emplace(m_currentId, ActiveRequestsData(mediaSourceType, maxMediaBytes));
+    if (!inserted)
+    {
+        do
+        {
+            ++m_currentId;
+        } while (m_requestMap.find(m_currentId) != m_requestMap.end());
+
+        m_requestMap.emplace(m_currentId, ActiveRequestsData(mediaSourceType, maxMediaBytes));
+    }


The duplicate-id resolution loop increments m_currentId until it finds a free slot, but it doesn’t handle wrap-around inside the loop. If m_currentId approaches std::numeric_limits<uint32_t>::max() and the map already contains that value, ++m_currentId will wrap to 0 and can potentially probe reserved/invalid IDs or loop unexpectedly.

Consider handling wrap-around within the do/while (and deciding whether 0/1 are reserved), and add a guard for the (theoretical) case where the map is full.

Copilot · 2026-04-29T07:40:25Z

+        RIALTO_SERVER_LOG_ERROR("Session with id: %d does not exists", sessionId);
+        return false;


Grammar in the log message: "does not exists" → "does not exist".

Copilot · 2026-04-29T07:40:26Z

+        RIALTO_SERVER_LOG_ERROR("Session with id: %d does not exists", sessionId);
+        return false;


Grammar in the log message: "does not exists" → "does not exist".

Copilot · 2026-04-29T07:40:26Z

+/**
+ * @fn void getDuration(int session_id)
+ * @brief Get the playback Duration in nanoseconds.
+ *
+ * @param[in]  session_id   The id of the A/V session.
+ *
+ * This method is considered to be synchronous, it returns current playback duration
+ *
+ */
+message GetDurationRequest {
+    optional int32 session_id = 1 [default = -1];
+}
+message GetDurationResponse {
+    optional int64 duration = 1 [default = -1];
+}


The PR title/description indicates this is only about switching gst_app_src_set_max_bytes → gst_app_src_set_max_buffers, but the diff also introduces new IPC/API surface (GetDuration, SetReportDecodeErrors, GetQueuedFrames) and unrelated concurrency/timer changes. Please update the PR description/scope (or split into separate PRs) so reviewers can assess API and behavior changes independently from the appsrc limit refactor.

Copilot · 2026-04-29T07:40:26Z

    m_active = false;
+    m_cv.notify_one();
+
+    if (std::this_thread::get_id() == m_thread.get_id())
+    {
+        if (m_thread.joinable())
+        {
+            m_thread.detach();
+        }
+        return;
+    }


Timer::cancel() detaches the timer thread when called from within the timer thread (e.g. from the callback). This can lead to use-after-free: if the timer is destroyed from within its own callback, the destructor calls cancel(), detaches, and the still-running thread continues to access members via the captured this after the object has been destroyed.

Consider avoiding detach() here. A safer approach is to make the worker thread stop without joining when called from the same thread (set m_active=false + notify + return), and ensure destruction cannot occur until the thread has exited (e.g. by redesigning timer ownership so the thread holds a shared/self reference, or by deferring destruction/join to another thread).

Copilot · 2026-04-29T07:40:27Z

+    auto sourceIter = std::find_if(m_attachedSources.begin(), m_attachedSources.end(),
+                                   [sourceId](const auto &src) { return src.second == sourceId; });
+    if (sourceIter == m_attachedSources.end())
+    {
+        RIALTO_SERVER_LOG_ERROR("Failed - Source not found");
+        return false;
+    }
+    return m_gstPlayer->getQueuedFrames(queuedFrames);
+}


getQueuedFramesInternal() validates the sourceId exists, but then ignores the resolved MediaSourceType and unconditionally calls m_gstPlayer->getQueuedFrames(queuedFrames) (which currently queries the VIDEO decoder). This means calling getQueuedFrames() with a non-video sourceId will still return the video decoder’s queued frames (or succeed unexpectedly), which is incorrect per the public API contract (“for this source”).

Either (a) make queued-frames explicitly VIDEO-only by rejecting non-video sourceIds here (return false + log), or (b) plumb the MediaSourceType through the IGstGenericPlayer API so the underlying implementation queries the correct decoder for the given source.

Co-authored-by: Koky2701 <90915184+Koky2701@users.noreply.github.com> Co-authored-by: Marcin Wojciechowski <105790697+skywojciechowskim@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-29T08:20:40Z

+#if GST_CHECK_VERSION(1, 20, 0)
+    EXPECT_CALL(*m_gstWrapperMock, gstAppSrcSetMaxBuffers(&m_appSrc, 20));
+#else
    EXPECT_CALL(*m_gstWrapperMock, gstAppSrcSetMaxBytes(&m_appSrc, 10 * 1024));
+#endif
    EXPECT_CALL(*m_glibWrapperMock, gObjectSetStub(G_OBJECT(&m_appSrc), StrEq("format")));


On GStreamer >= 1.20 the production code configures appsrc with both max-buffers and max-bytes (see GstWebAudioPlayer::initWebAudioPipeline), but this component test only expects gstAppSrcSetMaxBuffers. With a StrictMock this will fail due to an unexpected gstAppSrcSetMaxBytes call. Update the GST_CHECK_VERSION(1, 20, 0) branch to also expect gstAppSrcSetMaxBytes(&m_appSrc, kWebAudioMaxBytes) (or align production/test behavior).

Copilot · 2026-04-29T08:20:40Z

+add_compile_definitions( RIALTO_ENABLE_BUFFER_SIZE_LOGGING )
+


RIALTO_ENABLE_BUFFER_SIZE_LOGGING is documented in GstPlayerConfig.h / GstSrc.cpp as an opt-in build flag, but it is now enabled unconditionally for all builds here. This can significantly increase log volume/perf overhead and can also introduce unused-variable warnings when RIALTO_LOG_DEBUG_ENABLED is OFF (because the log macro compiles out but the bufferSize local remains). Consider making this a CMake option (OFF by default) and only adding the compile definition when explicitly enabled (or tie it to Debug builds / RIALTO_LOG_DEBUG_ENABLED).

Copilot · 2026-04-29T08:20:41Z

 {
    EXPECT_CALL(*m_gstWrapperMock, gstElementFactoryMake(StrEq("appsrc"), StrEq("audsrc"))).WillOnce(Return(&m_appSrc));
+#if GST_CHECK_VERSION(1, 20, 0)
+    EXPECT_CALL(*m_gstWrapperMock, gstAppSrcSetMaxBuffers(GST_APP_SRC(&m_appSrc), 20));


GstWebAudioPlayer now sets both gstAppSrcSetMaxBuffers(...) and gstAppSrcSetMaxBytes(...) on GStreamer >= 1.20, but this StrictMock expectation only covers gstAppSrcSetMaxBuffers. This will fail the unit tests due to an unexpected gstAppSrcSetMaxBytes call. Add the corresponding EXPECT_CALL for gstAppSrcSetMaxBytes in the GST_CHECK_VERSION(1, 20, 0) branch (or adjust production code so it only makes the calls the test expects).

Suggested change

EXPECT_CALL(*m_gstWrapperMock, gstAppSrcSetMaxBuffers(GST_APP_SRC(&m_appSrc), 20));

EXPECT_CALL(*m_gstWrapperMock, gstAppSrcSetMaxBuffers(GST_APP_SRC(&m_appSrc), 20));

EXPECT_CALL(*m_gstWrapperMock, gstAppSrcSetMaxBytes(GST_APP_SRC(&m_appSrc), 10 * 1024));

gst_app_src_set_max_bytes to gst_app_src_set_max_buffers

5dd938b

Copilot AI review requested due to automatic review settings April 15, 2026 03:42

Copilot started reviewing on behalf of narenr94 April 15, 2026 03:42 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

nrames759 and others added 2 commits April 15, 2026 09:19

enable buffer size logging

a4ad249

master sync (#490)

91d2b66

Co-authored-by: Koky2701 <90915184+Koky2701@users.noreply.github.com> Co-authored-by: Marcin Wojciechowski <105790697+skywojciechowskim@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 29, 2026 07:33

Copilot started reviewing on behalf of narenr94 April 29, 2026 07:34 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

narenr94 and others added 3 commits April 29, 2026 13:42

master sync (#491)

f0be8d6

Co-authored-by: Koky2701 <90915184+Koky2701@users.noreply.github.com> Co-authored-by: Marcin Wojciechowski <105790697+skywojciechowskim@users.noreply.github.com>

Merge branch 'master' into feature/RDKEMW-15991

4f6b625

copilot review address

753224a

Copilot AI review requested due to automatic review settings April 29, 2026 08:15

Copilot started reviewing on behalf of narenr94 April 29, 2026 08:17 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

copilot comment addres 2

a6b562a

		RIALTO_SERVER_LOG_ERROR("Session with id: %d does not exists", sessionId);
		return false;

		add_compile_definitions( RIALTO_ENABLE_BUFFER_SIZE_LOGGING )

	EXPECT_CALL(*m_gstWrapperMock, gstAppSrcSetMaxBuffers(GST_APP_SRC(&m_appSrc), 20));
	EXPECT_CALL(*m_gstWrapperMock, gstAppSrcSetMaxBuffers(GST_APP_SRC(&m_appSrc), 20));
	EXPECT_CALL(m_gstWrapperMock, gstAppSrcSetMaxBytes(GST_APP_SRC(&m_appSrc), 10 1024));

Conversation

narenr94 commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

narenr94 commented Apr 15, 2026 •

edited

Loading