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

id3: add duration to metadata samples, mimic Safari behavior on unbounded cues #4967

Merged
merged 2 commits into from
Nov 3, 2022
Merged

id3: add duration to metadata samples, mimic Safari behavior on unbounded cues #4967

merged 2 commits into from
Nov 3, 2022

Conversation

jprjr
Copy link
Contributor

@jprjr jprjr commented Oct 14, 2022 •
edited

This PR will...

  • Add a duration field to Metadata Samples.
  • Set the duration to Infinite from sources that don't carry a duration.
  • Set the duration to Infinite from fragmented MP4 files with an event_duration of 0xffffffff or <= .001 seconds.
  • Create Cues from ID3 tags with an Infinite duration (where possible), or Number.MAX_VALUE as a fallback.
  • On new ID3 tags, update any existing, unbounded Cues

Why is this Pull Request needed?

On Safari, when an ID3 track is present, unbounded cues are initially created with an Infinite endTime. As new ID3 tags come in, all previous cues have the endTime updated. Cues may also have a valid endTime, if the underlying source supports it (fragmented mp4).

The current implementation is tracking for changes on the individual ID3 frame level, but Safari will update all existing, unbounded Cues, not just cues with the same ID3 frame ID.

Additionally, current implementation uses the playlist length - on a live playlist this is often quite short, and the Cue expires earlier than intended, especially if ID3 tags aren't being inserted often.

I have a short demo here comparing native, latest release, and this branch: https://jrjrtech.com/mpegts-loop/

Are there any points in the code the reviewer needs to double check?

The section that checks for a negative duration and sets a minimum duration. Infinity - (anything) is still infinity, so in the future I don't think it will be possible for that case to occur.

Resolves issues:

#4964 (fully)

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • [] API or design changes are documented in API.md

@jprjr jprjr changed the title id3 cues: default to infinite/max_value length (WIP) id3 cues: default to infinite/max_value length Oct 15, 2022
@jprjr jprjr mentioned this pull request Oct 15, 2022
Closed
5 tasks
@robwalch robwalch self-requested a review October 18, 2022 20:47
@robwalch
Copy link
Collaborator

robwalch commented Oct 18, 2022 •
edited

Hi @jprjr,

To resolve this build error https://github.com/video-dev/hls.js/actions/runs/3251442546/jobs/5393313941#step:7:29\
please run npm run prettier and commit the formatting changes.

The rest looks great! Infinite duration when possible and updating unbounded cues at once is a nice optimization and appears to match Safari well.

@robwalch robwalch added this to the 1.2.5 milestone Oct 18, 2022
@jprjr
Copy link
Contributor Author

jprjr commented Oct 18, 2022 via email

@jprjr jprjr changed the title (WIP) id3 cues: default to infinite/max_value length id3: add duration to metadata samples, mimic Safari behavior on unbounded cues Oct 23, 2022
@jprjr
Copy link
Contributor Author

jprjr commented Oct 23, 2022

@robwalch I just pushed up another, more comprehensive attempt on this - it adds a duration field to Metadata samples and uses Number.POSITIVE_INFINITY for packed audio and MPEG-TS, and either the duration, or Number.POSITIVE_INFINITY for emsg.

robwalch
robwalch requested changes Oct 26, 2022
View reviewed changes
src/controller/id3-track-controller.ts Outdated
const Cue = getCueClass();
let updateCueRanges = false;
const frameTypesAdded: Record<string, number | null> = {};
let updateCueRanges = true;
Copy link
Collaborator

@robwalch robwalch Oct 26, 2022 •
edited

Choose a reason for hiding this comment

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

Not a Contribution

(@jprjr you can ignore the disclaimer above)

I see a discrepancy between the native and HLS.js behavior when a segment has two samples/frames starting at different times. Given these samples and parsed frames:

samples = [{
    data: Uint8Array(71),
    dts: 0.04401111111111078,
    duration: Infinity,
    len: 71,
    pts: 0.04401111111111078,
    type: "https://aomedia.org/emsg/ID3"
  }, {
    data: Uint8Array(71),
    dts: 5.0440000000000005,
    duration: Infinity,
    len: 71,
    pts: 5.0440000000000005,
    type: "https://aomedia.org/emsg/ID3"
  }];
frames = { // same for each sample
  data:  " *** THIS IS Timed MetaData *** "
  info: ""
  key: "TXXX"
}.

Safari native HLS playback lays them out one after the other:

image

With this changes in this PR, we get samples containing the same frame within a segment overlapping each other:

image

The current release produces the same results as Safari for cases like this, but the optimization to only update cues for each segment once seems to be introducing this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, yeah - I think I can change it to just always update any previously unbounded cues, I'll get an update pushed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robwalch - made a change to update any previously-created, unbounded cues on any new ID3 frame, tested on Safari and it should match the behavior - let me know if these two commits should be squashed or kept as separate commits

@mataspetrikas mataspetrikas mentioned this pull request Nov 1, 2022
Closed
5 tasks
@jprjr
id3: add duration to metadata samples, mimic Safari behavior on
da9da57 
unbounded cues.

Related to video-dev#4964

On Safari, when an ID3 track is present, cues are initially created with
an Infinite endTime for MPEG-TS and Packed Audio sources. With fragmented
MP4, they may have an Infinite endTime if the duration is 0xffffffff, or
very small - otherwise it's finite.

As new ID3 tags come in, any unbounded cues have the endTime updated.

The current draft of VTTCue allows an unrestricted double for the
endTime - which allows for using Number.POSITIVE_INFINITY.
It hasn't rolled out yet, so this falls back to Number.MAX_VALUE.
@robwalch robwalch self-requested a review November 3, 2022 23:41
@robwalch robwalch merged commit e419d9b into video-dev:master Nov 3, 2022
robwalch pushed a commit that referenced this pull request Nov 4, 2022
@jprjr @robwalch
id3: add duration to metadata samples, mimic Safari behavior on unbou…
5a4ec7a 
…nded cues (#4967)

* id3: add duration to metadata samples, mimic Safari behavior on
unbounded cues.

Related to #4964

On Safari, when an ID3 track is present, cues are initially created with
an Infinite endTime for MPEG-TS and Packed Audio sources. With fragmented
MP4, they may have an Infinite endTime if the duration is 0xffffffff, or
very small - otherwise it's finite.

As new ID3 tags come in, any unbounded cues have the endTime updated.

The current draft of VTTCue allows an unrestricted double for the
endTime - which allows for using Number.POSITIVE_INFINITY.
It hasn't rolled out yet, so this falls back to Number.MAX_VALUE.

* id3: remove the optimization to only update cues once
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robwalch robwalch robwalch approved these changes

Assignees
No one assigned
Labels
metadata
Projects
None yet
Milestone
1.2.5
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jprjr @robwalch