-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: main
Are you sure you want to change the base?
Conversation
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. |
Incremental code coverage: 95.77% |
There was a problem hiding this 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.
externs/shaka/player.js
Outdated
/** | ||
* @typedef {{ | ||
* version: number | ||
* }} | ||
* @property {number} version | ||
*/ | ||
shaka.extern.RequestMode; | ||
|
||
/** | ||
* @typedef {{ | ||
* requestMode: shaka.extern.RequestMode | ||
* }} | ||
*/ | ||
shaka.extern.Reporting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add description for those typedefs and their properties.
lib/player.js
Outdated
*/ | ||
getLiveLatency() { | ||
const element = this.video_; | ||
const now = this.getPresentationStartTimeAsDate().valueOf() + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's technically the same but a bit easier to read
const now = this.getPresentationStartTimeAsDate().valueOf() + | |
const now = this.getPresentationStartTimeAsDate().getTime() + |
* | ||
* @return {number} | ||
*/ | ||
getLiveLatency() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit test for this method.
lib/player.js
Outdated
* @return {number} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return null if nothing is playing.
* @return {number} | ||
*/ | ||
getLiveLatency() { | ||
const element = this.video_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check does video element exists.
lib/util/cmcd_manager.js
Outdated
this.playbackPlayTime_ = performance.now(); | ||
} | ||
} | ||
|
||
/** | ||
* Apply CMCD data to a manifest request. | ||
*/ | ||
onPlaybackPlaying() { | ||
if (!this.playbackPlayingTime_) { | ||
this.playbackPlayingTime_ = performance.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
performance.now()
is not available on all supported platforms. Please either switch to Date.now()
or add some utility method which checks which one is supported.
There was a problem hiding this 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!
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. |
My initial thoughts:
|
Thanks everyone for the reviews. |
I would like to get an answer to this question before approving/rejecting this PR: cta-wave/common-media-client-data#138 (comment) |
This PR introduces two new CMCD v2 keys to shaka player:
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:
cmcd.reporting.requestMode.version
must be set to 2 to use these new CMCD v2 keys.getLiveLatency()
method in player to calculate the latency and use it for ltc.cmcd.reporting.requestMode.version
is set to 2.Testing:
How to Test manually:
Test that msd and ltc values are sent correctly
Set the following configuration to the player:
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.
The msd value should be higher than usual.
Notes: