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.

Sign up for GitHub

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

Jump to bottom

Bugfix for subtitle loading after hls.stopload() was used #3063

Merged
merged 16 commits into from Oct 8, 2020
Merged

Bugfix for subtitle loading after hls.stopload() was used #3063

Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cbe8f87
Reset subtitle-stream-controller state to IDLE on startLoad()
netTrekfd Sep 26, 2020
f2aadf7
Cleanup any existing intervals and call tick() immediately
netTrekfd Sep 26, 2020
ad80f18
Only restart interval if we have a current track
netTrekfd Sep 26, 2020
2736608
Fix stop loading wrong segment order after initial seek
netTrekfd Sep 28, 2020
cbd0b77
Reset subtitle-stream-controller state to IDLE on startLoad()
netTrekfd Sep 26, 2020
dc3ebff
Cleanup any existing intervals and call tick() immediately
netTrekfd Sep 26, 2020
bc1512d
Only restart interval if we have a current track
netTrekfd Sep 26, 2020
30b76ea
Fix stop loading wrong segment order after initial seek
netTrekfd Sep 28, 2020
b29432d
Merge branch 'bugfix/subtitle-loading-after-stopload' of https://gith…
netTrekfd Sep 29, 2020
991de69
Refactored fix for segment loading order after seeking
netTrekfd Sep 29, 2020
d63cf0c
Update src/controller/subtitle-stream-controller.js
netTrekfd Sep 30, 2020
d107216
Cancel existing loader on errors
netTrekfd Oct 6, 2020
be2e12b
Added some checks if the seeking position is inside the currently loa…
netTrekfd Oct 6, 2020
fe219db
Update src/controller/subtitle-stream-controller.js
netTrekfd Oct 8, 2020
d67865a
Removed comment
netTrekfd Oct 8, 2020
b93c938
Merge branch 'bugfix/subtitle-loading-after-stopload' of https://gith…
netTrekfd Oct 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/controller/subtitle-stream-controller.js
View file
Open in desktop
Expand Up @@ -40,6 +40,18 @@ export class SubtitleStreamController extends BaseStreamController {
this._onMediaSeeking = this.onMediaSeeking.bind(this);
}

startLoad () {
this.stopLoad();
this.state = State.IDLE;

// Check if we already have a track with necessary details to load fragments
const currentTrack = this.tracks[this.currentTrackId];
if (currentTrack && currentTrack.details) {
this.setInterval(TICK_INTERVAL);
this.tick();
}
}

onSubtitleFragProcessed (data) {
const { frag, success } = data;
this.fragPrevious = frag;
Expand Down Expand Up @@ -237,6 +249,7 @@ export class SubtitleStreamController extends BaseStreamController {

stopLoad () {
this.lastAVStart = 0;
this.fragPrevious = null;
super.stopLoad();
}

Expand All @@ -245,6 +258,18 @@ export class SubtitleStreamController extends BaseStreamController {
}

onMediaSeeking () {
let frag = this.fragCurrent;
if (frag) {
if (frag.loader) {
frag.loader.abort();
}
this.fragmentTracker.removeFragment(frag);
}

this.fragCurrent = null;
this.fragPrevious = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good. It makes sense and it is a step in the right direction. Here's the catch:

Aborting the request should only be done when seeking outside the range of fragCurrent. When seeking a bit within the range of this fragment, loading would just be forced to start over. The key to the logic in base-stream-controller's onMediaSeeking is that it deals with this issue for audio and video. The difference is base-stream-controller relies on BufferHelper.bufferInfo(media and here we need to use BufferHelper.bufferInfo(this._getBuffered().

Copy link
Collaborator

@robwalch robwalch Sep 29, 2020 •
edited

Choose a reason for hiding this comment

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

Suggested change
let frag = this.fragCurrent;
if (frag) {
if (frag.loader) {
frag.loader.abort();
}
this.fragmentTracker.removeFragment(frag);
}
this.fragCurrent = null;
this.fragPrevious = null;
if (this.state === State.FRAG_LOADING) {
const fragCurrent = this.fragCurrent;
const bufferInfo = BufferHelper.bufferedInfo(this._getBuffered(), media.currentTime, this.config.maxBufferHole);
const tolerance = this.config.maxFragLookUpTolerance;
const fragStartOffset = fragCurrent.start - tolerance;
const fragEndOffset = fragCurrent.start + fragCurrent.duration + tolerance;
// check if we seek position will be out of currently loaded frag range : if out cancel frag load, if in, don't do anything
if (currentTime < fragStartOffset || currentTime > fragEndOffset) {
if (fragCurrent.loader) {
fragCurrent.loader.abort();
}
this.fragCurrent = null;
this.fragPrevious = null;
this.fragmentTracker.removeFragment(fragCurrent);
// switch to IDLE state to load new fragment
this.state = State.IDLE;
}
}
}

It's quite a bit of duplicate code that could be addressed by changing _getBuffered to get mediaBuffer and using the base controller's onMediaSeeking logic, so maybe give that a try. It's certainly something I'd like to have in v1.

The suggested change above duplicates some code but that's OK. This will be addressed in v1.0.0 by exposing the buffered time ranged in the subtitle-stream-controller as a mediaBuffer property: https://github.com/video-dev/hls.js/pull/3060/files#diff-9f7fbd2b9b9e7d0824937236683db215R47-R295 which is dealt with the same for all stream controllers in base-stream-controllers onMediaSeeking handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, i will take a look

Copy link
Contributor Author

@netTrekfd netTrekfd Oct 6, 2020 •
edited

Choose a reason for hiding this comment

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

Hi @robwalch ,
i just added some changes here be2e12b.

Unfortunately the check state === stateState.FRAG_LOADING did not work for me. As soon as i check for that state i'm easily able to reproduce the issue by seeking slowly backwards. There is probably a loader running and needs to be cancelled even if the state does not reflect it.

I also did some tests with the buffering test BufferHelper.bufferInfo(this._getBuffered() but am not sure if i got your suggestion right. The root of this problem is that as soon as a loader is running, it continues to load in the order given by the last loaded segment. Because of that i think we need to cancel the fragCurrent.loader independend of the fact we are seeking to an already buffered position. The buffer will be respected inside the next tick.
As soon as i do not cancel the loader when seeking to an already buffered position, it will keep loading from the previous position which caused the problem.
I hope this is understandable. If not, i will try to explain it better ;)

Thanks
Florian


this.state = State.IDLE;
this.tick();
}
}