VPLAY-12164 Http 404 for mpd while channel change#845
VPLAY-12164 Http 404 for mpd while channel change#845sayanurag wants to merge 1 commit intodev_sprint_25_2from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses HTTP 404 errors occurring during channel changes by moving the MPD downloader release operation to happen immediately after stream abstraction deletion, rather than waiting for the complete Stop() sequence to finish.
Changes:
- Added conditional MPD downloader release inside
TeardownStream()whendisableDownloadsis true - The early release prevents unnecessary MPD update requests that would fail with HTTP 404 during channel transitions
6098e5f to
62d4beb
Compare
42b9a38 to
d1bcc85
Compare
d1bcc85 to
0c0ac9c
Compare
| /** | ||
| * @fn SetFogTsbDeleted | ||
| * @brief Function to Set if Fog TSB is deleted | ||
| */ | ||
| void SetFogTsbDeleted(bool bDeleted) {mFogTsbDeleted = bDeleted;} | ||
|
|
There was a problem hiding this comment.
New public API SetFogTsbDeleted() and the new early-exit behavior in the download loop should have unit coverage (e.g., ensure setting the flag causes the downloader thread to stop requesting MPDs / exits promptly). There are existing AampMPDDownloader functional tests, so extending them here will help prevent regressions.
AampMPDDownloader.cpp
Outdated
| if(mFogTsbDeleted) | ||
| { | ||
| //TODO: Deprecate when Fog tsb is removed. Ignore the manifest request from AAMP after the TSB deleted in fog when the stop called. | ||
| AAMPLOG_INFO("Fog tsb deleted. Ignoring MPD request and exiting"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
The flag check at line 399 lacks mutex protection. While this flag is being added to prevent unnecessary downloads after TSB deletion, accessing it without synchronization creates a race condition with the setter in PrivateInstanceAAMP::Stop().
There are two approaches to fix this:
- If the flag is changed to
std::atomic_bool(as recommended in the header file comment), use.load()here:if(mFogTsbDeleted.load()) - Alternatively, check this flag within the existing mutex-protected section (lines 357-372) where
mReleaseCalledis checked, since both serve similar purposes of terminating the download loop gracefully.
AampMPDDownloader.h
Outdated
| * @fn SetFogTsbDeleted | ||
| * @brief Function to Set if Fog TSB is deleted | ||
| */ | ||
| void SetFogTsbDeleted(bool bDeleted) {mFogTsbDeleted = bDeleted;} |
There was a problem hiding this comment.
There is a mismatch between the method name declared here (SetFogTsbDeleted) and how it's called in priv_aamp.cpp:8365 (SetTsbDeleted). This will cause a compilation error because the method names don't match.
The declaration should be renamed to match the call site, or vice versa. Based on the pattern in the codebase and the fact that "Fog" is implementation-specific, SetTsbDeleted seems like the more appropriate name.
| * @fn SetFogTsbDeleted | |
| * @brief Function to Set if Fog TSB is deleted | |
| */ | |
| void SetFogTsbDeleted(bool bDeleted) {mFogTsbDeleted = bDeleted;} | |
| * @fn SetTsbDeleted | |
| * @brief Function to set if TSB is deleted | |
| */ | |
| void SetTsbDeleted(bool bDeleted) { mFogTsbDeleted = bDeleted; } |
AampMPDDownloader.cpp
Outdated
| if(mFogTsbDeleted) | ||
| { | ||
| //TODO: Deprecate when Fog tsb is removed. Ignore the manifest request from AAMP after the TSB deleted in fog when the stop called. | ||
| AAMPLOG_INFO("Fog tsb deleted. Ignoring MPD request and exiting"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
The new SetTsbDeleted method and the flag check logic in downloadMPDThread1() lack test coverage. Given that this functionality addresses a critical issue (HTTP 404 errors during channel changes), it should have unit tests to verify:
- That setting the flag prevents subsequent MPD download attempts in the download thread
- That the flag is properly initialized to false
- Thread safety of the flag access (if implemented with atomic operations)
The existing test suite in test/utests/tests/AampMPDDownloader/FunctionalTests.cpp provides a good foundation for adding these tests.
9085e19 to
7db7319
Compare
| if(mFogTsbDeleted.load()) | ||
| { | ||
| //TODO: Depecrate when Fog tsb is removed. Ignore the manifest request from AAMP after the TSB deleted in fog when the stop called. | ||
| AAMPLOG_INFO("Fog tsb deleted. Ignoring MPD request and exiting"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Missing test coverage: The new functionality for setting and checking the Fog TSB deletion flag lacks test coverage. Given that the test suite has comprehensive tests for AampMPDDownloader (see test/utests/tests/AampMPDDownloader/FunctionalTests.cpp), new behavior should be covered. Consider adding a test that verifies the download loop exits correctly when mFogTsbDeleted is set to true.
| mCheckedLLDData(false),mMPDNotifierMtx(),mMPDNotifierCondVar(),mManifestRefreshCount(0),mIsLowLatency(false), | ||
| mMPDDnldDataMtx(),mMPDDnldDataCondVar() | ||
| ,mLLDashData(),mCurrentposDeltaToManifestEnd(-1),mPublishTime(0),mMinimalRefreshRetryCount(0),mMPDNotifyPending(false) | ||
| ,mLLDashData(),mCurrentposDeltaToManifestEnd(-1),mPublishTime(0),mMinimalRefreshRetryCount(0),mMPDNotifyPending(false),mFogTsbDeleted(false) |
There was a problem hiding this comment.
The fake implementation of AampMPDDownloader in test/utests/fakes/FakeAampMPDDownloader.cpp needs to be updated to include the new mFogTsbDeleted member in its constructor initializer list to match the production code. This inconsistency could cause issues in tests that depend on the fake implementation.
| if(mMPDDownloaderInstance != nullptr) | ||
| { | ||
| mMPDDownloaderInstance->SetTsbDeleted(true); | ||
| } |
There was a problem hiding this comment.
Potential race condition: The SetTsbDeleted(true) call sets the atomic flag, but there's no synchronization to ensure the download thread has seen this flag before the TSB deletion proceeds. While std::atomic provides memory ordering guarantees, consider whether additional synchronization (e.g., waiting for the download thread to acknowledge the flag) is needed to ensure no HTTP 404 errors occur if the download thread is in the middle of processing a request when the TSB is deleted.
7db7319 to
313f366
Compare
313f366 to
7f30bba
Compare
7f30bba to
aef3b23
Compare
Reason for change: MPD requested to fog through downloadMPDThread1 even after deleting the fog tsb from PrivateInstanceAAMP::Stop(). It results in HTTP 404 error during fast channel change. This change will avoid the MPD request if the Fog TSB is deleted. Test Procedure: updated in ticket Risks: Low Signed-off-by: Anurag Krishnan <akrish513@cable.comcast.com>
aef3b23 to
5647483
Compare
| { | ||
| if(mFogTsbDeleted.load()) | ||
| { | ||
| //TODO: Deprecate when Fog tsb is removed. Ignore the manifest request from AAMP after the TSB deleted in fog when the stop called. |
There was a problem hiding this comment.
Breaking out of the download loop when mFogTsbDeleted is set exits the downloader thread without setting mReleaseCalled. Since IsDownloaderDisabled() is based on mReleaseCalled, this can leave the downloader appearing “active” while no thread is running, and (combined with the flag not being reset) can also block future tunes. Consider treating this path as a release/stop signal (set mReleaseCalled and wake any waits), and ensure the flag is cleared as part of normal Initialize()/Start() for the next tune.
| //TODO: Deprecate when Fog tsb is removed. Ignore the manifest request from AAMP after the TSB deleted in fog when the stop called. | |
| //TODO: Deprecate when Fog tsb is removed. Ignore the manifest request from AAMP after the TSB deleted in fog when the stop called. | |
| { | |
| std::lock_guard<std::recursive_mutex> lock(mMPDDnldMutex); | |
| mReleaseCalled = true; | |
| } |
Reason for change: Moving the Teardown operation before the mpd update release cause some delay and results in more Http 404 errors.
Test Procedure: updated in ticket
Risks: Low