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

Question about playlists alignment based on discontinuity counter #4416

Closed
hongfeih-es opened this issue Nov 8, 2021 · 4 comments · Fixed by #6542
Closed

Question about playlists alignment based on discontinuity counter #4416

hongfeih-es opened this issue Nov 8, 2021 · 4 comments · Fixed by #6542
Labels
Enhancement Verify Fixed An unreleased bug fix has been merged and should be verified before closing.
Milestone

Comments

@hongfeih-es
Copy link
Contributor

What do you want to do with Hls.js?

We noticed sometimes playback will be stalled when play ssai stream (with several discontinuities), and it normally happened when switching levels during ad breaks.

What have you tried so far?

Checked the code, findDiscontinuousReferenceFrag try to find first segment with same cc of of the first frag of the new level:

// Find the first frag in the previous level which matches the CC of the first frag of the new level
export function findDiscontinuousReferenceFrag(
prevDetails: LevelDetails,
curDetails: LevelDetails
) {
const prevFrags = prevDetails.fragments;
const curFrags = curDetails.fragments;
if (!curFrags.length || !prevFrags.length) {
logger.log('No fragments to align');
return;
}
const prevStartFrag = findFirstFragWithCC(prevFrags, curFrags[0].cc);
if (!prevStartFrag || (prevStartFrag && !prevStartFrag.startPTS)) {
logger.log('No frag in previous level to align on');
return;
}
return prevStartFrag;
}

But I think it's wrong.
For example,

it('finds the first fragment in an array which matches the CC of the first fragment in another array', function () {
    const mockReferenceFrags = [
      {
        start: 20,
        startPTS: 20,
        endPTS: 24,
        duration: 4,
        cc: 0,
      },
      {
        start: 24,
        startPTS: 24,
        endPTS: 28,
        duration: 4,
        cc: 0,
      },
      {
        start: 28,
        startPTS: 28,
        endPTS: 32,
        duration: 4,
        cc: 1,
      }
    ];
    const mockFrags = [
      {
        start: 0,
        startPTS: 0,
        endPTS: 4,
        duration: 4,
        cc: 0,
      },
      {
        start: 4,
        startPTS: 4,
        endPTS: 8,
        duration: 4,
        cc: 1,
      },
      {
        start: 8,
        startPTS: 8,
        endPTS: 16,
        duration: 8,
        cc: 1,
      },
    ];
    const prevDetails = {
      fragments: mockReferenceFrags,
    };
    const curDetails = {
      fragments: mockFrags,
    };
    const actual = findDiscontinuousReferenceFrag(prevDetails, curDetails);
    expect(actual.start).to.equal(24);
  });

findDiscontinuousReferenceFrag will find first segment of mockReferenceFrags, which is start: 20,, and new playlist will be adjusted to :

{
        start: 20,
        startPTS: 20,
        endPTS: 24,
        duration: 4,
        cc: 0,
      },
      {
        start: 24,
        startPTS: 24,
        endPTS: 28,
        duration: 4,
        cc: 1,
      },
      {
        start: 28,
        startPTS: 28,
        endPTS: 36,
        duration: 8,
        cc: 1,
      },

which is wrong, it should be:

{
        start: 24,
        startPTS: 24,
        endPTS: 28,
        duration: 4,
        cc: 0,
      },
      {
        start: 28,
        startPTS: 28,
        endPTS: 32,
        duration: 4,
        cc: 1,
      },
      {
        start: 32,
        startPTS: 32,
        endPTS: 40,
        duration: 8,
        cc: 1,
      },

I think start segment of next break (cc+1) should be used to align playlists between levels, like:

export function findDiscontinuousReferenceFrag2(
  prevDetails: LevelDetails,
  curDetails: LevelDetails
) {
  const prevFrags = prevDetails.fragments;
  const curFrags = curDetails.fragments;

  if (!curFrags.length || !prevFrags.length) {
    logger.log('No fragments to align');
    return;
  }

  const prevStartFrag = findFirstFragWithCC(prevFrags, curFrags[0].cc + 1);
  const curStartFrag = findFirstFragWithCC(curFrags, curFrags[0].cc + 1);

  if (!prevStartFrag || (prevStartFrag && !prevStartFrag.startPTS)) {
    logger.log('No frag in previous level to align on');
    return;
  }

  if (curStartFrag?.start) {
    return {
      start:  prevStartFrag.start - curStartFrag.start
    };
  }
}

Any comment? Thanks.

@hongfeih-es hongfeih-es added Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. Question labels Nov 8, 2021
@robwalch robwalch added this to Top priorities in Release Planning and Backlog via automation Jul 19, 2022
@robwalch
Copy link
Collaborator

robwalch commented Jan 4, 2023

I think the real problem is in alignDiscontinuities which uses findDiscontinuousReferenceFrag.

alignDiscontinuities needs a sliding time for the difference between two matching fragments. You are correct in that matching the first segment of a playlist to the first segment with the same discontinuity in another will not produce the correct offset if the first segment in the former playlist is not also the first in the discontinuity sequence.

This method should find any match between two fragments that start the same discontinuity sequence. It would also make sense for it to return a number when it finds a match, or null when unsuccessful.

@robwalch robwalch added Enhancement and removed Question Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels Jan 4, 2023
@robwalch
Copy link
Collaborator

robwalch commented Jan 4, 2023

Changed to enhancement. Will escalate to Bug if given steps to repro.

@robwalch
Copy link
Collaborator

robwalch commented Sep 30, 2023

It looks like we made improvements to alignMediaPlaylistByPDT in #4984 that are relevant to this issue. When aligning on PDT we actually find the best DISCONTINUITY for the job:

const targetCC = Math.min(refDetails.endCC, details.endCC);
if (refDetails.startCC < targetCC && details.startCC < targetCC) {
refFrag = findFirstFragWithCC(refFragments, targetCC);
frag = findFirstFragWithCC(fragments, targetCC);
}
if (!refFrag || !frag) {
refFrag = refFragments[Math.floor(refFragments.length / 2)];
frag =
findFirstFragWithCC(fragments, refFrag.cc) ||
fragments[Math.floor(fragments.length / 2)];

But we don't do the same in alignDiscontinuities - we only use the first which may not be present in both. As long as we use a disco sequence present in both, we should get the same alignment. If the same discontinuity sequence has different durations across playlists that is an issue with playlist authoring.

@robwalch
Copy link
Collaborator

@hongfeih-es would you bring the targetCC logic from alignMediaPlaylistByPDT into alignDiscontinuities and resolve this issue?

@robwalch robwalch added this to the 1.6.0 milestone May 12, 2024
@robwalch robwalch added the Verify Fixed An unreleased bug fix has been merged and should be verified before closing. label Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Verify Fixed An unreleased bug fix has been merged and should be verified before closing.
Projects
Release Planning and Backlog
  
Top priorities
Development

Successfully merging a pull request may close this issue.

3 participants