Buffer Controller TypeScript #2073
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I updated the tooling that was doing the linting on the earlier build that caused Interface definitions to come up as However, nothing seems broken so in the hope of keeping the change as small as possible, I won't upgrade the TypeScript version. The build is still failing because of the issue in #2075. Once that is merged and on master, if we re-run Travis build here it should pass. |
|
Link #2070 |
tjenkinson
reviewed
Jan 10, 2019
|
@johnBartos Ah yes, I see the message was printed already. Thanks! |
|
Blocked by #2075. |
tjenkinson
reviewed
Jan 11, 2019
tjenkinson
reviewed
Jan 11, 2019
tjenkinson
reviewed
Jan 12, 2019
1 task
|
Excellent work! 👍 thanks for taking point on the reviews @tjenkinson |
itsjamie
commented
Jan 13, 2019
|
So one aspect that I just fixed with my PR here was that because of usage of things like |
itsjamie
commented
Jan 15, 2019
itsjamie
commented
Jan 16, 2019
itsjamie
commented
Jan 16, 2019
itsjamie
commented
Jan 16, 2019
tjenkinson
reviewed
Jan 16, 2019
itsjamie
commented
Jan 18, 2019
tjenkinson
reviewed
Jan 18, 2019
itsjamie
commented
Jan 19, 2019
|
I just noticed there was a change where a conditional was reversed in I'm waiting for the CI functional tests to run, and if they pass I'm thinking it would be good to merge in #2054 soon-ish, since the unit tests passed with the logic reversed for This also would be why the MPEG Audio functional test was failing. The media element duration wasn't updating. |
michaelcunningham19
previously approved these changes
Jan 21, 2019
src/controller/buffer-controller.ts
Outdated
| event.fatal = true; | ||
| hls.trigger(Events.ERROR, event); | ||
| } else { | ||
| hls.trigger(Events.ERROR, event); |
There was a problem hiding this comment.
Couldn't we remove the 3 occurrences of hls.trigger(Events.ERROR, event); with one at the end the catch block?
There was a problem hiding this comment.
I think so. I'll take a look.
itsjamie
dismissed stale reviews from michaelcunningham19 and tjenkinson
via
0503ea7
Thank you to Tom Jenkinson for acting as a reviewer for this work, including writing some suggestions via the GitHub PR github.com//pull/2073. The commit history has been squashed as it was a fairly lengthly process to get everything working to support the conversion to TypeScript. Early in the history of this commit, we converted properties to be initialized to `null`. This was incorrect, as it changes the behaviour of code that uses `Object.keys` calls to check if an object has any keys on it. Before, they would be empty, but if the properties are set to null, this check is no longer valid and it changes the behaviour. This is something to look out for in the future of other conversions, as the values themselves are often times reset to the `null`, but begin as undefined. Added a common `types` folder for interfaces where the ownership of the type wasn't clear for the individual file. The default no-op logger which is the inferred usage did not have any arguments in it's method signature. By adding an argument, TypeScript is able to infer the correct interface. As well the Karma and Webpack configuration had to be updated to enable building TypeScript, and getting testing coverage information from Istanbul. Define lib "es2015" and "dom" to add HTMLMediaElement, SourceBuffer and Number.isFinite definitions to the TypeScript binary when it is doing type-checking. When attempting to lint the older version of `typescript-eslint-parser` would emit `no-undef` errors for TypeScript interfaces. Updating the package fixes this issue. Tom pointed out that we should not use global module declarations to extend default HTML types since users will eventually be importing *.d.ts files from the hls.js project and this would also extend their types. Due to this feedback switched to defining a module local type `ExtendedSourceBuffer`. Which instead extends with the `ended` prop. Event Handlers were set to be part of the private scope. While this has no effect now, it will be important to ensure other modules do not directly invoke the event handlers. Other methods that were prefixed with a `_` were also set into the private scope. Where possible, code was switched to early-exit to lower the cognitive load in conditional statements, an example of this was the doAppend method. Another cleanup statement was switching handling of error codes and the comments to be closer to the conditional to which they applied. Switched to using the Record<Key, Value> type to help support access via the index signature [key: T] syntax in the future. An issue that was fixed in this commit was that The controller previous exposed the local `pendingTracks` property through the event and did not set the buffer on `this.tracks` instead opting to set it on the individual track, which bubbled up to the pendingTracks object, whose reference was lost in the buffer-controller module. This commit changes this behaviour so that the controller publishes `tracks` from the public variable on the controller rather than the pendingTracks which are passed into createSourceBuffers. This also means that this fixes the `.buffer.buffered` being undefined on the demo page. Added unit tests to cover the new behaviour of throwing an error if createSourceBuffer is called without an attached media element.
|
@michaelcunningham19 @tjenkinson Sorry for dismissing your reviews, I fixed the last comment from Michael, and took that as an opportunity to also squashed the long history down into a single commit. |
|
@tjenkinson or @michaelcunningham19 can we merge this if everything is good? Thank yas. |
|
🎉 |
johnBartos
added a commit
to jwplayer/hls.js
that referenced
this pull request
Feb 22, 2019
* isCodecSupportedInMp4 test audio codecs as audio Fixes video-dev#1824. * Do not show `undefined` * package: rm npm-run-all as it contains "event-stream" (and maybe other dormant exploits?). see "flatmap-stream" * rewrite package-lock: removed event-stream and all deps got not fixed versions instead of a hat * update lock-file generated with npm v6.4.1 * package deps: npm remove --save mversion jshint * Added OpenPlayerJS to the list of players that support HLS.js (video-dev#2023) * NIT: fix typos in readme 1. Fix spelling of interdependent and environment 2. Correct the agreement in the phrase: 'imported/required by each other.' * Revert video-dev#2004 (video-dev#2027) * Fix 0.11.1 Regressions (video-dev#2028) * Create sourceBuffers if the max of 2 tracks has been reached, regardless of buffer codec events received * Replace Object.values with [].slice.call * Remux according to initial PTS so that streams start at 0 (video-dev#2030) * Buffer streams starting at 0 initPTS * Use PTS to determine if segments are contiguous * Remove code setting startPosition to buffer start * Remove seek to buffered start logic * Simplify EOS check by using frag states, move logic into shared superclass (video-dev#2040) * Simplify EOS check by using frag states, move logic into shared superclass * Move shared seek logic into base, clear fragCurrent on seek out of buffered range * Revert PTS wrapping code which produces incorrect cue times (video-dev#2044) * deploy demo/docs to netlify, not gh-pages * update demo links in issue template * format commit as code because it is slightly more readable * remove accidental "/" in readme * update note to access specific version api docs * include tag in idShort * escape ` * Check if PTS has wrapped after applying PTS time offset (video-dev#2056) * Check if PTS has wrapped after applying PTS time offset * Fix bug accessing buffered track array * Intialize vttCCs map with cc 0 (video-dev#2058) * Intiialize vttCCs map with cc 0 * use the version from package.json when deploying and include this in the deployments readme * fix: abrFactor greater than in docs * Changes clone function used in webpack config to be the webpack recommended merging library. * prod: add bandwidthEstimate method * docs: add bandwidthEstimate * test: add autotest * Convert buffer-controller.js to TypeScript Thank you to Tom Jenkinson for acting as a reviewer for this work, including writing some suggestions via the GitHub PR github.com/video-dev/pull/2073. The commit history has been squashed as it was a fairly lengthly process to get everything working to support the conversion to TypeScript. Early in the history of this commit, we converted properties to be initialized to `null`. This was incorrect, as it changes the behaviour of code that uses `Object.keys` calls to check if an object has any keys on it. Before, they would be empty, but if the properties are set to null, this check is no longer valid and it changes the behaviour. This is something to look out for in the future of other conversions, as the values themselves are often times reset to the `null`, but begin as undefined. Added a common `types` folder for interfaces where the ownership of the type wasn't clear for the individual file. The default no-op logger which is the inferred usage did not have any arguments in it's method signature. By adding an argument, TypeScript is able to infer the correct interface. As well the Karma and Webpack configuration had to be updated to enable building TypeScript, and getting testing coverage information from Istanbul. Define lib "es2015" and "dom" to add HTMLMediaElement, SourceBuffer and Number.isFinite definitions to the TypeScript binary when it is doing type-checking. When attempting to lint the older version of `typescript-eslint-parser` would emit `no-undef` errors for TypeScript interfaces. Updating the package fixes this issue. Tom pointed out that we should not use global module declarations to extend default HTML types since users will eventually be importing *.d.ts files from the hls.js project and this would also extend their types. Due to this feedback switched to defining a module local type `ExtendedSourceBuffer`. Which instead extends with the `ended` prop. Event Handlers were set to be part of the private scope. While this has no effect now, it will be important to ensure other modules do not directly invoke the event handlers. Other methods that were prefixed with a `_` were also set into the private scope. Where possible, code was switched to early-exit to lower the cognitive load in conditional statements, an example of this was the doAppend method. Another cleanup statement was switching handling of error codes and the comments to be closer to the conditional to which they applied. Switched to using the Record<Key, Value> type to help support access via the index signature [key: T] syntax in the future. An issue that was fixed in this commit was that The controller previous exposed the local `pendingTracks` property through the event and did not set the buffer on `this.tracks` instead opting to set it on the individual track, which bubbled up to the pendingTracks object, whose reference was lost in the buffer-controller module. This commit changes this behaviour so that the controller publishes `tracks` from the public variable on the controller rather than the pendingTracks which are passed into createSourceBuffers. This also means that this fixes the `.buffer.buffered` being undefined on the demo page. Added unit tests to cover the new behaviour of throwing an error if createSourceBuffer is called without an attached media element. * test: review changes * docs: review fix * prod: add review fix * docs: update Co-Authored-By: Beraliv <beraliv.spb@gmail.com> * docs: update 2 * docs: fix link * test: equal => strictEqual * test: add mocked _bwEstimator * test: change description * test: (() => { ... }) => function () { ... } * Increase BufferHelper test coverage to 100% (video-dev#2052) * Increase adts.js test coverage to 100% (video-dev#2053) * Increase adts.js test coverage to 100% * Convert eme-controller to TypeScript. Added TypeScript types to the code. Handled media detached and removing the encrypted event listener. * Listen to media detached Clears the encrypted event listener from the HTMLMediaElement in controller, and releases the stored reference. * Choose bitrate in Bps (video-dev#2091) Simplify bandwidth calculation by making; change docs to correctly reference bandwidth in bits/s * Upgrade tests to Chai BDD & optimize runner (video-dev#2037) * Convert tests to Chai BDD * Invoke mocha with exit so that travis doesnt hang on func test completion * Fix video-dev#2061 * configure netlify for PR's this follows the same process that runs for canary releases * check if repo is shallow before fetching because on netlify it clones the whole thing * handle 'netlifyPr' in set-package-version * tabs => spaces * add commit to version when PR * always trim command output * Update Fragment and LevelKey to typescript. * Update usage of Fragment and LevelKey. * Don't pass prevFrag into EXT-X-MAP handling. * Add tests for parsing byte range as seperate call. * Only parse byte range if it exists. * Apply suggestions from code review Co-Authored-By: itsjamie <1956521+itsjamie@users.noreply.github.com> * Review changes. parseByteRange -> setByteRange - removed it returning the parsed value since its usage was like a setter. * Review comments pt.2 * Change tests to call setByteRange * Explicit handling of 'initSegment' conversion for bitwise operation. * Fix lint warnings in m3u8-parser.js in prep for TS conversion. (video-dev#2106) * fix console null reference in logger (in webworkers) (video-dev#2095) Fix null reference in logger when console is not available * (misc) Adding ace editor to demo to enable advanced customization (video-dev#2077) * Add handling for AES-128 encrypted initialization segments needing IV. * Change IV handling for initSegments to just log a warning. * Update webpack to use babel loader with support for TS. (video-dev#2119) * Upgrade to webpack 4, babel 7 * Use straight git link for webworkify dep * Use simpler git url * Re-add globalObject workaround * Bump karma dep versions * Dont preprocess non-test files * Use @babel/register over ts-node * Update demo `const` usage. * Use webpack debug config in Karma configuration rather than duplication. * Delete duplicate declaration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will...
This adds TypeScript definitions to the buffer controller to help migrate the internals of hls.js over to TypeScript.
Are there any points in the code the reviewer needs to double check?
I wasn't sure what controller or file should "own" the types for Segment / Track types so I created a types folder and added them there for the purpose of moving forward. If a better location or file for ownership is known can move it there.
One additional change I made was I changed the functions that were bound as event listeners to become instance methods using the
name = () => { }definition style, rather than bind the prototype function to the instance and keep track of the reference. This is why variables such asthis.onmsewere removed and not defined.Checklist