From b81b423ec9b59661baaee04e9ba41205724e5bc0 Mon Sep 17 00:00:00 2001 From: psiva01 Date: Thu, 9 Jul 2026 12:43:02 +0530 Subject: [PATCH 1/5] VPAAMP-737: [Xumo] Playback fails when played DASH VOD assets Reason for change: If bandwidth has changed after async download job submitted & new IDX is not yet loaded (the transient window), fall back to the submission-time URL so URL and range stay paired. Signed-off-by: psiva01 --- MediaStreamContext.cpp | 33 ++++++++++++++++++++++++++++++--- fragmentcollector_mpd.cpp | 2 ++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/MediaStreamContext.cpp b/MediaStreamContext.cpp index 476920dcbe..4a8ae6ff28 100644 --- a/MediaStreamContext.cpp +++ b/MediaStreamContext.cpp @@ -1047,10 +1047,37 @@ bool MediaStreamContext::DownloadFragment(DownloadInfoPtr dlInfo) URIInfo uriInfo; if (dlInfo->uriList.size() > 0) { - // Asses the current bandwidth and get the appropriate URIInfo from the map with resolved URLs - if (dlInfo->uriList.find(fragmentDescriptor.Bandwidth) != dlInfo->uriList.end()) + // For SegmentBase content the byte range in dlInfo was computed against the + // profile active at job-submission time (dlInfo->bandwidth). An ABR switch + // clears IDX before reloading it for the new profile. If an async media- + // segment job runs inside that window, fragmentDescriptor.Bandwidth already + // reflects the new profile but IDX is still empty, so the bandwidth-change + // recompute below is skipped. Using the new-profile URL with the old- + // profile byte range then fetches data at the wrong offset and GStreamer + // sees a bogus box size (VPAAMP-614). + // + // Guard: when bandwidth has changed AND the IDX has been cleared (not yet + // reloaded for the new profile), fall back to the submission-time bandwidth + // for the URL lookup so that URL and range stay paired. When IDX is + // present the recompute block below will derive the correct new-profile + // range and the new-profile URL is safe to use. + BitsPerSecond urlLookupBw = fragmentDescriptor.Bandwidth; + if (!dlInfo->range.empty() && + dlInfo->bandwidth > 0 && + dlInfo->bandwidth != fragmentDescriptor.Bandwidth) { - uriInfo = dlInfo->uriList[fragmentDescriptor.Bandwidth]; + std::lock_guard idxLock(mIdxMutex); + if (IDX.empty()) + { + // IDX cleared for ABR switch but not yet reloaded — cannot + // recompute the range; keep the submission-time URL so the + // existing range remains valid. + urlLookupBw = dlInfo->bandwidth; + } + } + if (dlInfo->uriList.find(urlLookupBw) != dlInfo->uriList.end()) + { + uriInfo = dlInfo->uriList[urlLookupBw]; } if (uriInfo.url.empty() && dlInfo->uriList.size() > 0) { diff --git a/fragmentcollector_mpd.cpp b/fragmentcollector_mpd.cpp index 7b18a4b0f2..df93992518 100644 --- a/fragmentcollector_mpd.cpp +++ b/fragmentcollector_mpd.cpp @@ -682,6 +682,8 @@ bool StreamAbstractionAAMP_MPD::FetchFragment(MediaStreamContext *pMediaStreamCo // init download completion on the worker thread (fragmentOffset is mutated on // both threads simultaneously). Run SegmentBase init synchronously to guarantee // the init bytes are injected before the FetcherLoop advances. + // SegmentBase media segments are safe to download in parallel; the ABR-switch + // URL/range consistency race is addressed in DownloadFragment (VPAAMP-614). const bool segmentBaseInit = isInitializationSegment && !range.empty(); if (ISCONFIGSET(eAAMPConfig_DashParallelFragDownload) && !segmentBaseInit) { From 88dd2c5e90d9b6658ce097d59e3640b95463e075 Mon Sep 17 00:00:00 2001 From: psiva01 Date: Thu, 9 Jul 2026 22:43:08 +0530 Subject: [PATCH 2/5] VPAAMP-737: [Xumo] Playback fails when played DASH VOD assets Signed-off-by: psiva01 --- MediaStreamContext.cpp | 33 +++------------------------------ fragmentcollector_mpd.cpp | 25 +++++++++++++------------ 2 files changed, 16 insertions(+), 42 deletions(-) diff --git a/MediaStreamContext.cpp b/MediaStreamContext.cpp index 4a8ae6ff28..476920dcbe 100644 --- a/MediaStreamContext.cpp +++ b/MediaStreamContext.cpp @@ -1047,37 +1047,10 @@ bool MediaStreamContext::DownloadFragment(DownloadInfoPtr dlInfo) URIInfo uriInfo; if (dlInfo->uriList.size() > 0) { - // For SegmentBase content the byte range in dlInfo was computed against the - // profile active at job-submission time (dlInfo->bandwidth). An ABR switch - // clears IDX before reloading it for the new profile. If an async media- - // segment job runs inside that window, fragmentDescriptor.Bandwidth already - // reflects the new profile but IDX is still empty, so the bandwidth-change - // recompute below is skipped. Using the new-profile URL with the old- - // profile byte range then fetches data at the wrong offset and GStreamer - // sees a bogus box size (VPAAMP-614). - // - // Guard: when bandwidth has changed AND the IDX has been cleared (not yet - // reloaded for the new profile), fall back to the submission-time bandwidth - // for the URL lookup so that URL and range stay paired. When IDX is - // present the recompute block below will derive the correct new-profile - // range and the new-profile URL is safe to use. - BitsPerSecond urlLookupBw = fragmentDescriptor.Bandwidth; - if (!dlInfo->range.empty() && - dlInfo->bandwidth > 0 && - dlInfo->bandwidth != fragmentDescriptor.Bandwidth) + // Asses the current bandwidth and get the appropriate URIInfo from the map with resolved URLs + if (dlInfo->uriList.find(fragmentDescriptor.Bandwidth) != dlInfo->uriList.end()) { - std::lock_guard idxLock(mIdxMutex); - if (IDX.empty()) - { - // IDX cleared for ABR switch but not yet reloaded — cannot - // recompute the range; keep the submission-time URL so the - // existing range remains valid. - urlLookupBw = dlInfo->bandwidth; - } - } - if (dlInfo->uriList.find(urlLookupBw) != dlInfo->uriList.end()) - { - uriInfo = dlInfo->uriList[urlLookupBw]; + uriInfo = dlInfo->uriList[fragmentDescriptor.Bandwidth]; } if (uriInfo.url.empty() && dlInfo->uriList.size() > 0) { diff --git a/fragmentcollector_mpd.cpp b/fragmentcollector_mpd.cpp index df93992518..1a8fcb074d 100644 --- a/fragmentcollector_mpd.cpp +++ b/fragmentcollector_mpd.cpp @@ -674,18 +674,19 @@ bool StreamAbstractionAAMP_MPD::FetchFragment(MediaStreamContext *pMediaStreamCo timeBasedBufferManager->PopulateBuffer(fragmentDuration); } - // SegmentBase init segments are byte-range requests against a single file. - // The FetcherLoop continues immediately after SubmitJob returns and will call - // LoadIDX then submit media segment[0] to the same worker queue. Although the - // worker processes jobs in order, GStreamer still sees an invalid stream because - // the IDX-load + media-segment path in FetchNextFragment races with the async - // init download completion on the worker thread (fragmentOffset is mutated on - // both threads simultaneously). Run SegmentBase init synchronously to guarantee - // the init bytes are injected before the FetcherLoop advances. - // SegmentBase media segments are safe to download in parallel; the ABR-switch - // URL/range consistency race is addressed in DownloadFragment (VPAAMP-614). - const bool segmentBaseInit = isInitializationSegment && !range.empty(); - if (ISCONFIGSET(eAAMPConfig_DashParallelFragDownload) && !segmentBaseInit) + // SegmentBase content (both init and media segments) uses byte-range requests + // against a single file and requires strict ordering. The FetcherLoop can + // race ahead after SubmitJob returns, loading IDX and submitting subsequent + // segments while async downloads are still in flight. During ABR switches + // the IDX is cleared and reloaded for the new profile; if a stale async job + // then runs with the new profile's IDX it gets a start offset that matches + // the new profile but an end offset from the old profile's range, producing + // a partial fragment download that confuses the IsoBmff parser with a + // declared box size exceeding the available bytes (VPAAMP-614). Run ALL + // SegmentBase downloads synchronously so that each (URL, IDX, range) triple + // is consistent at execution time. + const bool segmentBaseContent = !range.empty(); + if (ISCONFIGSET(eAAMPConfig_DashParallelFragDownload) && !segmentBaseContent) { auto future = aamp->GetAampTrackWorkerManager()->SubmitJob(downloadInfo->mediaType, downloadJob, (isInitializationSegment && pMediaStreamContext->profileChanged)); if (future.valid()) From bb37b8e146b1893560b16a35f37fc28a4b8b136d Mon Sep 17 00:00:00 2001 From: pstroffolino Date: Wed, 15 Jul 2026 00:13:05 -0400 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- fragmentcollector_mpd.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fragmentcollector_mpd.cpp b/fragmentcollector_mpd.cpp index 04dda84fd7..95c524cdab 100644 --- a/fragmentcollector_mpd.cpp +++ b/fragmentcollector_mpd.cpp @@ -685,7 +685,7 @@ bool StreamAbstractionAAMP_MPD::FetchFragment(MediaStreamContext *pMediaStreamCo // declared box size exceeding the available bytes (VPAAMP-614). Run ALL // SegmentBase downloads synchronously so that each (URL, IDX, range) triple // is consistent at execution time. - const bool segmentBaseContent = !range.empty(); + const bool segmentBaseContent = (pMediaStreamContext->representation != nullptr) && (pMediaStreamContext->representation->GetSegmentBase() != nullptr) && !range.empty(); if (ISCONFIGSET(eAAMPConfig_DashParallelFragDownload) && !segmentBaseContent) { auto future = aamp->GetAampTrackWorkerManager()->SubmitJob(downloadInfo->mediaType, downloadJob, (isInitializationSegment && pMediaStreamContext->profileChanged)); From b4772ffeaffbf6ef18f9604ffa2f75737a3cf1ca Mon Sep 17 00:00:00 2001 From: Sivasubramanian Patchaiperumal Date: Wed, 15 Jul 2026 11:16:21 +0530 Subject: [PATCH 4/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- fragmentcollector_mpd.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fragmentcollector_mpd.cpp b/fragmentcollector_mpd.cpp index 7fb953121f..ac2c0937dd 100644 --- a/fragmentcollector_mpd.cpp +++ b/fragmentcollector_mpd.cpp @@ -685,7 +685,11 @@ bool StreamAbstractionAAMP_MPD::FetchFragment(MediaStreamContext *pMediaStreamCo // declared box size exceeding the available bytes (VPAAMP-614). Run ALL // SegmentBase downloads synchronously so that each (URL, IDX, range) triple // is consistent at execution time. - const bool segmentBaseContent = (pMediaStreamContext->representation != nullptr) && (pMediaStreamContext->representation->GetSegmentBase() != nullptr) && !range.empty(); + const auto* representation = pMediaStreamContext->representation; + const bool segmentBaseContent = + (representation != nullptr) && + (representation->GetSegmentBase() != nullptr) && + !range.empty(); if (ISCONFIGSET(eAAMPConfig_DashParallelFragDownload) && !segmentBaseContent) { auto future = aamp->GetAampTrackWorkerManager()->SubmitJob(downloadInfo->mediaType, downloadJob, (isInitializationSegment && pMediaStreamContext->profileChanged)); From 614fca0ea71c5b8026b88c4a6f0db6f99e837a3b Mon Sep 17 00:00:00 2001 From: psiva01 Date: Wed, 15 Jul 2026 15:14:11 +0530 Subject: [PATCH 5/5] VPAAMP-737: Playback fails when played DASH VOD assets Reason for change: Added L1 tests to verify DASH Init & SegmentBase downloads are serialized Signed-off-by: psiva01 --- .../fakes/FakeAampTrackWorkerManager.cpp | 7 ++ .../FetcherLoopTests.cpp | 102 ++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/test/utests/fakes/FakeAampTrackWorkerManager.cpp b/test/utests/fakes/FakeAampTrackWorkerManager.cpp index b7d04020b0..988f1d8a9b 100644 --- a/test/utests/fakes/FakeAampTrackWorkerManager.cpp +++ b/test/utests/fakes/FakeAampTrackWorkerManager.cpp @@ -19,6 +19,12 @@ #include "AampTrackWorkerManager.hpp" +// Global counter incremented each time AampTrackWorkerManager::SubmitJob is +// called. Tests that need to distinguish the synchronous Execute() path from +// the async SubmitJob path reset this to 0 before the window under test and +// inspect it afterwards. +int g_submitJobCallCount = 0; + namespace aamp { /** @@ -116,6 +122,7 @@ namespace aamp */ std::shared_future AampTrackWorkerManager::SubmitJob(AampMediaType mediaType, std::shared_ptr job, bool highPriority) { + g_submitJobCallCount++; if (job) { job->Run(); // Execute the job immediately for testing purposes diff --git a/test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp b/test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp index dfc20bfba4..12b652ca08 100644 --- a/test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp +++ b/test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp @@ -3327,4 +3327,106 @@ TEST_F(FetcherLoopTests, SegmentBase_SkipFragments_FloatingPointEpsilon_CrossesF EXPECT_EQ(ctx->fragmentIndex, 1); EXPECT_NEAR(ctx->fragmentTime, 2.0, 0.001); +} + +// Declared in FakeAampTrackWorkerManager.cpp — incremented each time +// AampTrackWorkerManager::SubmitJob is called. +extern int g_submitJobCallCount; + +/** + * @brief VPAAMP-614: SegmentBase init fragment must NOT be submitted via + * AampTrackWorkerManager::SubmitJob when DashParallelFragDownload is enabled. + * + * The SegmentBase init segment is a byte-range request. Submitting it + * asynchronously with highPriority=true causes it to jump ahead of already- + * queued old-profile media jobs on the worker queue. The decoder then + * receives old-profile media after the new moov, producing macroblocking. + * The fix gates on segmentBaseFirstInit so the init executes inline + * (Execute()) regardless of eAAMPConfig_DashParallelFragDownload. + * + * Oracle: g_submitJobCallCount == 0 after InitializeMPD with a SegmentBase + * manifest and DashParallelFragDownload=true (default fixture setting), + * because the init segment's non-empty byte range forces Execute(). + */ +TEST_F(FetcherLoopTests, SegmentBase_InitFragment_NotSubmittedViaSubmitJob) +{ + // eAAMPConfig_DashParallelFragDownload=true is already the default. + EXPECT_CALL(*g_mockPrivateInstanceAAMP, LoadIDX(_, _, _, _, _, _, _, _, _, _)) + .WillRepeatedly(WithArg<3>(Invoke([](std::vector &idxBuffer) + { + idxBuffer.insert(idxBuffer.end(), std::cbegin(sidxBox), std::cend(sidxBox)); + }))); + + EXPECT_CALL(*g_mockMediaStreamContext, + CacheFragment(kSegBaseVideoUrl, _, _, _, _, true, _, _, _)) + .WillRepeatedly(Return(true)); + + // Reset immediately before the window under test. + g_submitJobCallCount = 0; + + AAMPStatusType status = InitializeMPD(kSegmentBaseVodManifest); + ASSERT_EQ(status, eAAMPSTATUS_OK); + + // The init segment range is derived from indexRange="500-999" (init = "0-499"). + // segmentBaseFirstInit=true -> Execute() path -> SubmitJob not called. + // If the guard is accidentally removed, this count becomes >= 1. + EXPECT_EQ(g_submitJobCallCount, 0) + << "SegmentBase init must execute inline via Execute(), not SubmitJob. " + "Submitting with highPriority=true jumps the worker queue and delivers " + "old-profile media after the new moov, causing macroblocking."; +} + +/** + * @brief VPAAMP-614: SegmentBase media fragments must NOT be submitted via + * AampTrackWorkerManager::SubmitJob when DashParallelFragDownload is enabled. + * + * All SegmentBase downloads (init and media) use byte-range requests against a + * single file. Submitting media segments asynchronously allows the FetcherLoop + * to race ahead: it loads IDX and submits the next segment before the previous + * download has completed. During ABR switches this produces a stale (URL, IDX, + * range) triple that confuses the IsoBmff parser with a bogus box-size error + * (VPAAMP-614). The segmentBaseContent guard forces all SegmentBase downloads + * through the synchronous Execute() path regardless of + * eAAMPConfig_DashParallelFragDownload. + * + * Oracle: g_submitJobCallCount == 0 after one PushNextFragment for a SegmentBase + * media segment, confirming the synchronous Execute() path is taken. + * If the guard is narrowed to init-only (segmentBaseInit), this count becomes 1, + * re-exposing the race. + */ +TEST_F(FetcherLoopTests, SegmentBase_MediaFragment_ExecutesSynchronously) +{ + // eAAMPConfig_DashParallelFragDownload=true is already the default. + EXPECT_CALL(*g_mockPrivateInstanceAAMP, LoadIDX(_, _, _, _, _, _, _, _, _, _)) + .WillRepeatedly(WithArg<3>(Invoke([](std::vector &idxBuffer) + { + idxBuffer.insert(idxBuffer.end(), std::cbegin(sidxBox), std::cend(sidxBox)); + }))); + + EXPECT_CALL(*g_mockMediaStreamContext, + CacheFragment(kSegBaseVideoUrl, _, _, _, _, true, _, _, _)) + .WillRepeatedly(Return(true)); + + AAMPStatusType status = InitializeMPD(kSegmentBaseVodManifest); + ASSERT_EQ(status, eAAMPSTATUS_OK); + + // Reset after init (init took Execute(), not SubmitJob) so the counter + // reflects only the media-segment download that follows. + g_submitJobCallCount = 0; + + // First media segment: ref[0] bytes "1000-17383", duration 2.0 s. + EXPECT_CALL(*g_mockMediaStreamContext, + CacheFragment(kSegBaseVideoUrl, _, _, _, _, false, _, _, _)) + .WillOnce(Return(true)); + + ASSERT_TRUE(PushNextFragment(eTRACK_VIDEO)); + + // A SegmentBase media fragment must go through Execute(), not SubmitJob, so + // the FetcherLoop cannot race ahead and produce a stale (URL, IDX, range) + // triple during ABR switches. Narrowing the guard to init-only would leave + // this at 1, re-exposing the VPAAMP-614 parse error. + EXPECT_EQ(g_submitJobCallCount, 0) + << "SegmentBase media fragment must execute via Execute(), not SubmitJob. " + "Async submission allows the FetcherLoop to race ahead of IDX loading " + "during ABR switches, producing a stale byte-range and a bogus box-size error."; } \ No newline at end of file