Conversation
Co-authored-by: Sebastian Piquerez <89274285+sebastianpiq@users.noreply.github.com>
…s into mpd-lists/sample-page
[MPDList] Add sample page for List MPDs
[MPDList] Fix MPD Lists BaseURI issues
…ds-bug [MPDList] Fix handling of periods with duration but without start time
…eOffset-default [AlternativeMPD] Add default (60 seconds) to the erliestResolutionTimeOffset
* Fix bug with the period duration precedense * Don't update duration if the manifest have a media presentation duration
…nto cmcdv2/cmcd-parameters
…v2/cmcd-parameters
…/alternative-cmcd
…/alternative-cmcd
…v2/cmcd-parameters
* feat: implement CMCD v2 functionality * remove unused function * Update src/dash/vo/EventTarget.js Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * cmcdvs parameters unit test * fix unit tests * add batch config from mpd --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…nto sgai/alternative-cmcd
Summary of ChangesHello @cotid-qualabs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the player's capabilities by integrating advanced DASH features such as CMCDv2, Alternative MPD events, and List MPDs. These additions provide greater flexibility for content providers to dynamically manage and report on media playback, enabling use cases like server-side ad insertion and seamless content transitions. The changes involve substantial refactoring of internal components to support these new functionalities, ensuring a more modular and maintainable codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements across several core functionalities, primarily focusing on CMCD v2 support, List MPD implementation, and Alternative Media Presentations. Key changes include expanding the CmcdModel interface with new event handling methods and CMCD data retrieval functions, alongside adding new sample pages to demonstrate CMCD v2 reporting with network interceptors, List MPD use cases, and various Alternative Media Presentation scenarios (VOD, Live, Insert/Replace modes, executeOnce, clip, and returnOffset functionalities). Core logic updates involve modifying DashAdapter and DashManifestModel to handle merging and parsing of List MPDs and Alternative MPD events, introducing new Value Objects like AlternativeMpd and EventTarget, and updating Settings and CoreEvents to support these new features. The CMCD implementation has been refactored to use a CmcdConfigAccessor for unified configuration access and a CmcdBatchController for batched reporting, with corresponding updates to MediaPlayer, HTTPLoader, and ProtectionController to integrate these new CMCD mechanisms. Review comments highlighted the need for more specific TypeScript types in the CmcdModel interface, removal of redundant code, clarification of getKeysForJsonMode naming and implementation for CMCD body mode, correction of a JSDoc type for minEarliestResolutionTimeOffset, and a suggestion to relax a strict condition in mergeManifests regarding minBufferTime for period replacement.
| onStateChange(state: any): void; | ||
|
|
||
| getHeaderParameters(request: HTTPRequest): object | null; | ||
| onPeriodSwitchComplete(): void; | ||
|
|
||
| getQueryParameter(request: HTTPRequest): { key: string, finalPayloadString: string } | null; | ||
| onPlaybackStarted(): void; | ||
|
|
||
| initialize(): void; | ||
| onPlaybackPlaying(): void; | ||
|
|
||
| isCmcdEnabled(): boolean; | ||
| onRebufferingStarted(mediaType: string): void; | ||
|
|
||
| reset(): void; | ||
| onRebufferingCompleted(mediaType: string): void; | ||
|
|
||
| setConfig(config: object): void; | ||
| onPlayerError(errorData: any): void; | ||
|
|
||
| onPlaybackSeeking(): void; | ||
|
|
||
| onPlaybackSeeked(): void; | ||
|
|
||
| onPlaybackRateChanged(data: any): void; | ||
|
|
||
| wasPlaying(): boolean; | ||
|
|
||
| onManifestLoaded(data: any): void; | ||
|
|
||
| onBufferLevelStateChanged(data: any): void; | ||
|
|
||
| updateMsdData(mode: string): object; | ||
|
|
||
| resetInitialSettings(): void; | ||
|
|
||
| getCmcdParametersFromManifest(): CMCDParameters; | ||
|
|
||
| triggerCmcdEventMode(event: string): object; | ||
|
|
||
| getGenericCmcdData(mediaType?: string): object; | ||
|
|
||
| isIncludedInRequestFilter(type: string, includeInRequests?: any): boolean; | ||
|
|
||
| getLastMediaTypeRequest(): string; | ||
|
|
||
| onEventChange(state: any): void; |
There was a problem hiding this comment.
Many of the new methods in the CmcdModel interface use any as a parameter type. While this works, it reduces the benefits of TypeScript's static type checking. Consider using more specific types where possible. For example:
onStateChange(state: string): void;(or an enum for states)onPlayerError(errorData: DashJSError): void;onPlaybackRateChanged(data: { playbackRate: number }): void;onManifestLoaded(data: { manifest: object }): void;
This will improve type safety and make the API easier to use correctly.
| <script> | ||
| function getKeysForQueryMode(cmcdString) { | ||
| var cmcdData = {}; | ||
| var cmcdString = cmcdString; |
There was a problem hiding this comment.
This line is redundant and has no effect. It can be safely removed.
| function getKeysForJsonMode(event){ | ||
| const decoded = decodeURIComponent(event.cmcdString); | ||
| const entries = decoded.split(','); | ||
|
|
||
| const cmcdData = {}; | ||
|
|
||
| for (let entry of entries) { | ||
| let [key, value] = entry.split('='); | ||
|
|
||
| if (value?.startsWith('"') && value.endsWith('"')) { | ||
| value = value.slice(1, -1); | ||
| } | ||
|
|
||
| if (!isNaN(value) && value.trim() !== '') { | ||
| value = Number(value); | ||
| } | ||
|
|
||
| cmcdData[key] = value; | ||
| } | ||
| return cmcdData; | ||
| } |
There was a problem hiding this comment.
The function getKeysForJsonMode seems to be misnamed and its implementation might not align with the CMCD specification for body transmission mode. The CMCD specification states that when using HTTP POST (which body mode implies), the payload should be a JSON object. This function, however, parses a comma-separated key-value string, similar to query parameters.
If the event.cmcdString for body mode is indeed a JSON string, you should use JSON.parse(). If it's a custom format, the function should be renamed to avoid confusion (e.g., getKeysFromEncodedString). Please verify the format of event.cmcdString for CMCD_MODE_BODY and adjust accordingly.
|
|
||
| /** | ||
| * @typedef {Object} listMpdSettings | ||
| * @property {boolean} [minEarliestResolutionTimeOffset=0] |
There was a problem hiding this comment.
The JSDoc for minEarliestResolutionTimeOffset specifies the type as {boolean}, but the default value is 0 and its usage implies it's a number. The type should be {number}.
| * @property {boolean} [minEarliestResolutionTimeOffset=0] | |
| * @property {number} [minEarliestResolutionTimeOffset=0] |
| if (newPeriod.minBufferTime && (newPeriod.AdaptationSet || newPeriod.duration === 0)) { | ||
| manifest.Period[periodIndex] = newPeriod; | ||
| } else { | ||
| manifest.Period.splice(periodIndex, 1); | ||
| } |
There was a problem hiding this comment.
The condition for replacing or removing a period seems a bit strict. If newPeriod.minBufferTime is not present, the period will be removed from the manifest. Is this the intended behavior? It seems possible for a valid imported manifest to lack a top-level minBufferTime, which could lead to the period being unexpectedly removed. Consider relaxing this condition or adding a comment to explain why minBufferTime is a mandatory check here.
| alternativeMpd.clip = event.clip ? !(event.clip === 'false') : true; | ||
| alternativeMpd.startWithOffset = event.startWithOffset ? event.startWithOffset === 'true' : false; |
There was a problem hiding this comment.
The boolean conversions for clip and startWithOffset can be simplified for better readability.
| alternativeMpd.clip = event.clip ? !(event.clip === 'false') : true; | |
| alternativeMpd.startWithOffset = event.startWithOffset ? event.startWithOffset === 'true' : false; | |
| alternativeMpd.clip = event.clip !== 'false'; | |
| alternativeMpd.startWithOffset = event.startWithOffset === 'true'; |
* cmcd reporter initialization * request mode migration * event mode migration * cmcd model migration * fix cmcd model unit tests * fixes for cmcd parameters and cleanup - protection controller fixes WIP * cleanup and update unit tests * refactor unit tests and fixes * fix unit tests and remove batchTimer
* ab, lab and tab inner list and request mode * ab, tab and lab inner list for v2 * bl inner list and event mode * br inner list and event mode * toInnerList helper * bsd inner list * mtp inner list and event mode * nor inner list * pb inner list and event mode * tp inner list and event mode * tpb inner list and event mode * fix unit tests
…v2/cmcd-parameters
…/alternative-cmcd
…nto sgai/alternative-cmcd
…/alternative-cmcd
* chore: update CML dependencies * fix: update common media request * fix: update resourceTiming properties to use performance.now() * fix: update cmcd data formatting * fix: remove redundant rr values * refactor: simplify cmcd reporting * fix: test mock requests missing parameters * chore: update cml cmcd version * should not send report if events are undefined * fix unit tests --------- Co-authored-by: cotid-qualabs <constanzad@qualabs.com>
…/alternative-cmcd
11 participants
Alternative MPD and List MPD implementation with CMCDv2 integration