PoC: SGAI alterantive cmcd vast events#161
Draft
cotid-qualabs wants to merge 343 commits intodevelopmentfrom
Draft
PoC: SGAI alterantive cmcd vast events#161cotid-qualabs wants to merge 343 commits intodevelopmentfrom
cotid-qualabs wants to merge 343 commits intodevelopmentfrom
Conversation
…s into mpd-lists/sample-page
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
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…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
* 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
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 introduces significant enhancements to the player's capabilities for dynamic content delivery and client-side media data reporting. It integrates CMCDv2 for advanced analytics, implements support for alternative media presentations (such as ad insertion) through MPD events, and adds functionality for handling List MPD manifests. These changes aim to provide more flexible and robust solutions for content monetization and quality of experience monitoring. 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. Changelog
Activity
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 new functionality, including CMCDv2 integration, List MPD support, and Alternative Media Presentations. The architecture, especially the refactoring of CMCD logic into a dedicated controller and the addition of controllers for List MPDs and Alternative Media, is a major step forward. However, the implementation introduces a critical security vulnerability: the player makes network requests to URLs provided directly in the MPD manifest without validation. This can be exploited by a malicious manifest to perform forced requests from the user's browser (a form of client-side SSRF), potentially leading to data leakage (e.g., session IDs) or interaction with internal network services. It is strongly recommended to implement URL validation or allow-listing for all URLs extracted from the manifest that trigger network requests. Additionally, I've identified a few minor bugs and areas for potential refactoring to improve clarity and type safety.
Comment on lines
65
to
67
| this.LOADING_ABANDONED = 'loadingAborted'; | ||
| this.LOADING_COMPLETED = 'loadingCompleted'; | ||
| this.LOADING_DATA_PROGRESS = 'loadingDataProgress'; |
There was a problem hiding this comment.
The properties LOADING_ABANDONED, LOADING_COMPLETED, and LOADING_DATA_PROGRESS are defined twice in the constructor. These duplicated lines should be removed to avoid potential confusion and bugs.
|
|
||
| // Replace | ||
| this.returnOffset = NaN; | ||
| this.returnOffset = NaN; |
There was a problem hiding this comment.
The property returnOffset is initialized twice. This duplicate initialization is redundant and should be removed.
| ...cmcdModel.updateMsdData(Constants.CMCD_REPORTING_MODE.REQUEST), | ||
| }; | ||
|
|
||
| request.cmcd = cmcdData; //TODO: wrong because cmcdData only has data from model, not complete data with reporter |
There was a problem hiding this comment.
The request.cmcd property is being assigned an incomplete set of CMCD data, as noted in the TODO comment. It's missing stateful data managed by the CmcdReporter (like sn). The CMCD_DATA_GENERATED event triggered later will therefore contain this incomplete data. To fix this, request.cmcd should be assigned the complete data object from the reporter after applyRequestReport is called, or retrieved directly from the reporter's internal state.
Comment on lines
+232
to
+239
| const httpRequest = new CmcdReportRequest(); | ||
| httpRequest.url = request.url; | ||
| httpRequest.method = request.method; | ||
| httpRequest.headers = request.headers; | ||
| httpRequest.body = request.body; | ||
| httpRequest.type = HTTPRequest.CMCD_EVENT; | ||
|
|
||
| urlLoader.load({ |
There was a problem hiding this comment.
The CMCD v2 implementation allows the MPD manifest to specify reporting targets via the ReportingTargets element. The CmcdController uses these URLs to send playback reports without any validation or allow-listing. A malicious manifest can instruct the player to send requests to arbitrary URLs, potentially leaking session information or being used to interact with internal network services from the user's browser. It is recommended to implement a URL validation mechanism or an allow-list for reporting targets.
Comment on lines
898
to
899
| const url = event.messageData instanceof Uint8Array ? Utils.uint8ArrayToString(event.messageData) : event.messageData; | ||
| _sendCallbackRequest(url); |
There was a problem hiding this comment.
The EventController processes callback events from the manifest and makes network requests to the URLs provided in the event's message data. There is no validation of these URLs, allowing a malicious manifest to trigger requests to arbitrary destinations. It is recommended to validate callback URLs against an allow-list of trusted domains.
Comment on lines
+117
to
+122
| const relativePath = period.ImportedMPD.uri; | ||
| const baseUrl = period.BaseURL ?? manifest.BaseURL; | ||
| const resolvedUri = baseUrl ? baseUrl[0].__text + relativePath : relativePath; | ||
|
|
||
| const updatedManifest = new Promise(resolve => { | ||
| manifestLoader.load(resolvedUri, null, null, true) |
There was a problem hiding this comment.
The ListMpdController loads imported manifests from URLs specified in the ImportedMPD element of the main manifest. These URLs are resolved and fetched without validation, which can be exploited by a malicious manifest to force the player to fetch content from arbitrary URLs. It is recommended to restrict imported MPD URLs to a set of trusted base URLs.
| schemeIdUri: event.eventStream.schemeIdUri, | ||
| maxDuration: alternativeMpdNode.maxDuration / timescale, | ||
| alternativeMPD: { | ||
| url: alternativeMpdNode.url, |
There was a problem hiding this comment.
The AlternativeMediaController extracts alternative MPD URLs from the manifest. These URLs are later used to load content without validation. A malicious manifest could point to an arbitrary URL, causing the player to perform a forced request. It is recommended to validate these URLs before use.
Comment on lines
3757
to
3807
| export interface CmcdModel { | ||
| setup(): void; | ||
|
|
||
| reset(): void; | ||
|
|
||
| setConfig(config: object): void; | ||
|
|
||
| getCmcdData(request: HTTPRequest): object; | ||
|
|
||
| getCmcdParametersFromManifest(): CMCDParameters; | ||
| 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.
The CmcdModel interface uses any for several method parameters (onStateChange, onPlayerError, onPlaybackRateChanged, onManifestLoaded, onBufferLevelStateChanged, onEventChange). Using any reduces type safety and the benefits of TypeScript. Consider defining specific types or interfaces for these parameters to improve code clarity and prevent potential runtime errors. For example, for onPlayerError, you could use a more specific error type like DashJSError.
Comment on lines
+727
to
+769
| function _isFirstEventInSequence(event, eventsInSamePeriod, currentVideoTime) { | ||
| try { | ||
| if (!eventsInSamePeriod || !event.eventStream) { | ||
| return false; | ||
| } | ||
|
|
||
| const schemeIdUri = event.eventStream.schemeIdUri; | ||
| const eventsWithSameScheme = eventsInSamePeriod[schemeIdUri] || []; | ||
|
|
||
| // Get all events with noJump=1 from the same scheme that are not in the future | ||
| const noJump1Events = eventsWithSameScheme.filter(e => | ||
| e.alternativeMpd && | ||
| e.alternativeMpd.noJump === NO_JUMP_TRIGGER_ALL && | ||
| e.calculatedPresentationTime <= currentVideoTime | ||
| ); | ||
|
|
||
| if (noJump1Events.length === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| // Find the event with the lowest presentation time (the first one) | ||
| // While doing so, flag all subsequent events as triggered | ||
| const firstEvent = noJump1Events.reduce((earliest, current) => { | ||
| if (current.calculatedPresentationTime < earliest.calculatedPresentationTime) { | ||
| // Current event is earlier, so flag the previous (earliest) as triggered | ||
| if (!earliest.triggeredNoJumpEvent) { | ||
| earliest.triggeredNoJumpEvent = true; | ||
| } | ||
| return current; | ||
| } else { | ||
| // Earliest event is still the first one, so flag current as triggered | ||
| if (!current.triggeredNoJumpEvent) { | ||
| current.triggeredNoJumpEvent = true; | ||
| } | ||
| return earliest; | ||
| } | ||
| }); | ||
| return event.id === firstEvent.id; | ||
| } catch (e) { | ||
| logger.error(e); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The use of reduce in _isFirstEventInSequence (and similarly in _isLastEventInSequence) to both find an item and apply side effects to other items in the array can be confusing. A more readable approach would be to separate these concerns: first, find the target event (earliest/latest), and then iterate through the array to apply side effects. This would make the function's intent clearer and improve maintainability.
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
10 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.
Alternative MPD and List MPD implementation with: