-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add support for emsg ID3 metadata in fmp4 segments #4458
Conversation
f8a54a1
to
3c7b6b3
Compare
3e59a8a
to
7764d9a
Compare
Adding reference link for data structure for reference/posterity (link in the original issue is incorrect or out of date): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still going to do some testing, but on a code only review looks 👌 so far other than a few minor asks (defer to you on whether you think the changes are necessary).
} from '../utils/mp4-tools'; | ||
import { dummyTrack } from './dummy-demuxed-track'; | ||
import type { HlsEventEmitter } from '../events'; | ||
import type { HlsConfig } from '../config'; | ||
|
||
const emsgSchemePattern = /\/emsg[-/]ID3/i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about best practices here, but are we using this looser match to handle both "https://aomedia.org/emsg/ID3"
(defined in: https://aomediacodec.github.io/av1-id3/) and the outdated "https://developer.apple.com/streaming/emsg-id3"
? If so, should we be more explicit with those two matches for potential future-proofing concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purposes of this contribution, I wanted the broadest support for available test content, but could not explicitly add support for undocumented schemes. Perhaps other players are even less strict that this?
|
||
timeScale = readUint32(data, 12); | ||
presentationTimeDelta = readUint32(data, 16); | ||
eventDuration = readUint32(data, 20); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is unused in our codebase, but should we make eventDuration = undefined
or NaN
or some other special value in cases where its value is 0xFFFFFFFF
for clarity? From the spec:
In general, ID3 don’t carry a duration and in those cases the event_duration field should be set to 0xFFFFFFFF. If in a particular case, the ID3 message carries a duration, it should be reflected in the event_duration field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is part of the spec, it is included for future use. It would help to have sample content with event durations defined. For everything else we have an open issue with some discussion on the subject #3879 (duration=Infinity
until next cue).
id3Track, | ||
timeOffset, | ||
id3InitPts, | ||
id3InitPts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not having enough context here, wondering why we're using pts as dts here? Worried we might get some cross-browser inconsistencies with timings if we're not careful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of both PTS and DTS here is legacy code for dealing with MPEG-TS 33-bit timestamp rollover (this is not new code, just code I moved into flushTextTrackMetadataCueSamples
). Since we can expect all emsg samples to have identical pts and dts, we can use the same value for both inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basic smoke testing using the test playlists looks good. Consider this pre-approved, with your discretion on resolving any of the outstanding questions/asks.
7764d9a
to
c5993b5
Compare
c5993b5
to
a5c669a
Compare
Hi, We really want to use this feature, but from what I see it's not on the latest tag (v1.1.5), is there any ETA for the next release? Thanks! Eran |
Hello there, great feature, is there any ETA for the next release? Thanks |
Hi @erankor and @petronioamaral, This feature is in the latest beta release: v1.2.0-beta.1. If you have time next week, please try it out next week and provide feedback. v1.2.0 will be released soon. |
Hi @robwalch I tested here and seems work very well, let us know when will be merge with the v1.2.0. |
This PR will...
Add support for emsg timed metadata within fragmented MP4.
These changes allow for timed metadata delivery (ID3 via emsg) for use cases such as content labeling and advertising in CMAF streams in the same way HLS.js currently supports ID3 in other container formats.
HTMLMediaElement
text track and cues generated by HLS.js are similar in timing and content to those added with native HLS playback in Safari.Supported schemas include any url containing "/emsg/ID3" or "/emsg-id3" (case insensitive).
Sample test streams from #2360:
Resolves issues:
Closes #2360
Checklist