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

fix(seeking) #4956 - issue with seeking back when video is paused #5093

Closed
wants to merge 1 commit into from
Closed

fix(seeking) #4956 - issue with seeking back when video is paused #5093

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/controller/gap-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export default class GapController {
this.seeking = seeking;

// The playhead is moving, no-op
if (currentTime !== lastCurrentTime) {
if (currentTime !== lastCurrentTime || (media?.paused && seeking)) {
this.moved = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than circumvent all stall tracking and gap jumping when paused and seeking, we can avoid _trySkipBufferHole by not setting this.moved to false on line 114 when paused. I included that change in this branch which does the same as long as maxBufferHole configured to 0. The later nudge behavior ("Trying to nudge playhead over buffer-hole") is still expected when the playhead is at a point where the combined buffer is empty and media is not loaded.

https://github.com/video-dev/hls.js/compare/bugfix/nudging-over-buffer-hole-when-paused

Copy link
Author

Choose a reason for hiding this comment

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

Hey @robwalch thank you for the fast reply and for covering other cases, I wasn't aware of them.

I tested out your branch.

You mention in #2327 (comment)

As long as maxBufferHole and maxFragLookUpTolerance are set to 0

  1. maxBufferHole 0 maxFragLookUpTolerance 0 -> not working hitting the wall ❌
  2. maxBufferHole 0 -> works - it recovers and passes the wall ✅
  3. default config -> works - it recovers and passes the wall ( a bit slower than (2)) ✅

What is your opinion about [1]? Are there some consequences of setting maxBufferHole 0 (what we can expect)?

Copy link
Collaborator

@robwalch robwalch Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

maxBufferHole 0 maxFragLookUpTolerance 0 -> not working hitting the wall ❌

Can you provide more detail - maybe a close-up screenshot of the fragment and buffer source ranges in this state?

I suggested a maxFragLookUpTolerance of 0 because with higher values, the findFragment... methods favor selecting the next fragment when within tolerance. The branch introduces some changes to recognize when the first fragment selected is loaded and the previous one is needed, but I'm not exactly sure where you are hitting the wall.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @robwalch can you maybe guide what me exact variables should i log?

For example
this.fragmentTracker.getBufferedFrag

    "_byteRange": null,
    "relurl": "main_1080p6000kbps3954.ts",
    "elementaryStreams": {
        "audio": {
            "startPTS": 7910.3803333333335,
            "endPTS": 7912.343,
            "startDTS": 7910.3803333333335,
            "endDTS": 7912.343,
            "partial": false
        },
        "video": {
            "startPTS": 7910.1793333333335,
            "endPTS": 7912.1793333333335,
            "startDTS": 7910.1793333333335,
            "endDTS": 7912.1793333333335,
            "partial": false
        },
        "audiovideo": null
    },
    "_decryptdata": null,
    "rawProgramDateTime": null,
    "programDateTime": null,
    "tagList": [
        [
            "INF",
            "2.000000"
        ]
    ],
    "duration": 2,
    "sn": 3954,
    "type": "main",
    "loader": null,
    "keyLoader": null,
    "level": 2,
    "cc": 1,
    "startPTS": 7910.1793333333335,
    "endPTS": 7912.343,
    "appendedPTS": 7912.1793333333335,
    "startDTS": 7910.1793333333335,
    "endDTS": 7912.343,
    "start": 7910.1793333333335,
    "deltaPTS": 0.20100000000002183,
    "maxStartPTS": 7910.1793333333335,
    "minEndPTS": 7912.1793333333335,
    "stats": {
        "aborted": false,
        "loaded": 1674516,
        "retry": 0,
        "total": 1674516,
        "chunkCount": 1,
        "bwEstimate": 40905204.688538656,
        "loading": {
            "start": 41696.09999999963,
            "first": 41716.5,
            "end": 41730.5
        },
        "parsing": {
            "start": 41730.5,
            "end": 41797.20000000112
        },
        "buffering": {
            "start": 41788.09999999963,
            "first": 44300.70000000112,
            "end": 50793.800000000745
        }
    },
    "urlId": 0,
    "bitrateTest": false,
    "title": null,
    "initSegment": null

bufferedFragAtPos

{
    "_byteRange": null,

    "relurl": "main_1080p6000kbps3803.ts",
    "elementaryStreams": {
        "audio": {
            "startPTS": 7608.3803333333335,
            "endPTS": 7610.343,
            "startDTS": 7608.3803333333335,
            "endDTS": 7610.343,
            "partial": false
        },
        "video": {
            "startPTS": 7608.1793333333335,
            "endPTS": 7610.1793333333335,
            "startDTS": 7608.1793333333335,
            "endDTS": 7610.1793333333335,
            "partial": false
        },
        "audiovideo": null
    },
    "_decryptdata": null,
    "rawProgramDateTime": null,
    "programDateTime": null,
    "tagList": [
        [
            "INF",
            "2.000000"
        ]
    ],
    "duration": 2,
    "sn": 3803,
    "type": "main",
    "loader": null,
    "keyLoader": null,
    "level": 2,
    "cc": 1,
    "startPTS": 7608.1793333333335,
    "endPTS": 7610.343,
    "appendedPTS": 7610.1793333333335,
    "startDTS": 7608.1793333333335,
    "endDTS": 7610.343,
    "start": 7608.1793333333335,
    "deltaPTS": 0.20100000000002183,
    "maxStartPTS": 7608.1793333333335,
    "minEndPTS": 7610.1793333333335,
    "stats": {
        "aborted": false,
        "loaded": 1471288,
        "retry": 0,
        "total": 1471288,
        "chunkCount": 1,
        "bwEstimate": 39393857.00656811,
        "loading": {
            "start": 100967,
            "first": 100976.90000000037,
            "end": 100993
        },
        "parsing": {
            "start": 100993,
            "end": 101047.5
        },
        "buffering": {
            "start": 101043.29999999888,
            "first": 101059.19999999925,
            "end": 101064.29999999888
        }
    },
    "urlId": 0,
    "bitrateTest": false,
    "title": null,
    "initSegment": null
}

Screenshot from 2022-12-12 11-47-08

Copy link
Author

Choose a reason for hiding this comment

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

friendly reminder ping @robwalch

Copy link
Collaborator

@robwalch robwalch Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @salesh,

There are some gap-controller changes coming in #5257. I'll see if I can incorporate changes from bugfix/nudging-over-buffer-hole-when-paused or this PR after that's been merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@salesh you can test the upcoming changes at https://feature-gap-tag-handling.hls-js-4zn.pages.dev/demo/

As long as these settings are 0, then the player should load segments at currentTime without jumping forward. So, if you want frame by frame seeking, these settings should be used:

  "maxBufferHole": 0,
  "maxFragLookUpTolerance": 0

Let me know if this works for you and what you find off the gap handing branch. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

[1] tested on https://feature-gap-tag-handling.hls-js-4zn.pages.dev/demo/ with

  "maxBufferHole": 0,
  "maxFragLookUpTolerance": 0

✔️
[2] created hls.min.js from #5257 use

  "maxBufferHole": 0,
  "maxFragLookUpTolerance": 0

and check it in our app
✔️

Everything works @robwalch thank you very much for your efforts!

Copy link
Collaborator

@robwalch robwalch Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@salesh, would you be OK closing this PR?

I prefer not to merge as it would change the default behavior of the player to snap to the next segment when seeking even when paused. If the solution above is not enough, we'll need to either:

  1. Add checks for maxBufferHole === 0 && maxFragLookUpTolerance === 0 to your change
  2. Keep track of lastCurrentTime !== currentTime direction, and prevent forward gap jump seeks when the last difference between currentTime was negative

Copy link
Author

Choose a reason for hiding this comment

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

For sure @robwalch I agree with you! Thank you!

if (stalled !== null) {
// The playhead is now moving, but was previously stalled
Expand Down