Feat: Full CMCD v1 Spec#1571
Open
bbetter173 wants to merge 39 commits into
Open
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n CMCDSerializer.h - Add CMCDGroup enum class with four values: Object, Request, Session, Status - Add CMCDEntry struct with key, value, group, isInteger, isQuotedString, isBoolToken fields - Declare four free functions: RoundToNearest100, QuoteString, CMCDGroupToHeaderKey, SerializeToCMCDMap - Use #ifndef CMCDSerializer_h guard (matches CMCDHeaders.h style) - Include RDK Apache-2.0 copyright block and full Doxygen documentation
…target - Implement RoundToNearest100 using ((value + 50) / 100) * 100 (round-half-up) - Implement QuoteString with double-quote wrapping and backslash escaping of '"' and '\' - Implement CMCDGroupToHeaderKey returning "CMCD-Object:" etc. with trailing ':' - Implement SerializeToCMCDMap: sort entries alphabetically by key (SER-04), apply rounding/quoting/boolean encoding per type flags, omit integer entries rounding to zero - Register CMCDSerializer.cpp and CMCDSerializer.h in support/aampmetrics/CMakeLists.txt
…itives - Add CMCDSerializerGTest.cpp as the test runner (InitGoogleTest/RUN_ALL_TESTS) - Add CMCDSerializerTest.cpp with 8 TEST cases covering all serializer primitives: - RoundToNearest100: rounds-half-up and sub-fifty-rounds-to-zero - QuoteString: wraps and escapes interior double-quotes and backslashes - CMCDGroupToHeaderKey: returns correct header keys with trailing colon - SerializeToCMCDMap: sorts Object keys alphabetically and rounds integers - SerializeToCMCDMap: bool token true emits bare key, bool false omits group - SerializeToCMCDMap: quoted-string wraps sid in double-quotes - SerializeToCMCDMap: integer rounding to zero omits the entry entirely - Add CMakeLists.txt linking the REAL CMCDSerializer.cpp (not faked) - Register CMCDSerializer suite via add_subdirectory in tests/CMakeLists.txt
…zeToCMCDMap
- Add #include "CMCDSerializer.h"
- Replace hand-built CMCDSession+sessionId concatenation with structured
CMCDEntry{sid, sessionId, Session, isQuotedString=true}
- Call SerializeToCMCDMap to write CMCD-Session: with quoted sid (SER-03)
- Add subclass contract comment: call base first, then serialize own entries
…rows via SerializeToCMCDMap - Add #include "CMCDSerializer.h" to both files - Replace CMCDBITRATE/CMCDTOPBITRATE/etc string concatenation with std::vector<CMCDEntry> populated with type-flagged entries - br/tb/bl: isInteger=true (serializer rounds to nearest 100, omits zero) - nor/nrr: isQuotedString=true (serializer wraps in double-quotes per SER-03) - bs: isBoolToken=true, pushed only when bufferStarvation=true (SER-06) - com.comcast-dns/fb/lb: bare tokens (not isInteger) to preserve raw values - Group mapping, ot-token selection, and nor/nrr branch logic preserved - SerializeToCMCDMap merges into map; base CMCD-Session: entry survives
…MCDEntry via SerializeToCMCDMap
- Add #include "CMCDSerializer.h" to both files
- Replace CMCDObject+headerName concatenation with CMCDEntry{"ot","m"/"s",Object}
- Remove unused delimiter/headerName/headerValue locals
- SerializeToCMCDMap merges into map; base CMCD-Session: entry preserved
… base + serializer + all subclasses - New directory support/aampmetrics/test/tests/CMCDSerialization/ with CMakeLists.txt - CMakeLists links REAL CMCDHeaders.cpp, CMCDSerializer.cpp, and all four subclass .cpp (no Fake) - CMCDSerializationGTest.cpp runner file (mirrors VideoCMCDHeadersGTest.cpp pattern) - Registered add_subdirectory(CMCDSerialization) in tests/CMakeLists.txt
…07, CMP-01, CMP-02 - 22 TEST cases covering all Phase 1 requirements - SER-01/02: br/tb/bl rounding assertions (EXPECT_THAT HasSubstr and EXPECT_EQ) - SER-03: sid/nor/nrr quoted-string assertions - SER-04: CMCD-Object exact sort "br=3800,ot=v,tb=6000"; Request comcast-* before nor - SER-05: exact group key names with single trailing ':'; no double-colon variants - SER-06: bs bare token when starving; CMCD-Status: absent when not - SER-07: group mapping correctness across all four groups - CMP-01: com.comcast-dns/fb/lb retained in CMCD-Request (both nor and nrr branches) - CMP-02: empty-state DoesNotCrash and Manifest integration; gate note in comment - Integration test: base Session and subclass Object/Request coexist (merge verified)
…est CMakeLists - VideoCMCDHeaders/CMakeLists.txt: added CMCDSerializer.cpp to ABR_SOURCES - AudioCMCDHeaders/CMakeLists.txt: added CMCDSerializer.cpp to ABR_SOURCES - SubtitleCMCDHeaders/CMakeLists.txt: fixed pre-existing double-set bug (line 38 overwrote line 37 dropping SubtitleCMCDHeaders.cpp) and added CMCDSerializer.cpp; subclass .cpp files now call SerializeToCMCDMap which requires the serializer symbol Each binary still links the Fake library for no-op CMCDHeaders base setters (unchanged). No test assertion changes needed: all three existing tests call BuildCMCDCustomHeaders without asserting output, so no old non-compliant format to update.
…c values Wrap std::stoi in a try/catch and add an early-continue for empty values so a CMCDEntry with isInteger=true and a non-numeric or empty value string omits the key rather than terminating the calling thread with an uncaught std::invalid_argument or std::out_of_range exception. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The CMCDSerializer binary tests pure free functions (RoundToNearest100, QuoteString, SerializeToCMCDMap) that have no dependency on CMCDHeaders. Linking the Fake library contradicted the comment at lines 36-39 of this same file, risked ODR violations on stricter toolchains, and created a brittle dependency where a Fake change could break an unrelated unit test. Matches the Fake-free link line already used by CMCDSerialization/CMakeLists.txt. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… headers When dnsLookUptime==0, mNextRange is empty, and nextUrl is also empty (the default-constructed state), the serializer was emitting nor="" which violates CTA-5004's optional-key rule. Also fixes the dns branch which had the same exposure. - else branch: changed from unconditional to else-if (!nextUrl.empty()) - dns branch: added inner guard so nor is only pushed when nextUrl non-empty Also corrects the misleading "merges without clearing" Step 3 comments in both subclasses to accurately describe the disjoint-group-key behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add `if (value <= 0) return 0;` as the first statement so negative sentinel values (e.g. -1 for "unknown bitrate") and zero return 0 immediately rather than risking signed-integer overflow when value is near INT_MIN (INT_MIN + 50 is UB in C++). SerializeToCMCDMap already omits entries when the rounded result is 0, so this also silently drops unset/invalid integer fields. Update the Doxygen comment in CMCDSerializer.h to document the new contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously the function assigned (overwrote) out[kv.first] for each group, so a second SerializeToCMCDMap call targeting the same CMCD header group would silently discard the first call's tokens. The new implementation: if a group key already exists in out, splits both the pre-existing comma-delimited token string and the new tokens, combines them, re-sorts the combined set alphabetically by CMCD key name, and rejoins. This makes the function safe for multi-call patterns where, for example, a base class seeds CMCD-Session and a subclass adds further Session keys. Update the Doxygen comment in CMCDSerializer.h to document the merge contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CMCDHeaders has no std::mutex member and no locked method. The include created an unnecessary transitive dependency in every TU that includes CMCDHeaders.h (including StreamAbstractionAAMP.h consumers). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… merge semantics Three new tests in CMCDSerializationTest.cpp: 1. CMCDSerialization_Video/NorOmittedWhenNextUrlEmpty (IN-03/WR-01): Asserts that a default-constructed VideoCMCDHeaders with no nextUrl set does not emit nor= in CMCD-Request. Locks the WR-01 guard. 2. CMCDSerialization_Audio/NorOmittedWhenNextUrlEmpty (WR-01): Same assertion for AudioCMCDHeaders. 3. CMCDSerialization_Merge/TwoCallsSameGroupMergesAndSorts (WR-03): Calls SerializeToCMCDMap twice into the same CMCD-Session group and asserts the result contains all tokens from both calls, alpha-sorted (sf < sid < st). Locks the WR-03 merge-semantics fix. Also add #include "CMCDSerializer.h" needed by the direct SerializeToCMCDMap call in the merge test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Headers base - Add mStreamingFormat, mStreamType, mContentId (std::string), mPlaybackRate (float) protected members - Initialize mPlaybackRate to 1.0f in constructor so pr is omitted by default - Declare and implement SetStreamingFormat, SetStreamType, SetContentId, SetPlaybackRate virtual setters - Doxygen /** */ on each declaration per conventions
…aders - Add FormatPlaybackRate static helper using snprintf %g (strips trailing zeros) - Add #include <cstdio> for snprintf - Push v=1 (always), sf (if non-empty), st (if non-empty), pr (if rate != 1.0f), cid (if non-empty, isQuotedString=true) into the same entries vector as sid - Single SerializeToCMCDMap call produces alpha-sorted cid<pr<sf<sid<st<v - CMCDSerialization target builds cleanly (verified locally)
…ckRate to AampCMCDCollector
- Add DrmMediaFormat.h include so MediaFormat type is visible in header
- Declare three new public methods with Doxygen (mirrors SetBitrates style)
- Implement MediaFormatToSf() file-local helper: DASH->"d", HLS/HLS_MP4->"h",
Smooth->"s", PROGRESSIVE/OTA/HDMI/COMPOSITE/RMF/UNKNOWN->"" (sf omitted)
- CMCDSetSessionParams: mutex-locked, bCMCDEnabled-gated, loops all
mCMCDStreamData entries calling SetStreamingFormat + SetContentId (KEYS-07, KEYS-01)
- CMCDSetLiveStatus: loops all entries calling SetStreamType("l"/"v") (KEYS-08)
- CMCDSetPlaybackRate: loops all entries calling SetPlaybackRate(rate) (KEYS-06)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ackRate call sites - priv_aamp.cpp after mCMCDCollector->Initialize(): derive strippedContentId from mManifestUrl by finding first '?' or '#' (T-02-IL: strips auth tokens from cid), then call CMCDSetSessionParams(mMediaFormat, strippedContentId) (KEYS-07 + KEYS-01) - priv_aamp.cpp NotifySpeedChanged end: call CMCDSetPlaybackRate(rate) so all trick-play entries and return-to-1x transitions propagate pr (KEYS-06) - fragmentcollector_hls.cpp after aamp->SetIsLive(context->IsLive()): call aamp->mCMCDCollector->CMCDSetLiveStatus(context->IsLive()) (KEYS-08) - fragmentcollector_mpd.cpp after aamp->SetIsLive(mIsLiveManifest): call aamp->mCMCDCollector->CMCDSetLiveStatus(mIsLiveManifest) (KEYS-08) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…KEYS-01/06/07/08/09) - Fix 6 stale assertions that expected Session=sid only; now include v=1 (KEYS-09 from 02-01) - Add 16 new TEST(CMCDSerialization_Session, ...) cases covering: - VersionAlwaysPresent: KEYS-09 exact sid+v=1 assertion - SfHls/SfDash/SfSmooth/SfOmittedWhenEmpty: KEYS-07 sf mapping + omission - StLive/StVod/StOmittedBeforeSet: KEYS-08 st mapping + omission - PrPresentWhenNotNormal/PrFractional/PrOmittedAtNormalRate: KEYS-06 pr with trailing-zero strip lock - CidQuotedWhenSet/CidOmittedWhenEmpty: KEYS-01 quoted cid + omission - AllSixKeysAlphaSortedLive/AllSixKeysWithPrTrickPlay: SER-04 exact wire-format lock-ins - SidStillFirstAmongSlikeKeys: sf<sid<st ordering guard via EXPECT_LT - No CMakeLists change (binary already links real CMCDHeaders.cpp; no Fake used) - Suite result: 41 tests, 41 passed (25 existing + 16 new)
The header file had `@file AampCMCDCollector.cpp` — wrong filename. Change to `@file AampCMCDCollector.h` to match the actual file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SetTrackData is called from PrivateInstanceAAMP::SetCMCDTrackData on download threads, concurrent with CMCDGetHeaders (which already locks myMutex). The stale comment claiming it was an internal GetHeaders function was wrong; the missing lock_guard was a data race on mCMCDStreamData. Add std::lock_guard<std::mutex> matching sibling methods, and update the comment to reflect actual calling context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move the query+fragment stripping of mManifestUrl from the priv_aamp.cpp call site into AampCMCDCollector::CMCDSetSessionParams. The method now accepts the raw manifest URL and strips up to the earliest of '?' or '#' before using the result as cid. This co-locates the auth-token-leak mitigation with the setter, makes it unit-testable, and simplifies the call site to a single line. Update header declaration to document the new rawUrl parameter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Call CMCDSetPlaybackRate(rate) immediately after CMCDSetSessionParams in Tune(), so the current rate member is seeded into the collector once CMCD is enabled. Fixes the edge case where NotifySpeedChanged fires before Initialize (e.g. auto-resume GLib idle task at line 360), causing the rate to be silently dropped because bCMCDEnabled was false. Without this seed the first CMCD emission after fast-resume trick-play would omit the pr key, violating KEYS-06. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ull check Three new call sites introduced in Phase 2 dereference aamp->mCMCDCollector without a null guard: CMCDSetLiveStatus in fragmentcollector_hls.cpp:2230, CMCDSetLiveStatus in fragmentcollector_mpd.cpp:3038, and SetBitrates(Audio) in fragmentcollector_mpd.cpp:7366. Wrap each with `if (aamp->mCMCDCollector)` matching the existing guard pattern at fragmentcollector_mpd.cpp:7959 to prevent latent crashes on torn-down sessions during fast channel-change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…omission guard Replace `mPlaybackRate != 1.0f` with `std::fabs(mPlaybackRate - 1.0f) > 1e-4f` in CMCDHeaders::BuildCMCDCustomHeaders. While 1.0f has an exact IEEE-754 representation and AAMP_NORMAL_PLAY_RATE promotes to exactly 1.0f, a future double-to-float conversion near 1.0 could silently emit or suppress pr. The epsilon guard is robust to that without affecting genuine trick-play rates. Add <cmath> include for std::fabs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add CMCDSerialization_Session::AllSixKeysSortedWithCidAndPr which asserts the exact wire format when all six CMCD-Session keys (cid, pr, sf, sid, st, v) are simultaneously present. Locks the alpha-sort boundary cid<pr<sf<sid<st<v which is not covered by the five-key tests above (which omit either pr or cid). 42 tests all pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… fields and setters to CMCDHeaders base - Add int mFragmentDuration (d key), int mMeasuredThroughput (mtp key), bool mStartupUrgent (su key) as protected members - Initialize all three to 0/0/false in the constructor initializer list - Declare and implement SetFragmentDuration, SetMeasuredThroughput, SetStartupUrgent virtual setters - No change to BuildCMCDCustomHeaders, Session keys, or pre-existing setters
…ubclasses
- Add #include <algorithm> and #include <cmath> to both subclass .cpp files
- d (KEYS-02): CMCDEntry{Object, all-flags-false} guarded mFragmentDuration>0 — exact ms, no rounding
- dl (KEYS-03): derived inline bufferLength/max(|mPlaybackRate|,kMinRate=0.5f), CMCDEntry{Request,isInteger=true}
- mtp (KEYS-04): CMCDEntry{Request,isInteger=true} guarded mMeasuredThroughput>0
- su (KEYS-05): CMCDEntry{Request,isBoolToken=true} guarded mStartupUrgent — bare token like bs
- rtp (KEYS-10): derived bitrate*kRtpFactor=2.0f (CTA-5004 client-discretion), CMCDEntry{Status,isInteger=true}
- ManifestCMCDHeaders and SubtitleCMCDHeaders unchanged
- Fix(Rule 1): relax KeysInCorrectGroups test EXPECT_EQ(status,"bs") to HasSubstr("bs") now that rtp also appears in Status when bitrate>0
- All 42 CMCDSerialization + 9 CMCDSerializer tests pass
…gent collector setters - CMCDSetFragmentDuration(AampMediaType, int durationMs): single-instance setter for d key - CMCDSetMeasuredThroughput(AampMediaType, int kbps): single-instance setter for mtp key - CMCDSetStartupUrgent(AampMediaType, bool): single-instance setter for su key - All three: mutex-locked, bCMCDEnabled-gated, Phase-2 CR-01 guarded (it!=end()&&it->second) - Single-instance find() pattern (not all-instances loop) — per-download values
- SetCMCDTrackData: compute mtpKbps=GetNetworkBandwidth()/1000, call CMCDSetMeasuredThroughput - SetCMCDTrackData: compute suFlag=bufferRedStatus||IsTuneTypeNew, call CMCDSetStartupUrgent (recomputed every call so su clears after startup — RESEARCH Pitfall 3) - GetFile: after mmediaT remap, call CMCDSetFragmentDuration(mmediaT, fragmentDurationMs) gated to eMEDIATYPE_VIDEO||eMEDIATYPE_AUDIO only (init/manifest excluded) uses remapped mmediaT to hit correct header instance (RESEARCH Pitfall 4) - All calls match existing mCMCDCollector usage pattern (always-allocated, no extra null guard)
…ation - KEYS02: d present/omit/no-round (Video + Audio), absent in ManifestCMCDHeaders - KEYS03: dl present/rounded-100ms/omit-zero/scaled-by-rate - KEYS04: mtp present/rounded-100kbps/omit-zero - KEYS05: su bare token when true, omitted when false (no "su=") - KEYS10: rtp present/rounded-100kbps/omit-zero-bitrate (derived bitrate*2) - FullSet lock-in: exact CMCD-Object/Request/Status strings for normal play and startup - Suite: 61/61 CMCDSerialization + 9/9 CMCDSerializer all pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…MCD key set - SER01: named tests for mtp (4842->4800) and rtp (3842->7700) rounding to 100 kbps - SER02: named test for dl rounding (2350->2400) and explicit d-not-rounded lock (2150 stays 2150) - SER04: exact alpha-sort assertions for Object (br<d<ot<tb), full Request group (bl<com.comcast-*<dl<mtp<nor<su), and Status (bs<rtp) - SER06: su bare-token true/false coverage paired with existing bs tests - SER07: group-mapping tests for d->Object, dl/mtp/su->Request, rtp->Status - All 72 CMCDSerialization tests pass; CMCDSerializer still 9/9 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… null guard
CR-01: In GetFile, add else-if branch for eMEDIATYPE_INIT_VIDEO/INIT_AUDIO that
calls CMCDSetFragmentDuration(mmediaT, 0) to clear mFragmentDuration on the shared
VIDEO/AUDIO CMCDHeaders instance before CMCDGetHeaders. Prevents the preceding
media-segment d value from leaking into init-segment CMCD-Object headers (CTA-5004
§3 violation). The outer if(mCMCDCollector) guard also absorbs the pre-existing
unconditional CMCDGetHeaders call, hardening the entire block.
WR-02: Wrap the CMCDSetPlaybackRate call in NotifySpeedChanged inside
if (mCMCDCollector) { ... } to match the defensive guard style used at all
other mCMCDCollector call sites and protect against null if the construction
path is extended or the GLib idle task fires before mCMCDCollector is set.
Both changes are inspection-verified (libaamp is not locally buildable on this host).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CR-01 regression (CR01_FragmentDurationClearedToZeroOmitsD): locks the header-level clearing behavior — SetFragmentDuration(2000) then SetFragmentDuration(0) must produce CMCD-Object with no "d=" token, proving that a zero value clears the stale duration from a prior media-segment request. WR-01 Audio coverage (KEYS10_RtpPresentForAudio, FullAudioRequest_AllNewKeysNormalPlay, FullAudioRequest_AllNewKeysStartup): closes the gap where rtp, dl, mtp, and su had no AudioCMCDHeaders tests. Full-set lock-in asserts exact CMCD-Object / Request / Status strings for both su=false and su=true scenarios on the Audio subclass. IN-01 negative-rate dl (KEYS03_DlNegativeRateTreatedAsAbsolute): locks the fabs() branch — SetPlaybackRate(-2.0f) + SetBufferLength(4000) must produce dl=2000, identical to the positive-rate equivalent, confirming reverse trick-play rates are handled correctly. Test counts: 72 → 77 (5 new). All 77 CMCDSerialization + 9 CMCDSerializer pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The .planning/ directory holds local-only GSD planning artifacts and is gitignored. Untracking PROJECT.md and the codebase maps that were committed before the gitignore was added; local copies are retained on disk. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
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
2 participants
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.
No description provided.