VPAAMP-410 linear cdai race condition that can cause a required init segment to not be injected#1500
Open
pstroffolino wants to merge 4 commits into
Open
VPAAMP-410 linear cdai race condition that can cause a required init segment to not be injected#1500pstroffolino wants to merge 4 commits into
pstroffolino wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR primarily adds INFO-level diagnostic logging to help investigate a linear CDAI (clear ad → encrypted content) transition issue where init segments may not be injected. It also introduces additional changes around MP4 integrity monitoring, log-level locking behavior, and macOS dependency installation.
Changes:
- Add
[CDAI-DBG]INFO logs at key CDAI transition points (init injection gate/fetch, ad state transitions, stream-selection decisions) and init-fragment cache HIT/MISS. - Add an optional MP4 integrity monitor that parses downloaded audio/video segments with persistent
Mp4Demuxvalidators and dumps corrupt segments. - Improve macOS install scripts to detect/install GStreamer pkg-config paths; comment out some noisy CLI/downloader logs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
fragmentcollector_mpd.cpp |
Adds [CDAI-DBG] logs for init injection gating/fetching, adbreak completion, and period/stream-selection decisions. |
MediaStreamContext.cpp |
Adds [CDAI-DBG] init-fragment cache HIT/MISS logging; short-circuits injection when eAAMPConfig_SuppressDecode is set. |
priv_aamp.h |
Adds CheckSegmentIntegrity() declaration and persistent Mp4Demux validator members; includes MP4Demux.h. |
priv_aamp.cpp |
Implements MP4 segment integrity checking and hooks it into GetFile() under config control; comments out a WARN log. |
AampConfig.h |
Adds eAAMPConfig_MonitorMp4Integrity boolean config. |
AampConfig.cpp |
Registers monitorMp4Integrity config key; changes ConfigureLogSettings() to lock log level for `"warn" |
scripts/install_dependencies.sh |
Adds macOS GStreamer framework/homebrew detection and installs via brew if missing. |
scripts/install_aampcli.sh |
Adds macOS GStreamer framework/homebrew pkg-config path discovery and clearer error on missing GStreamer. |
downloader/AampCurlDownloader.cpp |
Comments out a WARN log line related to network persona. |
test/aampcli/Aampcli.cpp |
Comments out manifest-refresh related CLI prints. |
Comments suppressed due to low confidence (1)
test/aampcli/Aampcli.cpp:689
- Same issue here: leaving a commented-out AAMPCLI_PRINTF makes the intent unclear and risks bitrot. Prefer removing it entirely or controlling it via an explicit verbosity/log-level setting.
default:
{
std::string manifest = mAampcli.mSingleton->GetManifest();
//AAMPCLI_PRINTF("[AAMPCLI] Dash Manifest length [%zu]\n", manifest.length());
break;
Comment on lines
+12427
to
+12431
| AAMPLOG_INFO("[CDAI-DBG] onAdEvent OUTSIDE_ADBREAK: mCurrentPeriod=%s endPeriodId=%s endPeriodOffset=%u mCurrentPeriodIdx=%d", | ||
| mCurrentPeriod ? mCurrentPeriod->GetId().c_str() : "null", | ||
| mCdaiObject->mAdBreaks[mCdaiObject->mCurPlayingBreakId].endPeriodId.c_str(), | ||
| mCdaiObject->mAdBreaks[mCdaiObject->mCurPlayingBreakId].endPeriodOffset, | ||
| mCurrentPeriodIdx); |
Comment on lines
66
to
71
| #include "AampDefine.h" | ||
| #include "AampCurlDefine.h" | ||
| #include "AampLLDASHData.h" | ||
| #include "mp4demux/MP4Demux.h" | ||
| #include "AampMPDPeriodInfo.h" | ||
| #include "TsbApi.h" |
Comment on lines
+5466
to
+5473
| if (ISCONFIGSET_PRIV(eAAMPConfig_MonitorMp4Integrity) && | ||
| ISCONFIGSET_PRIV(eAAMPConfig_UseMp4Demux) && | ||
| !buffer.empty() && | ||
| (mediaType == eMEDIATYPE_VIDEO || mediaType == eMEDIATYPE_INIT_VIDEO || | ||
| mediaType == eMEDIATYPE_AUDIO || mediaType == eMEDIATYPE_INIT_AUDIO)) | ||
| { | ||
| CheckSegmentIntegrity(buffer, mediaType, remoteUrl); | ||
| } |
Comment on lines
+1844
to
+1852
| else if(logString.compare("warn") == 0 || logString.compare("mil") == 0 || logString.compare("error") == 0) | ||
| { | ||
| // Explicit quiet-level config: lock level so tune-start INFO elevation is suppressed | ||
| AAMP_LogLevel lvl = (logString.compare("error") == 0) ? eLOGLEVEL_ERROR | ||
| : (logString.compare("mil") == 0) ? eLOGLEVEL_MIL | ||
| : eLOGLEVEL_WARN; | ||
| AampLogManager::setLogLevel(lvl); | ||
| AampLogManager::lockLogLevel(true); | ||
| } |
Comment on lines
670
to
674
| case AAMP_EVENT_MANIFEST_REFRESH_NOTIFY: | ||
| { | ||
| ManifestRefreshEventPtr ev = std::dynamic_pointer_cast<ManifestRefreshEvent>(e); | ||
| AAMPCLI_PRINTF("[AAMPCLI] AAMP_EVENT_MANIFEST_REFRESH_NOTIFY received Dur[%u]:NoPeriods[%u]:PubTime[%u] manifestType[%s]\n",ev->getManifestDuration(),ev->getNoOfPeriods(),ev->getManifestPublishedTime(),ev->getManifestType().c_str()); | ||
| //AAMPCLI_PRINTF("[AAMPCLI] AAMP_EVENT_MANIFEST_REFRESH_NOTIFY received Dur[%u]:NoPeriods[%u]:PubTime[%u] manifestType[%s]\n",ev->getManifestDuration(),ev->getNoOfPeriods(),ev->getManifestPublishedTime(),ev->getManifestType().c_str()); | ||
| AAMPPlayerState state = mAampcli.mSingleton->GetState(); |
| AampNetworkPersona::Instance().LoadFromFile(personaPath); | ||
| } | ||
| AAMPLOG_WARN("priv_aamp: download url=%s personaLoaded=%d", remoteUrl.c_str(), AampNetworkPersona::Instance().IsLoaded() ? 1 : 0); | ||
| // AAMPLOG_WARN("priv_aamp: download url=%s personaLoaded=%d", remoteUrl.c_str(), AampNetworkPersona::Instance().IsLoaded() ? 1 : 0); |
| // acquire-load (~1 ns) with no mutex, no clock call, nothing else. | ||
| const bool personaActive = AampNetworkPersona::Instance().IsLoaded(); | ||
| AAMPLOG_WARN("AampCurlDownloader::Download personaActive=%d url=%s", personaActive ? 1 : 0, urlStr.c_str()); | ||
| // AAMPLOG_WARN("AampCurlDownloader::Download personaActive=%d url=%s", personaActive ? 1 : 0, urlStr.c_str()); |
Introduces eAAMPConfig_MonitorMp4Integrity (bool, default false).
When enabled alongside useMp4Demux=true, every downloaded video/audio
segment (including init segments) is parsed by a persistent per-track
Mp4Demux instance. The per-track validator preserves init-segment
state so encrypted streams do not produce false-positive
MP4_PARSE_ERROR_INVALID_IV_SIZE errors on media segments.
Each processed segment is logged at INFO level. On parse failure the
segment is logged at WARN level with the Mp4ParseError code and written
to harvestPath (or /tmp as fallback) using the server-side filename.
Files changed:
AampConfig.h - add eAAMPConfig_MonitorMp4Integrity enum value
AampConfig.cpp - add bool lookup table entry for monitorMp4Integrity
priv_aamp.h - add CheckSegmentIntegrity() declaration and
mVideoIntegrityValidator / mAudioIntegrityValidator
unique_ptr<Mp4Demux> members
priv_aamp.cpp - implement CheckSegmentIntegrity(); hook into GetFile()
after harvest block for VIDEO/AUDIO/INIT media types
trace/debug/info already locked the log level so that the tune-time INFO elevation could not override an explicit verbosity setting. warn/mil/error had no equivalent — log=warn in aamp.cfg set the string config but ConfigureLogSettings never acted on it, so setLogLevel(INFO) at tune start always overrode it. Add an else-if branch that matches warn/mil/error strings, sets the corresponding level, and calls lockLogLevel(true) so that tune-time setLogLevel(INFO) calls are blocked. This allows log=warn in aamp.cfg to produce WARN/MIL/ERROR-only output regardless of player state, which is useful when running in suppressDecode mode where the tune never formally completes.
Three sources of log noise suppressed: 1. Aampcli.cpp - AAMP_EVENT_MANIFEST_REFRESH_NOTIFY handler printed the full manifest refresh details and Dash Manifest length on every MPD update cycle (every ~2s on a live stream). There is already a structured event for this; the printf spew adds no diagnostic value and is removed. 2. AampCurlDownloader.cpp - AAMPLOG_WARN for personaActive printed on every single HTTP request. This was introduced alongside network persona support and was clearly debug instrumentation that was never intended to ship at WARN level. Commented out. 3. priv_aamp.cpp (GetFile) - matching AAMPLOG_WARN for download url / personaLoaded, same root cause as above. Commented out. The AampConfig.cpp log=warn locking change (already committed) addresses the fourth item in the JIRA: tune-time INFO level elevation now respects an explicit log=warn setting in aamp.cfg so that suppressDecode test runs do not spew info-level logs indefinitely.
Add INFO-level diagnostic logs at four key points in the CDAI clear-to-encrypted ad-to-content transition path: 1. FetchAndInjectInitialization gate: log profileChanged and discontinuity per track before the init fetch condition is evaluated, to detect silent skip of init injection. 2. FetchAndInjectInitialization fetch: log the resolved init URL and discontinuity flag when the gate passes, to detect use of a wrong (clear/ad) init segment URL. 3. onAdEvent OUTSIDE_ADBREAK: log mCurrentPeriod, endPeriodId, endPeriodOffset and mCurrentPeriodIdx at the point mBasePeriodId is updated, to detect period index mismatch after ad completion. 4. SelectSourceOrAdPeriod exit: log periodChanged, adStateChanged and requireStreamSelection before returning, to confirm StreamSelection will run for the content period. 5. MediaStreamContext init cache: log HIT/MISS and URL for every init segment fetch, to detect stale cache entries returning a clear init segment for an encrypted period URL. All new log lines use [CDAI-DBG] prefix for easy filtering. VPAAMP-410
6b4d56d to
5232473
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for Change: Add INFO-level diagnostic logs at four key points in the CDAI
clear-to-encrypted ad-to-content transition path:
FetchAndInjectInitialization gate: log profileChanged and
discontinuity per track before the init fetch condition is
evaluated, to detect silent skip of init injection.
FetchAndInjectInitialization fetch: log the resolved init URL
and discontinuity flag when the gate passes, to detect use of
a wrong (clear/ad) init segment URL.
onAdEvent OUTSIDE_ADBREAK: log mCurrentPeriod, endPeriodId,
endPeriodOffset and mCurrentPeriodIdx at the point mBasePeriodId
is updated, to detect period index mismatch after ad completion.
SelectSourceOrAdPeriod exit: log periodChanged, adStateChanged
and requireStreamSelection before returning, to confirm
StreamSelection will run for the content period.
MediaStreamContext init cache: log HIT/MISS and URL for every
init segment fetch, to detect stale cache entries returning
a clear init segment for an encrypted period URL.
All new log lines use [CDAI-DBG] prefix for easy filtering.
When the soak test captures the failure, grep the INFO logs for [CDAI-DBG] around the timestamp of the crash. The sequence to look for in the bad case will be SelectSourceOrAdPeriod exit showing requireStreamSelection=0, followed immediately by FetchAndInjectInitialization showing profileChanged=0 discontinuity=0 for the video track — confirming the init fetch was silently skipped before the encrypted segments began flowing.