Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: CMCD v2 LTC and MSD keys #7412

Merged
merged 19 commits into from
Nov 29, 2024
Merged

Conversation

JoaquinBCh
Copy link
Contributor

@JoaquinBCh JoaquinBCh commented Oct 10, 2024

This PR introduces two new CMCD v2 keys to shaka player:

  • ltc (live stream latency): The time delta between when a given media timestamp was made available at the origin and when it was rendered by the client.
  • msd (media start delay): Measures the initial delay in wall-clock time from when a player is instructed to play media for a given session to when any media begins playback. (This is sent only once when it is calculated)

While this update does not implement full CMCD v2 support, it extends the CMCD v1 implementation to handle these two new values when requested.

Summary:

  • Updated the player settings: cmcd.reporting.requestMode.version must be set to 2 to use these new CMCD v2 keys.
  • Implemented logic to retrieve the values for ltc and msd if requested and if the version is set to 2.
  • Implemented logic to send the msd value only once.
  • Created getLiveLatency() method in player to calculate the latency and use it for ltc.
  • Created unit tests for the two new keys (ltc and msd) to ensure their correct behavior when cmcd.reporting.requestMode.version is set to 2.

Testing:

  • Verified that the ltc and msd keys are included when the reporting version is set to 2 and these keys are requested. And the msd key is sent only once when it is calculated.
  • Verified that these keys are not included when the version is set to 1 or it is not set.

How to Test manually:

Test that msd and ltc values are sent correctly

Set the following configuration to the player:

player.configure({
  cmcd: {
    enabled: true,
    version: 2,
    includeKeys: ['sid', 'cid', 'v', 'msd', 'ltc'],
  }
});

Verify that the msd value is sent only once after being calculated, and the ltc value is sent with every request.

Test the msd value is calculated correctly

Use throttling in your browser to slow down your connection and reproduce the streaming.

image

The msd value should be higher than usual.

Notes:

  • This PR adds support for the ltc and msd keys but does not implement the entire CMCD v2

Copy link

google-cla bot commented Oct 10, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@shaka-bot
Copy link
Collaborator

shaka-bot commented Oct 10, 2024

Incremental code coverage: 95.77%

@tykus160 tykus160 added the type: enhancement New feature or request label Oct 11, 2024
Copy link
Member

@tykus160 tykus160 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Here are a few suggestions.
Additionally, it looks like you didn't sign CLA. Please check it.

Copy link
Member

@avelad avelad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign the CLA!

@avelad avelad added the priority: P3 Useful but not urgent label Oct 18, 2024
@avelad avelad added this to the v4.12 milestone Oct 18, 2024
@avelad avelad added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Oct 18, 2024
@joeyparrish
Copy link
Member

I would also like @littlespex to review this. This week, he and I discussed his plans for CMCD v2, so I want to make sure this does not complicate those plans.

@littlespex
Copy link
Contributor

My initial thoughts:

  1. The new CMCD keys and serialization rules will all be replaced by the CML enums and utilities. Until that happens I don't see anything wrong with defining them along with the other internal CMCD type defs.
  2. New internal functions for tracking state like getLiveLatency will be useful even after CML is introduced.
  3. Changes to the player config need thorough review as they are the only public API facing code, and we won't be able to change that later without introducing a breaking change/version. V2 adds some complexity with the different delivery modes, and calls into question some how some of the current properties like includeKeys will work with these new modes. The config changes in this PR are based on similar work in dash.js. I am reviewing this proposed structure to see if it will also work for shaka.

@JoaquinBCh
Copy link
Contributor Author

Thanks everyone for the reviews.
@avelad we've already signed the CLA, but we haven't been added as members of the organization yet. We're expecting to have this resolved as soon as possible

@JoaquinBCh JoaquinBCh requested a review from tykus160 October 18, 2024 19:27
@shaka-bot shaka-bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Oct 18, 2024
@avelad avelad added the component: CMCD The issue involves CMCD specifically label Oct 21, 2024
@JoaquinBCh JoaquinBCh requested a review from avelad October 21, 2024 18:12
@littlespex
Copy link
Contributor

I would like to get an answer to this question before approving/rejecting this PR:

cta-wave/common-media-client-data#138 (comment)
cta-wave/common-media-client-data#146

@JoaquinBCh
Copy link
Contributor Author

Hi @littlespex,
I was following the discussion about the structure you suggested to use, and based on my understanding for this PR where we only implemented ltc and msd we should place the version field outside of reporting and requestMode, and add ltc and msd to the enabledKeys list.
An example configuration could be:

cmcd: {
    enabled: true,
    version: 2,
    enabledKeys: ['sid', 'cid', 'v', 'msd', 'ltc'],
},

If this looks good to you, I can proceed with the changes.

@littlespex
Copy link
Contributor

The only things that needs to change is adding the version property to the top level cmcd config. enabledKeys is a dash.js key, in shaka it's includeKeys. That said, you shouldn't need to do anything there. If the player is configured for v2, then those keys will be included by default.

@avelad avelad modified the milestones: v4.12, v4.13 Nov 13, 2024
Copy link
Member

@avelad avelad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM just a few more minor changes.

Copy link
Member

@avelad avelad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I leave final approval and merge to @littlespex

@avelad avelad dismissed littlespex’s stale review November 29, 2024 11:31

Reviewed by Wojciech and Alvaro

@avelad avelad merged commit b2673fd into shaka-project:main Nov 29, 2024
18 of 19 checks passed
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Jan 28, 2025
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Jan 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: CMCD The issue involves CMCD specifically priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants