Skip to content

VPLAY-12061: Cherry-pick closed captioning fixes to federated branch #970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 4 commits into
base: feature/RDK-60641_federated_2601
Choose a base branch
from
Open

VPLAY-12061: Cherry-pick closed captioning fixes to federated branch #970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
125436a
Initial plan
Copilot Feb 4, 2026
3ff7b94
Revert "VPLAY-12061:Closed captioning issues after introducing webvTT…
pstroffolino Jan 30, 2026
60c4ec5
VPLAY-12061:Closed captioning issues after introducing webvTT along w…
Gnanesha Feb 3, 2026
a6644dc
Merge feature/VPLAY-12061_fed with cherry-picked commits
Copilot Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion middleware/InterfacePlayerRDK.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ total_bytes(0), n_audio(0), current_audio(0),
periodicProgressCallbackIdleTaskId(GST_TASK_ID_INVALID),
bufferingTimeoutTimerId(GST_TASK_ID_INVALID), video_dec(NULL), audio_dec(NULL), TaskControlMutex(), firstProgressCallbackIdleTask("FirstProgressCallback"),
video_sink(NULL), audio_sink(NULL), subtitle_sink(NULL), task_pool(NULL),
rate(GST_NORMAL_PLAY_RATE), zoom(GST_VIDEO_ZOOM_NONE), videoMuted(false), audioMuted(false), volumeMuteMutex(), subtitleMuted(false),
rate(GST_NORMAL_PLAY_RATE), zoom(GST_VIDEO_ZOOM_NONE), videoMuted(false), audioMuted(false), volumeMuteMutex(), subtitleMuted(true),
audioVolume(1.0), eosCallbackIdleTaskId(GST_TASK_ID_INVALID), eosCallbackIdleTaskPending(false),
firstFrameReceived(false), pendingPlayState(false), decoderHandleNotified(false),
firstFrameCallbackIdleTaskId(GST_TASK_ID_INVALID), firstFrameCallbackIdleTaskPending(false),
Expand Down
1 change: 1 addition & 0 deletions test/utests/tests/AampGstPlayer/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ message(GSTREAMER_INCLUDE_DIRS=${GSTREAMER_INCLUDE_DIRS})
set(TEST_SOURCES FunctionalTests.cpp
PauseOnPlaybackTests.cpp
AampGstPlayerTests.cpp
DefaultSubtitleMuteTests.cpp
)

set(AAMP_SOURCES ${AAMP_ROOT}/aampgstplayer.cpp
Expand Down
127 changes: 127 additions & 0 deletions test/utests/tests/AampGstPlayer/DefaultSubtitleMuteTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* If not stated otherwise in this file or this component's license file the
* following copyright and licenses apply:
*
* Copyright 2024 RDK Management
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include "middleware/InterfacePlayerRDK.h"
#include "middleware/InterfacePlayerPriv.h"
using ::testing::_;
/**
* @brief Test fixture for default subtitle mute state
*
* This test suite validates that the default subtitle/CC rendering behavior
* is correctly initialized and applied when the subtitle sink is created.
*/
class DefaultSubtitleMuteTests : public ::testing::Test
{
protected:
InterfacePlayerRDK *mInterfacePlayerRDK{};
void SetUp() override
{
// Create a fresh InterfacePlayerRDK instance
mInterfacePlayerRDK = new InterfacePlayerRDK();
}
void TearDown() override
{
delete mInterfacePlayerRDK;
mInterfacePlayerRDK = nullptr;
}
Comment on lines +33 to +43
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raw pointer member combined with manual memory management in SetUp/TearDown is a legacy C-style pattern. According to project modernization guidelines, prefer smart pointers for automatic resource management and RAII.

Consider using std::unique_ptr<InterfacePlayerRDK> instead:

std::unique_ptr<InterfacePlayerRDK> mInterfacePlayerRDK;

With smart pointers, the SetUp method would use mInterfacePlayerRDK = std::make_unique<InterfacePlayerRDK>(); and TearDown can be simplified or even removed since the smart pointer will automatically clean up when the test fixture is destroyed. This pattern is safer and aligns with modern C++ best practices (C++ Core Guidelines C.149).

Copilot uses AI. Check for mistakes.
};
/**
* @brief Test that GstPlayerPriv::subtitleMuted defaults to true
*
* This test verifies that subtitles start in a muted state by default,
* ensuring CC/WebVTT rendering is disabled unless explicitly enabled by the user.
*
* Related to VPLAY-12061: Closed captioning issues after introducing webvTT
* along with inBand CC tracks. The fix disables webvtt rendering by default.
*/
TEST_F(DefaultSubtitleMuteTests, GstPlayerPriv_DefaultSubtitleMutedState_IsTrue)
{
// Verify that the default state of subtitleMuted is true
ASSERT_TRUE(mInterfacePlayerRDK->interfacePlayerPriv->gstPrivateContext->subtitleMuted);
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is accessing the private member interfacePlayerPriv of InterfacePlayerRDK. According to the class definition in middleware/InterfacePlayerRDK.h (line 768), interfacePlayerPriv is declared in the private section of the class.

Tests should use the public interface. The InterfacePlayerRDK class provides a public accessor method GetPrivatePlayer() (line 171 in InterfacePlayerRDK.h) that should be used instead. This approach maintains proper encapsulation and ensures tests work with the documented public API.

Replace mInterfacePlayerRDK->interfacePlayerPriv with mInterfacePlayerRDK->GetPrivatePlayer() throughout this test file.

Copilot uses AI. Check for mistakes.
}
/**
* @brief Test that default subtitle mute state prevents rendering
*
* This test ensures that when subtitles are muted by default, they remain
* muted until explicitly unmuted by the application, preventing unexpected
* subtitle rendering behavior.
*/
TEST_F(DefaultSubtitleMuteTests, GstPlayerPriv_DefaultState_PreventsSubtitleRendering)
{
// Given: Fresh InterfacePlayerRDK instance with default state
auto *gstPriv = mInterfacePlayerRDK->interfacePlayerPriv->gstPrivateContext;
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using auto * for pointer types is less clear than using auto* (no space) or being explicit about the pointer type. According to modern C++ style guidelines and project conventions, when using auto with pointers, prefer one of these alternatives for clarity:

  1. Explicit type: InterfacePlayerPriv* gstPriv = ...
  2. No space: auto* gstPriv = ...

The explicit type is often preferred in production code for better readability, while auto* without a space is cleaner if type deduction is genuinely beneficial.

Copilot uses AI. Check for mistakes.

// Then: Subtitle mute should be enabled (true)
EXPECT_TRUE(gstPriv->subtitleMuted);

// And: Video and audio mute should be disabled (false) by default
EXPECT_FALSE(gstPriv->videoMuted);
EXPECT_FALSE(gstPriv->audioMuted);
}
/**
* @brief Test that SetSubtitleMute correctly updates the state
*
* This test validates that the SetSubtitleMute method properly updates
* the internal subtitleMuted state, which is used when creating the
* subtitle sink to set its initial mute property.
*/
TEST_F(DefaultSubtitleMuteTests, SetSubtitleMute_UpdatesInternalState_Correctly)
{
auto *gstPriv = mInterfacePlayerRDK->interfacePlayerPriv->gstPrivateContext;
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as line 69: Using auto * with a space. For consistency and clarity, prefer either explicit type InterfacePlayerPriv* gstPriv or auto* gstPriv (without space).

Copilot uses AI. Check for mistakes.

// Initially should be muted (true)
ASSERT_TRUE(gstPriv->subtitleMuted);

// When: Unmuting subtitles
mInterfacePlayerRDK->SetSubtitleMute(false);

// Then: Internal state should be updated
EXPECT_FALSE(gstPriv->subtitleMuted);

// When: Re-muting subtitles
mInterfacePlayerRDK->SetSubtitleMute(true);

// Then: Internal state should reflect muted again
EXPECT_TRUE(gstPriv->subtitleMuted);
}
/**
* @brief Test initial state consistency across player lifecycle
*
* This test ensures that creating multiple InterfacePlayerRDK instances
* consistently initializes subtitleMuted to true, preventing state pollution
* between different player instances.
*/
TEST_F(DefaultSubtitleMuteTests, MultipleInstances_ConsistentDefaultState)
{
// Create multiple instances and verify consistent default state
InterfacePlayerRDK *player1 = new InterfacePlayerRDK();
InterfacePlayerRDK *player2 = new InterfacePlayerRDK();
InterfacePlayerRDK *player3 = new InterfacePlayerRDK();

// All instances should have subtitles muted by default
EXPECT_TRUE(player1->interfacePlayerPriv->gstPrivateContext->subtitleMuted);
EXPECT_TRUE(player2->interfacePlayerPriv->gstPrivateContext->subtitleMuted);
EXPECT_TRUE(player3->interfacePlayerPriv->gstPrivateContext->subtitleMuted);

// Clean up
delete player1;
delete player2;
delete player3;
Comment on lines +114 to +126
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual memory management with raw new/delete is discouraged in modern C++ and violates the RAII principle. The test fixture already manages one instance via SetUp/TearDown, but this test creates three additional instances manually.

According to the coding guidelines (C++ Core Guidelines and project standards), use smart pointers or stack allocation for automatic cleanup and exception safety. Consider using std::unique_ptr to manage these instances, which will provide automatic cleanup even if an assertion fails:

auto player1 = std::make_unique<InterfacePlayerRDK>();
auto player2 = std::make_unique<InterfacePlayerRDK>();
auto player3 = std::make_unique<InterfacePlayerRDK>();

This eliminates the need for manual delete calls and makes the code exception-safe.

Copilot uses AI. Check for mistakes.
}
Loading