Skip to content

Replace CachedFragment::Copy with std::move in CacheTsbFragment#1350

Merged
pstroffolino merged 4 commits into
dev_sprint_25_2from
feature/VPAAMP-194
Apr 24, 2026
Merged

Replace CachedFragment::Copy with std::move in CacheTsbFragment#1350
pstroffolino merged 4 commits into
dev_sprint_25_2from
feature/VPAAMP-194

Conversation

@benmesander
Copy link
Copy Markdown
Contributor

Eliminate unnecessary deep copy of fragment data (2-7 MB for UHD) in CacheTsbFragment by using move assignment instead of Copy(). The move assignment operator already exists on CachedFragment.

Also add null guard for GetFetchChunkBuffer return value to prevent potential null pointer dereference. (bug discovered while doing above)

LL-DASH chunk mode callers that need the fragment for both CacheTsbFragment and EnqueueWrite now create a separate copy before moving into the cache.

Add unit test validating move semantics transfer ownership correctly.

Eliminate unnecessary deep copy of fragment data (2-7 MB for UHD) in
CacheTsbFragment by using move assignment instead of Copy(). The move
assignment operator already exists on CachedFragment.

Also add null guard for GetFetchChunkBuffer return value to prevent
potential null pointer dereference. (bug discovered while doing above)

LL-DASH chunk mode callers that need the fragment for both CacheTsbFragment
and EnqueueWrite now create a separate copy before moving into the cache.

Add unit test validating move semantics transfer ownership correctly.
@benmesander benmesander requested a review from a team as a code owner April 23, 2026 20:02
Comment thread MediaStreamContext.cpp
Comment thread MediaStreamContext.cpp
if (aamp->GetLLDashChunkMode())
{
CacheTsbFragment(fragmentToTsbSessionMgr);
auto fragmentForChunkCache = std::make_shared<CachedFragment>(*fragmentToTsbSessionMgr);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above

Comment thread test/utests/tests/MediaStreamContextTests/FragmentDownloadTests.cpp
Comment thread MediaStreamContext.cpp Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes TSB fragment caching by replacing a deep copy of CachedFragment payloads with move assignment, reducing large (multi-MB) memory copies in UHD scenarios, and adds a null guard to prevent a potential null dereference when acquiring a chunk-cache slot.

Changes:

  • Update MediaStreamContext::CacheTsbFragment to move-assign fragment data into the cache slot, avoiding deep copies.
  • Add a null check for GetFetchChunkBuffer(true) returning nullptr, returning false early with an error log.
  • Update LL-DASH chunk-mode paths to create a separate copy when the fragment must be used for both chunk caching and EnqueueWrite, and add a unit test intended to validate move semantics.

Reviewed changes

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

File Description
MediaStreamContext.cpp Replace Copy() with move assignment in CacheTsbFragment, add null guard, and adjust LL-DASH callers to copy before caching.
test/utests/tests/MediaStreamContextTests/FragmentDownloadTests.cpp Add a unit test for CacheTsbFragment move-based transfer into the chunk-cache slot.

Comment thread test/utests/tests/MediaStreamContextTests/FragmentDownloadTests.cpp
Comment thread MediaStreamContext.cpp
…1test

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
@pstroffolino pstroffolino merged commit 6e289e4 into dev_sprint_25_2 Apr 24, 2026
10 checks passed
@pstroffolino pstroffolino deleted the feature/VPAAMP-194 branch April 24, 2026 19:01
Abhi-jith-S pushed a commit that referenced this pull request May 12, 2026
…1350)

Reason for Change: Eliminate unnecessary deep copy of fragment data (2-7 MB for UHD) in
CacheTsbFragment by using move assignment instead of Copy(). The move
assignment operator already exists on CachedFragment.

Also add null guard for GetFetchChunkBuffer return value to prevent
potential null pointer dereference. (bug discovered while doing above)

LL-DASH chunk mode callers that need the fragment for both CacheTsbFragment
and EnqueueWrite now create a separate copy before moving into the cache.
Add unit test validating move semantics transfer ownership correctly.
Also: Refactoring to enforce std::move use at compile time.  Update for old (misleading) comment.

Test Guidance: regression + no test breakage
---------

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
varatharajan568 pushed a commit that referenced this pull request May 20, 2026
…1350)

Reason for Change: Eliminate unnecessary deep copy of fragment data (2-7 MB for UHD) in
CacheTsbFragment by using move assignment instead of Copy(). The move
assignment operator already exists on CachedFragment.

Also add null guard for GetFetchChunkBuffer return value to prevent
potential null pointer dereference. (bug discovered while doing above)

LL-DASH chunk mode callers that need the fragment for both CacheTsbFragment
and EnqueueWrite now create a separate copy before moving into the cache.
Add unit test validating move semantics transfer ownership correctly.
Also: Refactoring to enforce std::move use at compile time.  Update for old (misleading) comment.

Test Guidance: regression + no test breakage
---------

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants