Make the IDR interval configurable - #172
Draft
ruccho wants to merge 4 commits into
Draft
Conversation
The interval between IDR (key) frames was decided entirely on the native side, and inconsistently so: Android and FFmpeg forced one second, WebCodecs requested a keyframe every second from the frame loop, while VideoToolbox and Media Foundation were left at their platform defaults. Callers that do not rely on the bounded ring buffer, such as UnboundedRecordingSession or UniEnc used directly, pay for that forced interval without needing it. Expose the interval as VideoEncoderOptions.IdrIntervalSeconds, where null means "leave it at the platform encoder's default". It reaches the native options struct as a float whose non-positive values act as the sentinel for the absent case, since Option<f32> has no stable repr(C) layout. Each platform applies it through its own knob: - VideoToolbox: kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration - MediaCodec: KEY_I_FRAME_INTERVAL, now set as a float so sub-second intervals are expressible - Media Foundation: CODECAPI_AVEncMPVGOPSize, converted to frames using the frame rate hint, applied best-effort so an encoder that does not implement it still activates - FFmpeg: the -force_key_frames expression, omitted entirely for the default case - WebCodecs: the per-frame keyframe request threshold RealtimeEncodingOptions.Default keeps the previous one-second interval, so existing behaviour is unchanged unless the interval is set explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Modelling the interval as optional pushed a "leave it to the platform" case down to every encoder, and that case had no good answer: MediaCodec documents KEY_I_FRAME_INTERVAL as required for video encoders, and WebCodecs has no GOP-length knob at all, so both had to invent a value anyway. Requiring the interval removes the branch instead of choosing arbitrarily on each platform. VideoEncoderOptions.IdrIntervalSeconds becomes a plain float, validated alongside Width, Height and Bitrate, and every platform now applies it unconditionally. On the Rust side the trait method returns f32 with no default implementation, so a missing value is a compile error rather than a silent fallback. The native struct keeps its f32 field, now without the sentinel interpretation, so its layout is unchanged. InstantReplay keeps supplying 1 second: RealtimeEncodingOptions.Default, UniEncTranscoder, the PersistentRecorder sample and the UniEnc example all set it explicitly. BREAKING CHANGE: code that constructs UniEnc.VideoEncoderOptions directly must now set IdrIntervalSeconds. The struct's zero default is rejected by Validate(), so leaving it unset throws ArgumentException. Callers going through RealtimeEncodingOptions.Default are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… one Requiring the interval made the property a breaking addition: the struct's zero default is not a valid interval, so any existing code constructing VideoEncoderOptions directly would start throwing from Validate(). Make IdrIntervalSeconds nullable again, but give null a definite meaning. It selects VideoEncoderOptions.DefaultIdrIntervalSeconds — one second, the interval every platform used before this branch — rather than deferring to the platform encoder, whose own default differs per platform and can be far longer. The native side keeps receiving a concrete f32, so no platform code changes and the struct layout is untouched. With the default covering them, UniEncTranscoder, the PersistentRecorder sample and the UniEnc example no longer need to set the interval, so those three files revert to their original contents. RealtimeEncodingOptions.Default keeps setting it explicitly, since one second is InstantReplay's own choice rather than something to inherit silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the new option to the settings sample and explains what null means: the UniEnc default of one second, not the platform encoder's own default. Also notes the interaction with the export path, since an export can only begin at a key frame and an interval longer than the retained duration leaves the buffer with none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #166.
The interval between IDR (key) frames was decided entirely on the native side, and
inconsistently so:
KEY_I_FRAME_INTERVALhardcoded to 1-force_key_frames expr:gte(t,n_forced*1)The forced one-second interval exists so that
BoundedEncodedFrameBuffercan cut an exportedsegment close to the requested duration. Callers that do not use that buffer —
UnboundedRecordingSession, or UniEnc used directly — pay for it without needing it.Changes
UniEnc.VideoEncoderOptionsgainsfloat? IdrIntervalSeconds, in seconds, validated to befinite and greater than zero when set.
nullmeans the UniEnc default,VideoEncoderOptions.DefaultIdrIntervalSeconds(one second —the interval every platform effectively used before this branch). It does not mean "defer to
the platform encoder": platform defaults differ per platform and can be far longer, MediaCodec
documents
KEY_I_FRAME_INTERVALas required for video encoders, and WebCodecs has noGOP-length knob at all, so a genuine "leave it to the platform" case would have had no
consistent meaning. The native layer therefore always receives a concrete value, and each
platform applies it unconditionally.
This is not a breaking change: existing code that constructs
VideoEncoderOptionswithoutsetting the property keeps the same one-second interval it had before.
Each platform applies the value through its own knob:
kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration. Also stored on theencoder input so the
kVTInvalidSessionErrsession-recreation path preserves it.KEY_I_FRAME_INTERVAL, now set as a float (accepted since API 25, andmin_apiis 26) so sub-second intervals are expressible.CODECAPI_AVEncMPVGOPSize, converted from seconds to frames using theframe rate hint. Applied through a new post-activation hook on
Transform::new, best-effortso an encoder that does not implement the property still activates.
-force_key_framesexpression.On the Rust side
VideoEncoderOptions::idr_interval_secondsreturns a plainf32with nodefault implementation, so an implementor that omits it fails to compile rather than silently
falling back.
RealtimeEncodingOptions.Defaultsets one second explicitly, since that is InstantReplay's ownchoice rather than something to inherit silently from UniEnc.
Note on
BoundedEncodedFrameBufferGetFramesForDurationcan only start a segment at a keyframe, and returns nothing when thebuffer holds none. Raising the IDR interval beyond the retained duration therefore produces an
empty export. Eviction is also per-frame rather than per-GOP, so frames preceding the oldest
surviving keyframe are retained but unusable, and a longer interval leaves a larger share of the
memory budget unusable. This is documented in
<remarks>rather than clamped, since clampingwould be a behavioural change.
Testing
cargo check -p unienc_cpasses onaarch64-apple-darwin,aarch64-apple-ios,aarch64-linux-android,x86_64-pc-windows-msvc,x86_64-unknown-linux-gnuandwasm32-unknown-unknown.cargo check -p unienc --testspasses,cargo fmt --checkis clean,and
dotnet build -c Releasesucceeds forUniEnc(all three target frameworks) andUniEnc.Example.The Media Foundation and FFmpeg paths are verified by compilation and review only — no Windows
or Linux runtime was available.
Before merging
The native binaries under
Packages/jp.co.cyberagent.instant-replay/UniEnc/Plugins/must berebuilt via the
build-unienc.ymlworkflow (manual dispatch), sinceVideoEncoderOptionsNativegrew from 16 to 20 bytes and the managed and native sides must be updated together.