diff --git a/fragmentcollector_mpd.cpp b/fragmentcollector_mpd.cpp index 32a05d694c..ac2c0937dd 100644 --- a/fragmentcollector_mpd.cpp +++ b/fragmentcollector_mpd.cpp @@ -674,16 +674,23 @@ 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. - 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 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)); if (future.valid()) 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