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

[JW8-8804] Typescript level, audio and subtitle track controllers #207

Merged
merged 6 commits into from
Apr 30, 2019

Conversation

robwalch
Copy link

@robwalch robwalch commented Apr 24, 2019

This PR will...

  • Typescript:
    • audio-track-controller.ts
    • level-controller.ts
      • Default firstLevel to -1 so that it doesn't show up as NaN on the player api before loading a manifest.
    • subtitle-track-controller.ts
    • playlist-loader.ts
  • Add a Level class, used for all level instances in the level controller
    • Default realBitrate to 0 to provide strong typing. This simplifies max bitrate calculation in abr-controller.js
  • Add missing deltaPTS and maxStartPTS properties to Fragment
  • Type level parsing output
    • SingleLevel
    • LevelParsed
    • PlaylistMedia
    • LevelAttributes
  • Type event output:
    • ManifestLoadedData
    • ManifestParsedData
    • TrackLoadedData
    • LevelLoadedData
    • AudioTrackSwitchedData
    • FragLoadedData
    • MediaAttachedData
    • ErrorData
  • throw 'No Levels' error in level-controller when this._levels is null, rather than let an undefined exception occur later in the methods that are susceptible to this.
  • Clean up timeout typing window.setTimeout():number

Why is this Pull Request needed?

Typing these modules provides type checking for controllers that load manifests and create level and track arrays. We can see which properties are nullable and reason whether stronger typing is required in some cases.

Are there any points in the code the reviewer needs to double check?

Suggest alternatives to 'No Levels' exception. I don't think just returning when levels is null is a good solution. Existing behavior would be an exception - now it's explicit.

Are the types/events.ts and types/level.ts a good place to put some of these new interfaces or is there another convention for organizing interfaces and classes together or separately?

Resolves issues:

JW8-8804

@robwalch robwalch force-pushed the feature/typescript-level-and-track-controllers branch from 9061439 to b29f8d4 Compare April 24, 2019 18:40
@robwalch robwalch force-pushed the feature/typescript-level-and-track-controllers branch from b29f8d4 to ec9011b Compare April 24, 2019 19:21
@jwplayer-robot
Copy link

Copy link

@johnBartos johnBartos left a comment

Choose a reason for hiding this comment

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

LGTM overall, let's regroup on the level empty error stuff

src/controller/audio-track-controller.ts Outdated Show resolved Hide resolved
src/controller/audio-track-controller.ts Outdated Show resolved Hide resolved
newId = i;
break;
}
}

if (newId === previousId) {
logger.warn(`No fallback audio-track found for name/language: "${name}" / "${language}"`);
logger.warn(`No fallback audio-track found for name/language: "${name}" / "${lang}"`);

Choose a reason for hiding this comment

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

Can you add a log prefix to this (and other) statements? It can just go in the string instead of being in a separate method like the controllers. logger.log([class-name]: ...)

Copy link
Author

Choose a reason for hiding this comment

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

Everything extends EventHandler so why don't we put log and warn on EventHandler instead of BaseStreamController?

Copy link
Author

Choose a reason for hiding this comment

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

done

src/controller/level-controller.ts Outdated Show resolved Hide resolved
src/controller/level-controller.ts Show resolved Hide resolved
src/types/events.ts Show resolved Hide resolved
@@ -0,0 +1,83 @@
import LevelDetails from '../loader/level';

export interface SingleLevel {

Choose a reason for hiding this comment

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

Maybe better named as BaseLevel?

Copy link
Author

Choose a reason for hiding this comment

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

It's not a base class. It's the interface of the object emitted when only a single playlist is loaded (no master manifest).

Copy link
Author

Choose a reason for hiding this comment

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

Removed. I added attr: {} and bitrate: 0 to the [singleLevel] result from the m3u8 parser so that we don't have to worry about checking these types everywhere.

src/types/level.ts Show resolved Hide resolved
src/types/level.ts Show resolved Hide resolved
src/types/level.ts Show resolved Hide resolved
@jwplayer jwplayer deleted a comment from jwplayer-robot Apr 29, 2019
@jwplayer jwplayer deleted a comment from jwplayer-robot Apr 29, 2019
@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

JW8-8804
@jwplayer-robot
Copy link

@jwplayer-robot
Copy link

@jwplayer jwplayer deleted a comment from jwplayer-robot Apr 30, 2019
@johnBartos johnBartos merged commit 839f561 into LHLS Apr 30, 2019
@robwalch robwalch deleted the feature/typescript-level-and-track-controllers branch January 27, 2020 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants