Poc/L3D-DASH-CMCDV2 #125
Poc/L3D-DASH-CMCDV2 #125
Conversation
…t object with updated properties
Cmcdv2/response mode
Cmcdv2/unit test implementation for cmcd model
Cmcdv2/playhead bitrate implementation
Cmcdv2/top playable bitrate implementation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Cmcdv2/bsd key
* feat(cmcdmodel): add lowest aggregated bitrate key lab calculation * feat(cmcdmodel): add lowest aggreagted bitrate key lab to event mode data
* refactor(cmcdmodel): use active stream to access bitrate instead of player instance * test(cmcdmodel): add unit tests for aggregated bitrate keys (ab, lab, tab)
…onal-keys feat(cmcdmodel): add aggregated bitrate keys (ab, lab, tab) to event mode payload
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @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 initial support for handling sub-segments in DASH streams, specifically addressing the 'S@k' attribute within the SegmentTimeline. This enhancement allows the player to correctly identify and request individual sub-segments, which is crucial for advanced DASH features like low-latency streaming or precise segment access. It lays the groundwork for more granular control over segment fetching and playback.
Highlights
- Support for DASH Sub-segments (S@k): Introduction of logic to process and utilize the 'k' attribute from DASH SegmentTimeline, enabling the identification and handling of sub-segments within a larger segment.
- Enhanced Segment Requesting: Modification of the segment request mechanism to include 'subNumber' for more granular segment identification, ensuring correct sub-segment fetching.
- New Constant for Segment Sequence Representations: Addition of a new constant 'SEGMENT_SEQUENCE_REPRESENTATIONS' ('urn:mpeg:dash:ssr:2023') to recognize specific DASH extensions related to segment sequencing.
- Internal Data Model Updates: Extension of the 'Segment' and 'Representation' data models to store 'subNumber' and 'k' values respectively, facilitating internal tracking of sub-segment information.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces support for Segment Sequence Representations (SSR) in DASH, a feature for handling segments composed of multiple sub-segments. The changes span across settings, manifest parsing, and segment request logic. While the implementation seems to be on the right track for this new feature, I've identified a few issues that need attention. There is a critical logic issue in DashHandler.js that could cause playback problems, and a potential for a runtime error in DashManifestModel.js due to insufficient validation. Additionally, there are some leftover development artifacts like a TODO comment and a console.log that should be cleaned up before merging. Please address these points to ensure the stability and correctness of the new functionality.
| nextSubNumber = 0; | ||
| } else { | ||
| nextSubNumber ++; | ||
| lastSegment = previousSegment; |
There was a problem hiding this comment.
Setting lastSegment to previousSegment seems incorrect and could lead to playback issues. previousSegment could be null if the current segment is the first one. Even if it's not null, this would effectively move the player's state backwards, potentially causing it to get stuck re-requesting the same segment. The logic in _getRequest for handling sub-segments seems more appropriate, where lastSegment is only updated when all sub-segments of a segment have been dealt with. Please reconsider this logic.
| // TODO: double check this logic | ||
| nextSubNumber = representation.k - Math.ceil((time - segment.mediaStartTime) / | ||
| (segment.representation.segmentDuration / representation.k)); |
There was a problem hiding this comment.
A TODO comment has been left in the code. This indicates unresolved questions about the logic. Please resolve this and remove the comment before merging.
| if (segmentInfo.SegmentTimeline.S[0].hasOwnProperty('k')) { | ||
| voRepresentation.k = segmentInfo.SegmentTimeline.S[0].k || 1; | ||
| } |
There was a problem hiding this comment.
This code accesses segmentInfo.SegmentTimeline.S[0] without checking if segmentInfo.SegmentTimeline.S exists and is a non-empty array. This could lead to a runtime error if S is not present or empty. Please add a defensive check.
| if (segmentInfo.SegmentTimeline.S[0].hasOwnProperty('k')) { | |
| voRepresentation.k = segmentInfo.SegmentTimeline.S[0].k || 1; | |
| } | |
| if (segmentInfo.SegmentTimeline.S && segmentInfo.SegmentTimeline.S.length > 0 && segmentInfo.SegmentTimeline.S[0].hasOwnProperty('k')) { | |
| voRepresentation.k = segmentInfo.SegmentTimeline.S[0].k || 1; | |
| } |
| { schemeIdUri: Constants.EXT_URL_QUERY_INFO_SCHEME }, | ||
| { schemeIdUri: Constants.MATRIX_COEFFICIENTS_SCHEME_ID_URI, value: /0|1|5|6/ }, | ||
| { schemeIdUri: Constants.TRANSFER_CHARACTERISTICS_SCHEME_ID_URI, value: /1|6|13|14|15/ }, | ||
| { schemeIdUri: Constants.SEGMENT_SEQUENCE_REPRESENTATIONS, }, |
There was a problem hiding this comment.
There's a trailing comma on this line which is inconsistent with the surrounding code. Please remove it to maintain a consistent style.
{ schemeIdUri: Constants.SEGMENT_SEQUENCE_REPRESENTATIONS }| */ | ||
| function getValidTimeAheadOfTargetTime(time, mediaInfo, representation, targetThreshold) { | ||
| try { | ||
| console.log('l3d getValidTimeAheadOfTargetTime'); |
There was a problem hiding this comment.
A console.log statement has been left in the code. Please remove this debug statement.
No description provided.