Repository navigation
Add fmp4 segmented output - #1419
Conversation
|
AV-sync stats summary: view in run #39301754421 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if !pending { | ||
| return nil | ||
| } | ||
| return s.uploadInitSegment() |
There was a problem hiding this comment.
🟡 Backup copy inflates output size
When backup activates, uploadInitSegment adds the same initialization file to SegmentsInfo.Size again. The reported output size therefore counts one logical object twice.
Learn more
The first initialization-segment upload already contributes its bytes to SegmentsInfo.Size. The backup upload copies that same logical object for playlist consistency, but calling the same helper repeats the accounting.
Example: A 20 KB initialization segment contributes 20 KB after the primary upload. If media later falls back, the backup copy contributes another 20 KB, so the result is 20 KB larger than the produced playlist and unique media objects.
Recommended fix: Separate upload destination handling from logical-size accounting. Count the initialization segment only on its first successful upload, while still updating its manifest location when a backup copy becomes authoritative.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
| func configureFMP4Muxer(muxer *gst.Element) error { | ||
| return muxer.SetProperty("fragment-duration", uint64(fmp4FragmentDuration)) |
There was a problem hiding this comment.
Just a note: If partial segments (#EXT-X-PART) are needed in future for LL HLS, we would also want to set chunk-duration. Lowering fragment-duration for that purpose seems to add a key frame at fragment boundary by default, which will be an overhead.
There was a problem hiding this comment.
This is correct. This patch implements fmp4 and not LL-HLS. LL-HLS would be impossible with a scheme where segments and playlists are pushed to a blob store.
Each segment is its own fragment with 1 chunk and starts with a key frame.
| // fallen back to backup storage, so that a playlist uploaded there resolves its | ||
| // EXT-X-MAP. The path is relative to the playlist, so the two have to share a bucket. | ||
| // It is a no-op until the fallback happens, and once per fallback after that. | ||
| func (s *SegmentSink) ensureInitSegmentInBackup() error { |
There was a problem hiding this comment.
uploadPlaylist calls ensureInitSegmentInBackup first, but uploadLivePlaylist doesn't, and the live playlist also carries the EXT-X-MAP. After the first hour shouldUploadPlaylist throttles the event playlist to every Nth segment (up to a minute), while the live playlist is uploaded on every segment, so if the uploader falls back to backup on a segment that isn't the event playlist's turn, doesn't the live playlist land in backup pointing at an init segment that isn't there yet, until the next event-playlist upload re-uploads it? Since ensureInitSegmentInBackup is a no-op once done, could uploadLivePlaylist call it too?
There was a problem hiding this comment.
Yes, fixed in 7767479: uploadLivePlaylist now calls ensureInitSegmentInBackup first. Both playlist uploads run under playlistLock, so the init segment won't be uploaded twice.
🤖 Generated with Claude Code
|
I like that we kept the existing flow (despite the need for box surgery) - 👍 |
A segmented file output can now request HLS_FMP4_PROTOCOL and get fragmented MP4 segments instead of MPEG-TS ones. SegmentConfig gains SegmentOutputType, which describes the segments themselves as opposed to OutputType, which describes the playlist; an unrecognized protocol is now rejected rather than silently leaving the output type unset. splitmuxsink builds isofmp4mux for these segments. cmafmux only takes a single track, so it cannot carry audio and video together. The muxer is configured from muxer-added, because splitmuxsink only applies muxer-properties when finalizing asynchronously, and its fragment duration is set far beyond any segment so that splitmuxsink's own EOS stays the only fragment boundary. splitmuxsink resets the muxer for every segment, so each one is written with its own ftyp and moov. The sink keeps that copy from the first segment as the initialization segment the playlist points at with EXT-X-MAP, and removes it from every segment before upload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The known muxer test now covers isofmp4mux, so an image without it fails rather than only failing an fmp4 egress, and a new test pins the properties the segment bin sets on splitmuxsink and on the muxer. The fmp4 muxers come from gst-plugins-rs, which moves faster than the rest of the image. Factory names move to constants, shared by the builder and its tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RFC 8216 section 7 removed the tag in protocol version 7, which is the version fmp4 playlists declare, so it is only written below that now. The playlist versions move to constants, and the comment on them no longer claims EXT-X-MAP needs version 7: without EXT-X-I-FRAMES-ONLY it needs 6, and 7 is fmp4's own requirement. The integration playlist parser counted header lines, so a playlist without the tag shifted every field it read. It looks tags up by name instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two gaps from review, both about the initialization segment being uploaded once and then forgotten. The uploader sends everything to backup storage once a primary upload fails. The init segment is uploaded before the first media segment, so a failure after it left the playlist and the segments in backup referencing an init segment that only exists in primary, and an EXT-X-MAP relative to the playlist cannot reach it. The local copy is now kept and re-uploaded the first time a playlist is uploaded after the fallback. The manifest recorded the playlist and its segments but not the init segment, so a consumer copying what it lists ended up with media it could not decode. It is recorded as its own field rather than as a segment: it holds no media, and every segment depends on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The init segment's backup state was derived from the egress-wide BackupStorageUsed flag, which any output's fallback sets. Report the destination of each upload from the uploader instead, and decide on a re-upload from the segment sink's own uploader state. Also make sure the init segment is in backup before uploading the live playlist, which references it too. primaryFailed is now atomic, since segment uploads run concurrently. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
7767479 to
b117c3b
Compare
3 participants
A segmented file output can now request
HLS_FMP4_PROTOCOLand get fragmentedMP4 segments instead of MPEG-TS ones. Playlists stay
.m3u8, segments become.m4salongside aninit.mp4referenced withEXT-X-MAP.Uses the HLS_FMP4_PROTOCOL enum from livekit/protocol#1828.
Config
SegmentConfiggainsSegmentOutputType, describing the segments themselves asopposed to
OutputType, which describes the playlist, plusInitSegmentFilenamefor containers that need a separate initialization segment. An unrecognized
protocol is now rejected instead of silently leaving the output type unset.
Pipeline
splitmuxsink builds
isofmp4muxfor these segments.cmafmuxhas a singlealways-sink pad and cannot carry audio and video together — with both connected
the pipeline fails to link at all — so it is not usable for a muxed segment.
The muxer is configured from
muxer-addedrather thanmuxer-properties, whichsplitmuxsink only applies when finalizing asynchronously. Its fragment duration
is set far beyond any segment so splitmuxsink's own EOS stays the only fragment
boundary, giving one fragment per segment.
Initialization segment
splitmuxsink resets the muxer for every segment, so each is written starting with
its own
ftypandmoov. The sink keeps that copy from the first segment as theinitialization segment the playlist points at, and strips it from every segment
before upload.
reset-muxer=falsewould have avoided the repetition but does notwork with this muxer: the first file is valid and every later one comes out empty,
matching the property's own documented warning.
Worth considering later: driving
isofmp4muxdirectly, without splitmuxsink,would remove the stripping entirely. On its own the muxer emits the initialization
segment once and then one buffer per fragment, which is exactly the split we want.
It means owning segment file writing and the fragment accounting the sink currently
gets from splitmuxsink's bus messages, so it is a rewrite of the segment path
rather than a config change — but it is the cleaner shape, and it is what
hlscmafsinkdoes.Testing
Unit tests cover the protocol-to-output-type mapping, the muxer configuration, the
EXT-X-MAPplaylist header, the box walk that splits the initialization segment(including a fragment larger than the shift buffer), and the playlist parser across
TS, fmp4 and live playlists.
The integration suite gains a
RoomComposite/FMP4case, and its verification nowdownloads the initialization segment (it is not in the manifest, so ffprobe could
not resolve
EXT-X-MAPwithout it) and asserts the structure:init.mp4is exactlyftyp+moov, and every segment starts withstyp, holds onemoof, and repeatsneither
ftypnormoov. The existing playlist parser counted header linespositionally and panicked on an
EXT-X-MAP; it now finds the segments instead.I verified the pipeline behaviour out-of-band against real GStreamer output —
segments carry a single fragment, ffmpeg decodes the full playlist cleanly, and the
structure assertions fail as intended on unstripped segments. The integration case
itself has not been run; that needs a deployed server.
🤖 Generated with Claude Code