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: Add reporting rendered framerate #77

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

realeyes-craig
Copy link

Added:

  • Reporting framerate if available on targetQuality

Copy link
Contributor

@stonko1994 stonko1994 left a comment

Choose a reason for hiding this comment

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

  • Please add a Changelog entry
  • Please open the PR against develop
  • Optional: Please add unit tests 🙂

src/ts/ConvivaAnalytics.ts Show resolved Hide resolved
src/ts/ConvivaAnalytics.ts Show resolved Hide resolved
@realeyes-craig realeyes-craig changed the base branch from main to develop June 10, 2021 02:21
@dweinber
Copy link
Member

Just to be clear on rendered framerate - what the player provides is the framerate specified in the manifest for that video quality. Is this the intention of RENDERED_FRAMERATE or is this another field @realeyes-craig?

Copy link
Member

@dweinber dweinber left a comment

Choose a reason for hiding this comment

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

Please clarify if RENDERED_FRAMERATE is the right field for the data we provide.

Code change looks good and straightforward, please see the comments around the tests.

@@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]
### Added
- Tracking the current framerate
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Tracking the current framerate
- Tracking the framerate for the currently played video quality based on the framerate specified in the manifest file

@@ -241,7 +241,7 @@ interface EventEmitter {

fireAdErrorEvent(): void;

fireVideoPlaybackQualityChangedEvent(bitrate: number): void;
fireVideoPlaybackQualityChangedEvent(bitrate: number, frameRate: number): void;
Copy link
Member

Choose a reason for hiding this comment

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

Please make frameRate optional, so we can test events with and without frameRate being available.

@@ -422,7 +422,7 @@ class PlayerEventHelper implements EventEmitter {
});
}

fireVideoPlaybackQualityChangedEvent(bitrate: number): void {
fireVideoPlaybackQualityChangedEvent(bitrate: number, frameRate: number): void {
Copy link
Member

Choose a reason for hiding this comment

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

Please make frameRate optional, so we can test events with and without frameRate being available.

@@ -437,6 +437,7 @@ class PlayerEventHelper implements EventEmitter {
bitrate: bitrate,
width: null,
height: null,
frameRate: frameRate,
Copy link
Member

Choose a reason for hiding this comment

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

Please only expose frameRate if it's passed in the signature

it('report bitrate on event', () => {
playerMock.eventEmitter.firePlayEvent();
playerMock.eventEmitter.firePlayingEvent();
playerMock.eventEmitter.fireVideoPlaybackQualityChangedEvent(2_400_000);
playerMock.eventEmitter.fireVideoPlaybackQualityChangedEvent(2_400_000, 30);
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test case where we don't pass a frame rate and explicitly check that we don't set it (so we don't accidentally break it in the future)

@CLAassistant
Copy link

CLAassistant commented Jul 7, 2023

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants