Code review fixes

This commit is contained in:
Vanilagy
2026-04-16 17:43:12 +02:00
parent aadfa77489
commit b85ecd3109
13 changed files with 253 additions and 44 deletions
+4 -1
View File
@@ -117,11 +117,14 @@ const input = createInputFrom(file, ALL_FORMATS);
const duration = await input.computeDuration(); // in seconds
const videoTrack = await input.getPrimaryVideoTrack();
const audioTrack = await input.getPrimaryAudioTrack();
const displayWidth = await videoTrack.getDisplayWidth();
const displayHeight = await videoTrack.getDisplayHeight();
const { rotation } = videoTrack;
const rotation = await videoTrack.getRotation();
const sampleRate = await audioTrack.getSampleRate();
const numberOfChannels = await audioTrack.getNumberOfChannels();
const { title, artist, album } = await input.getMetadataTags();
```
+173
View File
@@ -0,0 +1,173 @@
================================================================================
MEDIABUNNY HLS BRANCH - BUG FIXES AND BEHAVIOR CHANGES TO EXISTING CODE
For use in release notes. Only covers changes to pre-existing code.
New HLS/CMAF features are not listed here.
================================================================================
============================================================
BUG FIXES
============================================================
1. SourceRef race conditions (commit aa810fc)
- Source cache entry was added BEFORE the reference was fully created,
allowing premature garbage collection of cached sources
- Cache eviction variable pointed to wrong ref (local instead of entry)
- `count > MAX_SOURCE_CACHE_SIZE` off-by-one -> `count >= MAX_SOURCE_CACHE_SIZE`
- Input.source and Input.target getters could return unused/stale instances
when using async callbacks; now properly tracked via _getRootSourceRef()
2. Track closing race conditions in all muxers (commit 94678dd)
- ISOBMFF, Matroska, MPEG-TS, and OGG muxers used `trackData.track.source._closed`
to check if tracks were closed. This was racy because the closed state could
change between check and use. Added explicit `closed: boolean` field to all
muxer track data structures, set synchronously in onTrackClose().
3. Target.slice() validation bug (commit 7b70756)
- Validation used `!Number.isInteger(offset) && offset < 0` (AND), meaning
negative non-integer offsets slipped through. Fixed to use OR (`||`).
4. MPEG-TS demuxer: off-by-one errors in reorder buffer logic
- Rewind loop changed from `reorderSize` to `reorderSize + 1` iterations,
fixing packet duration calculation near seek points
- End-of-stream rewind loop changed from `reorderSize - 1` to `reorderSize`
- Flush condition changed from `>= reorderSize` to `> reorderSize`, keeping
one extra packet in the buffer before flushing for correct presentation
order computation
5. MPEG-TS demuxer: first chunk assertion crash (line 1031-1035)
- Old code asserted that a key frame is always found in the first chunk,
crashing on files where that assumption doesn't hold (e.g., HLS segments
starting mid-GOP). Changed to gracefully return null.
6. MPEG-TS demuxer: video parameters only searched in first packet
- Some muxers place SPS/PPS in later packets. The demuxer now loops through
multiple packets to find AVC/HEVC decoder configuration records instead of
only checking the first one.
7. ISOBMFF: duration calculation didn't account for sample duration
- `lastPresentedSample()` found the sample with the highest timestamp but
didn't add its duration. Replaced with `presentationSpan()` that correctly
computes `maxEndTimestamp - minTimestamp`.
8. ISOBMFF demuxer: assertion crash on unavailable data
- Demuxer asserted that read slices are non-null. Changed to gracefully
return null when data is outside available range (important for streaming/
progressive scenarios).
9. ISOBMFF demuxer: multiple trun boxes per fragment rejected
- Old code logged a warning and skipped the second trun box. Now correctly
accumulates samples across multiple trun boxes per track per fragment,
maintaining cumulative offset and timestamp state.
10. Conversion API: track count validation before fan-out consideration
(commit 2f576aa)
- Tracks were validated against the max count before considering that some
track options had `discard: true`. Now validates inside the fan-out loop.
11. MP3 muxer: frame positions recorded at wrong offset
- Frame byte positions were recorded AFTER writing packet data. Now recorded
BEFORE writing, which is correct for Xing TOC frame offset calculations.
12. FLAC demuxer: missing STREAMINFO validation
- Added explicit error when STREAMINFO metadata block is missing,
producing a clear "Corrupted FLAC file" message instead of undefined
behavior downstream.
============================================================
BEHAVIOR CHANGES
============================================================
1. Default keyFrameInterval changed from 5 seconds to 2 seconds
(commit c6e505a)
Aligns with default HLS segment duration. Affects all video encoding
that doesn't explicitly set keyFrameInterval.
2. Conversion API copies input pairability graph by default (commit 6410ea9)
When track group is not explicitly specified, conversion now auto-creates
OutputTrackGroups that mirror the input track pairing relationships. This
means converted outputs preserve which tracks are meant to play together.
3. Matroska demuxer: removed implicit track sorting by default disposition
Tracks are no longer sorted so that default=true tracks come first.
Now all tracks have `primary: false` by default. Callers should use the
getPrimaryVideoTrack()/getPrimaryAudioTrack() methods for selection.
4. Removed superfluous end-position seeks in muxer finalize methods
(commit 7b70756)
MP3, FLAC, and Matroska muxers no longer seek to the end of file after
writing final metadata. The seek was unnecessary since the writer is
done at that point.
5. clampCropRectangle now returns a new object instead of mutating input
(src/sample.ts:1123)
6. Date.now() -> performance.now() in FinalizationRegistry callback
(src/sample.ts:48)
Uses monotonic time for timing measurements.
7. Codec string parsing: mp4a.40.34 now correctly identified as MP3
(src/codec.ts:677-686)
Previously would have matched the aac prefix check. MP3 check now
runs first and includes this codec string.
============================================================
DEPRECATIONS
============================================================
1. InputTrack sync property getters (commit 051578c)
All sync getters on InputTrack/InputVideoTrack/InputAudioTrack are now
deprecated in favor of async methods:
.codec -> await .getCodec()
.languageCode -> await .getLanguageCode()
.name -> await .getName()
.timeResolution -> await .getTimeResolution()
.disposition -> await .getDisposition()
.displayWidth -> await .getDisplayWidth()
.displayHeight -> await .getDisplayHeight()
.rotation -> await .getRotation()
(etc.)
The sync getters still work for non-HLS inputs but throw when the
backing requires async resolution (e.g., HLS tracks before hydration).
2. Source.onread callback -> source.on('read', handler)
The onread setter still works but is marked @deprecated.
3. Target.onwrite callback -> target.on('write', handler)
Same as above.
============================================================
REMOVALS
============================================================
1. Target.onfinalized callback (commit 6410ea9)
Removed entirely. Use the 'finalized' event: target.on('finalized', ...).
2. InputTrackDescriptor concept (commit 051578c)
The entire InputTrackDescriptor API was removed. Its functionality
(pairable tracks, primary track selection) is now directly on InputTrack.
3. InputTrack[] option for ConversionOptions.tracks (commit 606ee87)
ConversionOptions.tracks now only accepts 'all' | 'primary', no longer
accepts an array of InputTrack instances.
4. Unnecessary sync getter wrappers (commit 8241820)
Removed deprecated sync getters for: hasOnlyKeyPackets, bitrate,
averageBitrate, isRelativeToUnixEpoch on InputTrack.
============================================================
API ADDITIONS (for context, not release-note-worthy on their own)
============================================================
- PacketRetrievalOptions.skipLiveWait (fixes #342)
- DiscardedTrack.trackOptions field
- TargetRequest.mimeType
- TrackDisposition.primary
- Source/Target/Output extend EventEmitter
- CanvasSink methods now async generators (getCanvas, canvases, canvasesAtTimestamps)
- EncodedPacketSink.getFirstPacket/getPacket/getNextPacket now async
- Muxer writers deferred to start() instead of constructor
+1 -1
View File
@@ -499,4 +499,4 @@ On the flip side, you can always query which input tracks made it into the outpu
const conversion = await Conversion.init({ input, output });
conversion.utilizedTracks; // => InputTrack[]
```
A track may appear multiple times in this list when [fan-out](#fan-out) produces multiple output tracks from it.
A track may appear multiple times in this list when [fan-out](#track-fan-out) produces multiple output tracks from it.
+7 -3
View File
@@ -402,7 +402,9 @@ const validateVideoOptions = (videoOptions: ConversionVideoOptions) => {
|| (Array.isArray(videoOptions.group) && videoOptions.group.every(x => x instanceof OutputTrackGroup))
)
) {
throw new TypeError('options.video.group, when provided, must be a string or an array of strings.');
throw new TypeError(
'options.video.group, when provided, must be an OutputTrackGroup or an array of OutputTrackGroups.',
);
}
};
@@ -462,7 +464,9 @@ const validateAudioOptions = (audioOptions: ConversionAudioOptions) => {
|| (Array.isArray(audioOptions.group) && audioOptions.group.every(x => x instanceof OutputTrackGroup))
)
) {
throw new TypeError('options.audio.group, when provided, must be a string or an array of strings.');
throw new TypeError(
'options.audio.group, when provided, must be an OutputTrackGroup or an array of OutputTrackGroups.',
);
}
};
@@ -612,7 +616,7 @@ export class Conversion {
&& options.tracks !== 'primary'
) {
throw new TypeError(
'options.tracks, when provded, must be either \'all\' or \'primary\'.',
'options.tracks, when provided, must be either \'all\' or \'primary\'.',
);
}
if (
+3 -3
View File
@@ -203,7 +203,7 @@ export const validateVideoEncodingConfig = (config: VideoEncodingConfig) => {
throw new TypeError('config.transform.rotate, when provided, must be 0, 90, 180 or 270.');
}
if (config.transform.crop !== undefined) {
validateCropRectangle(config.transform.crop, 'config.');
validateCropRectangle(config.transform.crop, 'config.transform.');
}
if (config.transform.process !== undefined && typeof config.transform.process !== 'function') {
throw new TypeError('config.transform.process, when provided, must be a function.');
@@ -219,7 +219,7 @@ export const validateVideoEncodingConfig = (config: VideoEncodingConfig) => {
}
}
if (config.onEncodedPacket !== undefined && typeof config.onEncodedPacket !== 'function') {
throw new TypeError('config.onEncodedChunk, when provided, must be a function.');
throw new TypeError('config.onEncodedPacket, when provided, must be a function.');
}
if (config.onEncoderConfig !== undefined && typeof config.onEncoderConfig !== 'function') {
throw new TypeError('config.onEncoderConfig, when provided, must be a function.');
@@ -438,7 +438,7 @@ export const validateAudioEncodingConfig = (config: AudioEncodingConfig) => {
}
}
if (config.onEncodedPacket !== undefined && typeof config.onEncodedPacket !== 'function') {
throw new TypeError('config.onEncodedChunk, when provided, must be a function.');
throw new TypeError('config.onEncodedPacket, when provided, must be a function.');
}
if (config.onEncoderConfig !== undefined && typeof config.onEncoderConfig !== 'function') {
throw new TypeError('config.onEncoderConfig, when provided, must be a function.');
+1 -1
View File
@@ -496,7 +496,7 @@ export class HlsDemuxer extends Demuxer {
const channels = mediaTag.attributes.get('channels')
?? variantStream.attributes.get('channels');
const parsedChannels = channels !== null
? Number(channels)
? Number(channels.split('/')[0]!)
: null;
result.push({
+31 -22
View File
@@ -294,19 +294,21 @@ export class HlsMuxer extends Muxer {
}
}
const getMetadataKeyForTrack = ({ metadata }: OutputTrack) => {
let key = '';
key += `${metadata.languageCode ?? UNDETERMINED_LANGUAGE}-`;
key += `${metadata.name ?? ''}-`;
key += `${metadata.disposition?.default ?? true}-`;
key += `${metadata.disposition?.primary ?? false}-`;
key += `${metadata.disposition?.forced ?? false}-`;
return key;
};
// Video tracks that can't be paired with any other track always live on the top-level, the question is just if
// they need to be separated into #EXT-X-MEDIA tags or not
if (unpairedVideoTracks.length > 0) {
const uniqueMetadata = new Set(unpairedVideoTracks.map(({ metadata }) => {
let key = '';
key += `${metadata.languageCode ?? UNDETERMINED_LANGUAGE}-`;
key += `${metadata.name ?? ''}-`;
key += `${metadata.disposition?.default ?? true}-`;
key += `${metadata.disposition?.primary ?? false}-`;
key += `${metadata.disposition?.forced ?? false}-`;
return key;
}));
const uniqueMetadata = new Set(unpairedVideoTracks.map(getMetadataKeyForTrack));
if (uniqueMetadata.size > 1) {
// They differ in metadata, emit as group
@@ -336,16 +338,7 @@ export class HlsMuxer extends Muxer {
// Audio tracks that can't be paired with any other track always live on the top-level, the question is just if
// they need to be separated into #EXT-X-MEDIA tags or not
if (unpairedAudioTracks.length > 0) {
const uniqueMetadata = new Set(unpairedAudioTracks.map(({ metadata }) => {
let key = '';
key += `${metadata.languageCode ?? UNDETERMINED_LANGUAGE}-`;
key += `${metadata.name ?? ''}-`;
key += `${metadata.disposition?.default ?? true}-`;
key += `${metadata.disposition?.primary ?? false}-`;
key += `${metadata.disposition?.forced ?? false}-`;
return key;
}));
const uniqueMetadata = new Set(unpairedAudioTracks.map(getMetadataKeyForTrack));
if (uniqueMetadata.size > 1) {
// They differ in metadata, emit as group
@@ -686,6 +679,17 @@ export class HlsMuxer extends Muxer {
// Loop in case we can finalize multiple segments
while (true) {
// This here is the core segmentation logic. The segmentation logic figures out which packets are to be
// written into the next segment, and if we can write a segment at all. If tracks are still open and have
// not provided sufficient media data, no segment will be written. The packets will be added to the segment
// to maximize its duration AND keep it from exceeding the target duration. This condition is extended with
// a key frame rule for video, meaning the algorithm must guarantee that every segment with video data
// begins with a video key frame.
//
// The logic is quite complex but is solved in a straight-forward way: all possible permutations of the
// problem are checked in a nested if-else structure, making sure all cases behave correctly. This was the
// easiest, least error-prone way I found to express this behavior.
const currentSegmentEndTimestamp = playlist.currentSegmentStartTimestamp + this.targetSegmentDuration;
// These store the index (exclusive) until when packets can be added to the next segment
@@ -816,6 +820,9 @@ export class HlsMuxer extends Muxer {
if (this.singleFilePerPlaylist) {
if (playlist.singleFile === null) {
// INTENTIONALLY shadow the outside `segmentInfo` because we don't want to set it.
// In single-file mode, onSegment is called once in onPlaylistDone instead of per-segment,
// so the outer `segmentInfo` intentionally stays null in this case.
const segmentInfo: HlsOutputSegmentInfo = {
n: playlist.nextSegmentId,
format: playlist.segmentFormat,
@@ -1028,6 +1035,7 @@ export class HlsMuxer extends Muxer {
assert(Number.isFinite(nextSegmentStartTimestamp));
const segmentDuration = nextSegmentStartTimestamp - playlist.currentSegmentStartTimestamp;
assert(segmentDuration >= 0);
playlist.writtenSegments.push({
path: relativeSegmentPath,
@@ -1119,7 +1127,8 @@ export class HlsMuxer extends Muxer {
// Fallback: if no contiguous set falls within the range, use per-segment max
if (peakBitrate === 0) {
for (const segment of segments) {
peakBitrate = Math.max(peakBitrate, 8 * segment.byteSize / segment.duration);
const segmentDuration = segment.duration || 1; // To catch 0-duration segments which can happen
peakBitrate = Math.max(peakBitrate, 8 * segment.byteSize / segmentDuration);
}
}
@@ -1166,7 +1175,7 @@ export class HlsMuxer extends Muxer {
+ (!this.isLive ? '#EXT-X-PLAYLIST-TYPE:VOD\n' : '')
+ `#EXT-X-TARGETDURATION:${Math.ceil(targetDuration)}\n` // Must be a "decimal-integer"
+ (Number.isFinite(this.maxLiveSegmentCount) ? `#EXT-X-MEDIA-SEQUENCE:${playlist.mediaSequence}\n` : '')
+ '#EXT-X-INDEPENDENT-SEGMENTS\n' // Todo not for live?
+ '#EXT-X-INDEPENDENT-SEGMENTS\n'
+ (isKeyPacketsOnly ? '#EXT-X-I-FRAMES-ONLY\n' : '')
+ (playlist.initSegment
? (`#EXT-X-MAP:URI="${playlist.initSegment.path}"`
+1 -1
View File
@@ -187,7 +187,7 @@ export class HlsSegmentedInput extends SegmentedInput {
if (!line.startsWith('#')) {
if (!prevLastSegment) {
if (nextSegmentDuration === null) {
throw new Error('Invalid M3U8 file; a segment must be preceeded by a #EXTINF tag.');
throw new Error('Invalid M3U8 file; a segment must be preceded by an #EXTINF tag.');
}
let key = currentKey;
+20 -10
View File
@@ -13,23 +13,33 @@ export const readNextMp3FrameHeader = async (reader: Reader, startPos: number, u
header: Mp3FrameHeader;
startPos: number;
} | null> => {
const CHUNK_SIZE = 2 ** 16; // So we don't need to grab thousands of slices
let currentPos = startPos;
// todo optimize this shit wtf is this
while (until === null || currentPos < until) {
let slice = reader.requestSlice(currentPos, FRAME_HEADER_SIZE);
const maxLength = until !== null
? Math.min(CHUNK_SIZE, until - currentPos)
: CHUNK_SIZE;
let slice = reader.requestSliceRange(currentPos, FRAME_HEADER_SIZE, maxLength);
if (slice instanceof Promise) slice = await slice;
if (!slice) break;
if (!slice || slice.length < FRAME_HEADER_SIZE) break;
const word = readU32Be(slice);
while (slice.remainingLength >= FRAME_HEADER_SIZE) {
const posBeforeRead = slice.filePos;
const word = readU32Be(slice);
const remainingBytes = reader.fileSize !== null
? reader.fileSize - currentPos
: null;
const result = readMp3FrameHeader(word, reader.fileSize !== null ? reader.fileSize - currentPos : null);
if (result.header) {
return { header: result.header, startPos: currentPos };
const result = readMp3FrameHeader(word, remainingBytes);
if (result.header) {
return { header: result.header, startPos: currentPos };
}
slice.filePos = posBeforeRead + result.bytesAdvanced;
currentPos = slice.filePos;
}
currentPos += result.bytesAdvanced;
}
return null;
+3
View File
@@ -1342,6 +1342,9 @@ export class HlsOutputFormat extends OutputFormat {
if (options.getSegmentPath !== undefined && typeof options.getSegmentPath !== 'function') {
throw new TypeError('options.getSegmentPath, when provided, must be a function.');
}
if (options.getInitPath !== undefined && typeof options.getInitPath !== 'function') {
throw new TypeError('options.getInitPath, when provided, must be a function.');
}
if (options.onMaster !== undefined && typeof options.onMaster !== 'function') {
throw new TypeError('options.onMaster, when provided, must be a function.');
}
+2 -1
View File
@@ -445,7 +445,7 @@ export class Output<
&& typeof options.initTarget !== 'function'
) {
throw new Error(
'options.getInitTarget, when provided, must be a Target or a function that returns or resolves to'
'options.initTarget, when provided, must be a Target or a function that returns or resolves to'
+ ' a Target.',
);
}
@@ -550,6 +550,7 @@ export class Output<
target._output = this;
if (this.state === 'canceled') {
// Promise thrown away here, but no way to surface it to the user really
void target._close();
} else {
this._targets.add(target);
-1
View File
@@ -594,7 +594,6 @@ export class FilePathTarget extends Target {
chunked: true,
...options,
});
this._streamTarget._output = this._output;
}
/** @internal */
+7
View File
@@ -1945,10 +1945,13 @@ test('Single-file mode', async () => {
let playlistText: string | null = null;
const segmentPaths = new Set<string>();
const onSegment = vi.fn();
const output = new Output({
format: new HlsOutputFormat({
segmentFormat: new MpegTsOutputFormat(),
singleFilePerPlaylist: true,
onSegment,
}),
target: new PathedTarget('', (request) => {
const target = new BufferTarget();
@@ -1979,6 +1982,8 @@ test('Single-file mode', async () => {
await source.add(new EncodedPacket(avcPacketData, 'delta', 3, 0), avcMetadata);
await source.add(new EncodedPacket(avcPacketData, 'delta', 3.5, 0), avcMetadata);
expect(onSegment).toHaveBeenCalledTimes(0);
await output.finalize();
// Only one segment file should have been created
@@ -1987,6 +1992,8 @@ test('Single-file mode', async () => {
expect(playlistText).not.toBeNull();
expect(playlistText!.match(/#EXT-X-BYTERANGE/g)).toHaveLength(2);
expect(playlistText).toContain('#EXT-X-VERSION:4');
expect(onSegment).toHaveBeenCalledTimes(1);
});
test('StreamTarget, write is called for each target', async () => {