feat: cmcdv2/df-key#17
Conversation
cmcdv2/response mode
Merged MSD event listeners with Event mode event listeners
…quest, response and event mode
…ent event mode timer setup
- timeInterval can now be turned off by setting it to 0 - set default time interval for event mode to 10 seconds
…orting in CmcdManager
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…anagement into separate methods
…isten to player events
![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 @juanmanuel-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 CMCD (Common Media Client Data) implementation by adding support for CMCD v2 event mode and the 'df' (dropped frames) key. The changes enable the system to capture and report a wide array of player events and states, such as playback lifecycle, user interactions like mute/unmute and fullscreen changes, and application visibility. New configuration options provide fine-grained control over event reporting, and the integration with player statistics allows for the inclusion of dropped frame data, which is crucial for quality of experience monitoring. This feature provides more comprehensive client-side metrics to optimize content delivery.
Highlights
- CMCD v2 Event Mode Implementation: A new CMCD mode, 'EVENT', has been introduced to enable reporting of various player events and states, providing richer client-side data.
- Dropped Frames ('df') Key: The 'df' key, representing an absolute count of dropped frames since session initiation, has been added to CMCD data, allowing for better quality of experience monitoring.
- Comprehensive Event Reporting: The CMCD manager now captures and reports a comprehensive range of player events, including playback state changes (start, playing, paused, waiting, seeking, rebuffering, preloading, ended), mute/unmute, player expand/collapse (fullscreen changes), and application background/foreground state.
- Configurable Event Reporting: New configuration options, 'events' and 'timeInterval', have been added to 'CmcdTarget', providing granular control over which events are reported and the frequency of periodic reports.
- Improved 'msd' Handling: The 'msd' (Measured Start Delay) key is now specifically handled to be reported only on the first relevant event (e.g., playback start) in event mode, ensuring accurate initial reporting.
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 CMCDv2 event reporting and adds support for the df (dropped frames) key. The implementation is well-structured, including new event listeners, timers for interval-based reporting, and comprehensive tests. I have a few suggestions to improve efficiency, ensure correctness according to the specification, and clean up the tests.
| } | ||
|
|
||
| const stats = this.player_.getStats(); | ||
| data.df = stats.droppedFrames; |
There was a problem hiding this comment.
The CMCD specification for the df (droppedFrames) key states that it "should only be sent for content types of v, av or o". This implementation sends it for all requests. Please add a condition to only include df for the appropriate object types.
For example:
if (data.ot && ['v', 'av', 'o'].includes(data.ot)) {
data.df = stats.droppedFrames;
}| const allowedKeys = Array.from(new Set([ | ||
| ...shaka.util.CmcdManager.CmcdKeys.V2CommonKeys, | ||
| ...shaka.util.CmcdManager.CmcdKeys.V2EventKeys, | ||
| ])); |
There was a problem hiding this comment.
For performance, the allowedKeys array for event mode should be calculated once and cached, as reportEvent_ can be called frequently. This can be done by caching it as a static property on shaka.util.CmcdManager or as an instance property initialized in the constructor.
| it('should return only enabled event targets', () => { | ||
| const targets = [ | ||
| {mode: 'event', enabled: true, url: 'url1'}, | ||
| {mode: 'event', enabled: false, url: 'url2'}, | ||
| {mode: 'request', enabled: true, url: 'url3'}, | ||
| {mode: 'event', enabled: true, url: 'url4'}, | ||
| ]; | ||
| const cmcdManager = createCmcdManager(playerInterface, {targets}); | ||
| const enabledEventTargets = cmcdManager.getEventModeEnabledTargets_(); | ||
| expect(enabledEventTargets.length).toBe(2); | ||
| expect(enabledEventTargets[0].url).toBe('url1'); | ||
| expect(enabledEventTargets[1].url).toBe('url4'); | ||
| }); |
There was a problem hiding this comment.
This test case appears to be a duplicate of the one at lines 2007-2019. Please remove this duplicate to improve test suite maintainability.
2 participants
Note: needs base changes from Event Mode to include Player in CmcdManager: