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

Issue 4197: Typescript typings #4200

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

Conversation

Darkwinde
Copy link

Pull request targets issue 4197: #4197

@dsilhavy
Copy link
Collaborator

@Darkwinde Thanks for your PR. We need a signed CLA to merge these changes. I think Stefan and Görkem already discussed this with you?

Following our contribution guidelines, and since this seems to be your first PR, could you please send me a signed copy of dash.js feedback agreement?

@@ -1330,7 +1408,7 @@ declare namespace dashjs {

setTextTrack(idx: number): void;

getBitrateInfoListFor(type: MediaType): BitrateInfo[];
getBitrateInfoListFor(type: MediaType): QualityInfo[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this type changed?

Copy link
Author

Choose a reason for hiding this comment

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

In general I am not sure that the definition of "BitrateInfo" is correct...you defined it as a class but use it as an array object.
I changed it for "getBitrateInfoListFor" as I have seen during my testing it has more properties compared to "BitrateInfo" and they are the same like "QualityInfo".

@@ -1316,7 +1394,7 @@ declare namespace dashjs {

getDashMetrics(): DashMetrics;

getQualityFor(type: MediaType): number;
getQualityFor(type: MediaType): QualityInfo;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This only returns a number

Copy link
Author

Choose a reason for hiding this comment

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

Nope, not what I see / get it is an quality object back...but this is fine and even better, as there are all information that I need. Index requires another call / request to get the correct quality information :) Maybe not intended, but a good from my point of view.

console.log('getQualityFor("video")', this.dashjsplayerInstance.playerInstance.getQualityFor("video"));

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this based on the AdaptationSet Switching branch? Because for development it should be a number

Copy link
Author

Choose a reason for hiding this comment

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

Yes we currently use the ASS branch.
I checked with the public 4.7.1 NPM version and this is a number.
Good to know.

So this is something to be changed then in a future release? 4.7.2, 5.0?
Need to know then it is planned to be merged, as other partner also rely on this method to get the Bitrate and Rendition and currently cannot obtain the correct information.

index: number;
duration: number;
start: number;
mpd: Mpd;
nextPeriodId: string | null;
}

export interface IAdaption {
ContentProtection: IContentProtection[];
ContentProtection_asArray: IContentProtection[] // Why? ContentProtection is already type array
Copy link
Collaborator

Choose a reason for hiding this comment

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

Depends on how many entires there are. If there is only one entry ContentProtection is an object not an array. This is one of the reasons we are replacing the XML parser in v.5

Copy link
Author

Choose a reason for hiding this comment

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

ok understood. Was just wondering the duplication I have always seen :)

@dsilhavy dsilhavy modified the milestones: 4.7.1, 4.7.2 Jun 19, 2023
@Darkwinde
Copy link
Author

@Darkwinde Thanks for your PR. We need a signed CLA to merge these changes. I think Stefan and Görkem already discussed this with you?

Following our contribution guidelines, and since this seems to be your first PR, could you please send me a signed copy of dash.js feedback agreement?

Yes, will clarify internally this point.

@dsilhavy dsilhavy modified the milestones: 4.7.2, 5.0.0 Aug 17, 2023
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.

2 participants