Add a PlayerStats base interface with Cmcd conversion support #51
Conversation
|
PlayerStats might need to be updated with our needs, it is expected to be extensible. |
MathieuPOUX
requested review from
MathieuPOUX and
thomasjammet
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR adds a new PlayerStats class to the web-utils library with a toCmcd method for converting player statistics to CMCD (Common Media Client Data) format. It also adds the @svta/common-media-library dependency and configures rollup to bundle external dependencies.
Changes:
- Adds
PlayerStatsclass with properties for tracking media player metrics and atoCmcdconversion method - Includes
@svta/common-media-libraryas a new production dependency and adds@rollup/plugin-node-resolveas a dev dependency - Migrates from simple-git-hooks to husky for git hooks management (undocumented change)
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| stats/PlayerStats.ts | New class with media player statistics properties and CMCD conversion method |
| index.ts | Exports new PlayerStats class and re-exports CMCD types from @svta/common-media-library |
| package.json | Adds @svta/common-media-library dependency, @rollup/plugin-node-resolve, removes simple-git-hooks, updates prepare script to use husky |
| rollup.config.js | Adds nodeResolve plugin to bundle external dependencies |
| package-lock.json | Lock file updates reflecting dependency changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stats/PlayerStats.ts
Outdated
| toCmcd(trackId: number, prevStats?: PlayerStats): Cmcd { | ||
| const sfByProtocol = { | ||
| dash: 'd', | ||
| hls: 'h', | ||
| smooth: 's' | ||
| }; | ||
|
|
||
| const sf = this.protocol?.toLowerCase() | ||
| ? sfByProtocol[this.protocol?.toLowerCase() as keyof typeof sfByProtocol] | ||
| : undefined; | ||
|
|
||
| const cmcd: Cmcd = { | ||
| bl: this.bufferAmount, | ||
| bs: (this.stallCount ?? 0) - (prevStats?.stallCount ?? 0) > 0, | ||
| br: (this.audioTrackBandwidth ?? 0) + (this.videoTrackBandwidth ?? 0), | ||
| mtp: this.recvByteRate, | ||
| pr: this.playbackRate ?? this.playbackSpeed, | ||
| sf: sf as CmcdStreamingFormat | undefined, | ||
| su: this.waitingData | ||
| }; | ||
| if (trackId === this.videoTrackId) { | ||
| cmcd.br = this.videoTrackBandwidth ?? 0; | ||
| } else if (trackId === this.audioTrackId) { | ||
| cmcd.br = this.audioTrackBandwidth ?? 0; | ||
| } | ||
| return cmcd; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new PlayerStats class and its toCmcd method lack test coverage. This repository has comprehensive test files for other modules (e.g., Connect.spec.ts, EventEmitter.spec.ts, NetAddress.spec.ts), but no corresponding test file exists for PlayerStats. Consider adding stats/PlayerStats.spec.ts to test the toCmcd method logic, especially edge cases like undefined protocol values, missing track IDs, and the stall count calculation.
MathieuPOUX
changed the title
…d @rollup/plugin-node-resolve
| pr: this.playbackRate ?? this.playbackSpeed, | ||
| sf: sf as CML.CmcdStreamingFormat | undefined, | ||
| su: this.waitingData | ||
| }; |
There was a problem hiding this comment.
is missing it =>
cmcd.cid = url.pathname.split('/').pop();
cmcd.dl = this._playing.bufferAmount * this._playing.playbackRate;
cmcd.ot =
type === Media.Type.AUDIO
? CmcdObjectType.AUDIO
: type === Media.Type.VIDEO
? CmcdObjectType.VIDEO
: CmcdObjectType.OTHER;
cmcd.st = CmcdStreamType.LIVE;
cmcd.v = 1;
and I get a doubt also for cmcd.sid, maybe need to demystify it with IA
3 participants
https://linear.app/ceeblue/issue/ENG-1187
🚀 Features
⚙️ Miscellaneous Tasks