Skip to content

VPLAY-12164 Http 404 for mpd while channel change #845

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
sayanurag wants to merge 4 commits into
base: dev_sprint_25_2
Choose a base branch
from
Open

VPLAY-12164 Http 404 for mpd while channel change #845

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d6fffc
VPLAY-12164 Http 404 for mpd while channel change
Jan 13, 2026
4d6be34
Merge branch 'dev_sprint_25_2' into feature/VPLAY-12164_http-404
pstroffolino Jan 15, 2026
62d4beb
Merge branch 'dev_sprint_25_2' into feature/VPLAY-12164_http-404
sayanurag Jan 20, 2026
42b9a38
Merge branch 'dev_sprint_25_2' into feature/VPLAY-12164_http-404
pstroffolino Jan 21, 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
9 changes: 9 additions & 0 deletions priv_aamp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5238,6 +5238,15 @@ void PrivateInstanceAAMP::TeardownStream(bool newTune, bool disableDownloads)
}
ReleaseStreamLock();
}

if(disableDownloads)
{
// stop the mpd update immediately after Stream abstraction delete
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The comment "stop the mpd update immediately after Stream abstraction delete" is copied from line 8104 in the Stop() function but doesn't explain the rationale for this specific placement.

The comment should clarify:

  1. Why the MPD downloader needs to be released at this point in TeardownStream
  2. How this placement helps reduce HTTP 404 errors during channel changes (as mentioned in the PR description)
  3. The relationship with the existing Release() call in Stop() at line 8107

This will help future maintainers understand the design decision and prevent accidental removal or duplication.

Suggested change
// stop the mpd update immediately after Stream abstraction delete
/*
* DASH-specific teardown: explicitly stop MPD updates as soon as the
* stream abstraction has been stopped/deleted.
*
* Rationale:
* - When changing channels, the MPD downloader may still be running
* background manifest refreshes based on the old stream abstraction
* state (periods, adaptation sets, timelines, etc.).
* - If the service reuses the same MPD URL for the new channel or if
* the old MPD becomes invalid while those refreshes are still in
* flight, the downloader can issue requests that no longer match
* the server state, leading to transient HTTP 404s during channel
* changes (as described in the PR).
*
* By releasing the MPD downloader here, immediately after
* mpStreamAbstractionAAMP is torn down and only when downloads are
* being disabled, we ensure that no further MPD refreshes are issued
* based on stale stream state.
*
* Note:
* - There is also a later mMPDDownloaderInstance->Release() call in
* Stop() (line 8107) which serves as a final safety net / global
* cleanup.
* - This earlier Release() in TeardownStream is the intentional,
* protocol-aware shutdown point required to avoid the 404 bursts
* seen on rapid channel changes; the Stop()-level call must not be
* treated as a replacement for this one.
*/

Copilot uses AI. Check for mistakes.
if(mMPDDownloaderInstance != nullptr)
{
mMPDDownloaderInstance->Release();
Copy link
Contributor

@jfagunde jfagunde Jan 13, 2026

Choose a reason for hiding this comment

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

Can this call be removed from other places since it is done here? I.e. from PrivateInstanceAAMP::detach() and PrivateInstanceAAMP::Stop()

Comment on lines +5245 to +5247
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

The new code block accesses mMPDDownloaderInstance without holding mLock. While this appears consistent with similar patterns elsewhere in the codebase (e.g., Stop() at line 8091 and detach() at line 7110), consider whether explicit locking is needed here for thread safety. The Release() method has internal locking via mMPDDnldMutex, but the pointer access itself is not synchronized. If mMPDDownloaderInstance can be deleted or modified from another thread during teardown, this could lead to a race condition.

Suggested change
if(mMPDDownloaderInstance != nullptr)
{
mMPDDownloaderInstance->Release();
AampMPDDownloader *mpdDownloader = NULL;
lock.lock();
mpdDownloader = mMPDDownloaderInstance;
lock.unlock();
if(mpdDownloader != NULL)
{
mpdDownloader->Release();

Copilot uses AI. Check for mistakes.
}
}
Comment on lines +5242 to +5249
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

This change introduces a potential double-call to mMPDDownloaderInstance->Release(). When TeardownStream(true, true) is called from Stop() (line 8088), this new code will call Release(), and then Stop() calls it again at line 8091-8094. While Release() is idempotent (protected by the mReleaseCalled flag), consider whether the duplicate call in Stop() should be removed to avoid confusion and make the cleanup flow clearer.

Suggested change
if(disableDownloads)
{
// stop the mpd update immediately after Stream abstraction delete
if(mMPDDownloaderInstance != nullptr)
{
mMPDDownloaderInstance->Release();
}
}
// Note: MPD downloader Release() is handled by the caller (e.g. Stop())
// to avoid duplicate Release() calls when TeardownStream(true, true) is
// invoked from Stop().

Copilot uses AI. Check for mistakes.
Comment on lines +5242 to +5249
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

The code uses a raw pointer (mMPDDownloaderInstance) with manual lifecycle management (new at line 5628, SAFE_DELETE at line 8199). Consider refactoring to use a smart pointer (e.g., std::unique_ptr) to follow modern C++ RAII principles and improve memory safety. This would make ownership semantics clearer and eliminate the possibility of memory leaks.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +5242 to +5249
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

This new code path (calling mMPDDownloaderInstance->Release() inside TeardownStream when disableDownloads is true) lacks test coverage.

The existing TeardownStream tests in test/utests/tests/PrivAampTests/PrivAampTests.cpp (TeardownStreamTest, TeardownStreamTest_1, TeardownStreamTest_2) do not verify:

  1. That Release() is called when disableDownloads=true
  2. That Release() is NOT called when disableDownloads=false
  3. The interaction with mMPDDownloaderInstance being null vs non-null

Add test cases to verify this behavior, especially for the scenario described in the ticket (channel change causing HTTP 404 errors).

Copilot uses AI. Check for mistakes.
Comment on lines +5242 to +5249
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

This change introduces a potential duplicate Release() call when TeardownStream is invoked from the Stop() function. At line 8102, Stop() calls TeardownStream(true, true) with disableDownloads=true, which will trigger this new Release() call. Then at line 8107, Stop() calls mMPDDownloaderInstance->Release() again.

While Release() is idempotent (protected by the mReleaseCalled flag), this duplication suggests a design issue. Consider one of the following solutions:

  1. Remove the Release() call at line 8107 in Stop() since it's now handled in TeardownStream
  2. Remove this new code and keep the existing Release() call in Stop() at line 8107
  3. Add a clear comment explaining why Release() is called in both locations if both are truly needed

The PR description mentions "Moving the Teardown operation before the mpd update release cause some delay" which suggests option 1 (removing the call from Stop()) may be the intended solution.

Suggested change
if(disableDownloads)
{
// stop the mpd update immediately after Stream abstraction delete
if(mMPDDownloaderInstance != nullptr)
{
mMPDDownloaderInstance->Release();
}
}

Copilot uses AI. Check for mistakes.
m_lastSubClockSyncTime = std::chrono::system_clock::time_point();

lock.lock();
Expand Down
Loading