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

fix(FEC-7935): fix text selection logic with hlsjs #65

Merged
merged 6 commits into from
Jul 12, 2018
Merged

Conversation

OrenMe
Copy link
Contributor

@OrenMe OrenMe commented Jul 10, 2018

Description of the Changes

this is basically the same as before, but hls.js run different code paths when we use it's API for selecting text track.
Before this change we used to set the showing/hidden mode attribute of the text track in playkit-js html5 engine. we now set this in HLS.JS which internally does the same but also it manages internal state for this and so it's better to use its API and not access the textTrack.mode directly.
This fix depends on a fix in kaltura/playkit-js#263

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

this is basically the same as before, but hls.js run different code paths when we use it's API for selecting text track.
@OrenMe OrenMe requested a review from dan-ziv July 11, 2018 20:24
}
});

it('should hide the active text track', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

why to remove this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously we managed the tracks directly ourselves - so it made sense.
Now we use hlsjs API and it requires loading an actual manifest with text tracks - and we will basically be testing internals of hlsjs - which is out of scope for unit testing

Copy link
Contributor

@yairans yairans Jul 12, 2018

Choose a reason for hiding this comment

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

we just checking that our hideTextTrack does its work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand but the test doesn't reflect it - in our unit testing we inject the text tracks ourselves and not via hlsjs so we actually don't test correctly.
And now that we moved to using hlsjs API itself then the test is both irrelevant(as the testing is black boxed behind hls.js API) and not working as we are not using hlsjs to add and manage the text tracks in the test itself.

@@ -149,6 +149,9 @@ export default class HlsAdapter extends BaseMediaSourceAdapter {
adapterConfig.hlsConfig.startPosition = config.playback.startTime;
}
}
if (Utils.Object.hasPropertyPath(config, 'playback.useNativeTextTrack')) {
adapterConfig.subtitleDisplay = Utils.Object.getPropertyPath(config, 'playback.useNativeTextTrack');
Copy link
Contributor

@yairans yairans Jul 12, 2018

Choose a reason for hiding this comment

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

add it on adapterConfig.hlsConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not an hls.js config option - it's an API. I wanted to add it as config first but it doesn't get set on hls.js config...

@@ -202,6 +205,7 @@ export default class HlsAdapter extends BaseMediaSourceAdapter {
this._config.hlsConfig['pLoader'] = pLoader;
}
this._hls = new Hlsjs(this._config.hlsConfig);
this._hls.subtitleDisplay = this._config.subtitleDisplay;
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary. see the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my comment above

OrenMe added a commit to kaltura/playkit-js that referenced this pull request Jul 12, 2018
due to the way hls.js manages track selection, especialy when default text track is set in manifest, we had some incorrect logic.
This simplfies and fixes the various paths we had to handle this.
The fix is dependent on a fix on video-dev/hls.js#1582 and kaltura/playkit-js-hls#65.
@OrenMe OrenMe merged commit 3074d19 into master Jul 12, 2018
@OrenMe OrenMe deleted the FEC-7935 branch July 12, 2018 09:15
OrenMe added a commit to kaltura/playkit-js-dash that referenced this pull request Jul 12, 2018
use shaka setTextTrackVisibility to set text track display mode according to useNativeTextTrack config flag.
This change is part of kaltura/playkit-js-hls#65 and kaltura/playkit-js#263 - removing the overhead of handling in playkit-js is possible by using the Shaka APIs to set track visibility.
dan-ziv pushed a commit that referenced this pull request Jul 16, 2020
borhandarabi pushed a commit to TasvirChi/playchi-js-dash that referenced this pull request May 14, 2024
use shaka setTextTrackVisibility to set text track display mode according to useNativeTextTrack config flag.
This change is part of kaltura/playkit-js-hls#65 and kaltura/playkit-js#263 - removing the overhead of handling in playkit-js is possible by using the Shaka APIs to set track visibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants