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-11136): Tizen 4 got stuck on the beginning of the media #140

Merged
merged 14 commits into from
Apr 27, 2021

Conversation

Yuvalke
Copy link
Contributor

@Yuvalke Yuvalke commented Apr 25, 2021

Description of the Changes

Issue: Tizen 4 has stalled on the beginning of the media/after first seek we want to make sure the media wouldn't stop.
Solution: make a small seeks until playback keeps playing.

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

…artTV

Issue: Tizen 4 has stalls on the beginning of the media/after first seek we want to make sure the media wouldn't stop.
Solution: make a small seeks until playback keeps playing.
@Yuvalke Yuvalke requested a review from a team April 25, 2021 08:30
@Yuvalke Yuvalke self-assigned this Apr 25, 2021
let lastCurrentTime = this._videoElement.currentTime;
this._stallInterval = setInterval(() => {
if (lastCurrentTime === this._videoElement.currentTime) {
this._videoElement.currentTime = parseFloat(this._videoElement.currentTime.toFixed(1)) + 0.1;
Copy link
Contributor

Choose a reason for hiding this comment

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

move 0.1 to const, maybe expose as config?

} else {
this._clearStallInterval();
}
}, 2 * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

move to const, maybe expose as config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't want to do it too robust cause I believe we need to handle it on Shaka, WDYT?

Copy link
Contributor Author

@Yuvalke Yuvalke Apr 25, 2021

Choose a reason for hiding this comment

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

To create 3 configs for hack we're adding for edge case that we won't need anymore after Shaka will handle it, it sounds too robust, don't you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, let's start lean, but please move to consts and we will hope Shaka will approve this :-)

Comment on lines 409 to 410
this._eventManager.listenOnce(EventType.STALLED, this._stallSmartTVHandler);
this._eventManager.listenOnce(EventType.SEEKED, this._stallSmartTVHandler);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need/want to remove them after one handler is called?
If stalled occurred we can remove seeked and vide versa?

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 clear the stall interval if it's not stopped for each one of them.
Yes, the issue that we saw occur only on the beginning.

_stallInterval: ?IntervalID = null;

/**
* stall interval to break the stall on Smart TV
Copy link
Contributor

Choose a reason for hiding this comment

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

fix desc

*/
_maybeBreakStalls(): void {
if (this._config.forceBreakStall) {
this._eventManager.listenOnce(this._videoElement, EventType.STALLED, () => this._stallSmartTVHandler());
Copy link
Contributor

Choose a reason for hiding this comment

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

so you want to keep it once only?

@Yuvalke Yuvalke merged commit 13b9fb6 into master Apr 27, 2021
@Yuvalke Yuvalke deleted the FEC-11136 branch April 27, 2021 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants