refactor: assorted fixes, clean-ups, improvements and performance tuning#47
Merged
Conversation
…able - Remove Duration and FinalizeTime from RenderComplete (redundant with TotalTime) - Extract borderless table configuration to theme.BorderlessTable() shared helper - Update summary and help tables to use unified table styling - Simplify timing calculation in Pass 2 completion Signed-off-by: Martin Wimpress <code@wimpress.io>
- Reuse audioSamples buffer for initial audio write instead of allocating - Merge bar render and mirror loops in Frame.drawBars for clarity - Remove guard on font face in text overlay (self-guarded method) - Extract maxLineWidth helper to boxwidth_test.go where it is used - Remove redundant timing fields from UI model (overallStartTime, pass1StartTime, completionTime, quitting) and unused height field Signed-off-by: Martin Wimpress <code@wimpress.io>
…ities - Use unsafe.Slice() instead of custom unsafeByteSlice() helper in audio decoding - Replace manual unsafe pointer cast with unsafe.Slice() in encoder sample writing - Replace manual loops with slices.Max() in spectrum rendering - Replace manual min/max finding with slices.Min/Max in sparkline generation Signed-off-by: Martin Wimpress <code@wimpress.io>
The audio reader downmixes all input to mono. For --channels 2, the encoder expects interleaved L,R pairs, but mono samples were being sent unchanged—reinterpreted as interleaved stereo, resulting in half-length, double-speed, desynchronised audio. Duplicate each mono sample into both channels (s, s) when building the encoder buffer for stereo output. Pre-allocate stereoSamples buffer to avoid per-frame allocation, matching the existing buffer discipline. Also: - Add unit tests for internal/yuv RGB→YCbCr conversions (pin fixed-point arithmetic against stdlib color.RGBToYCbCr to catch clamp bugs) - Update justfile test-encoder to exercise --channels 2 and assert output audio duration matches source (catches reinterpreted-buffer regressions) Signed-off-by: Martin Wimpress <code@wimpress.io>
…hot paths Remove allocations from both the FFT analysis and video encode loops by reusing buffers and pooling resources: - FFT: fold real→complex conversion into windowing loop, reuse complex buffer (0 allocs/op, ~19% faster per ProcessChunk) - Audio read: introduce ReadInto(dst) to avoid throwaway slices per frame - Audio decode: decode directly into sampleBuffer tail via slices.Grow - Encoder: pre-allocate and reuse YUV/RGBA frames and video packets (software and NVENC paths) - YUV: replace per-frame goroutine spawning with persistent worker pool For a 30-minute encode: ~108k FFT allocs eliminated, ~54k per-frame frame allocations removed, ~864k goroutine spawns eliminated. Realistic 5-15% total wall-clock improvement on CPU-bound encodes. Output byte-identical verified by TestConversionEquivalence. Signed-off-by: Martin Wimpress <code@wimpress.io>
Extracted helpers and variants to eliminate code duplication and improve maintainability: - Extract expandMonoToStereo helper: stereo channel expansion now lives in one place, reducing bug-fix surface (main.go) - Extract receiveAndWriteAudioPackets helper: consolidate duplicated audio packet drain loops; fixes latent bug where FlushAudioEncoder was silently swallowing write errors (encoder.go) - Add SelectBestEncoderFrom variant: hardware encoder selection now accepts a pre-probed slice, eliminating redundant probe calls on the error path (hwaccel.go, main.go) - Consolidate image path dispatch: custom-vs-default resolution now lives solely in RuntimeConfig.Get*ImagePath methods, removing duplicated logic from renderer call sites (config.go, renderer/assets.go, renderer/thumbnail.go) The FlushAudioEncoder fix means failed audio packet writes during the final flush are no longer silently discarded; they now propagate as errors. Signed-off-by: Martin Wimpress <code@wimpress.io>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 24 files
Confidence score: 3/5
- There is a concrete regression risk in
internal/encoder/encoder.go: worker goroutines are started before fallible setup, and failure paths return without shutting them down. - Because this issue is medium severity (6/10) with high confidence (9/10), it can cause resource/goroutine leaks during initialization errors and may impact stability over time.
- Pay close attention to
internal/encoder/encoder.go- ensure all init-error paths cleanly stop/close started workers before returning.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…s, and eliminate encoder allocations - Fix A/V sync drift for non-44.1 kHz inputs by deriving samplesPerFrame from the file's actual sample rate instead of assuming hardcoded 44100 Hz. Update metadata tracking and config comments to clarify per-frame budget calculation. - Surface non-fatal asset-load warnings (background image, font) after Bubbletea alt screen exits so they remain visible instead of being silently dropped. Add PrintWarning helper to CLI styles. - Eliminate per-iteration alloc/free cycle in encoder.Close() drain loop by reusing the shared e.pkt; consolidate inline channel layout mapping to channelLayoutName helper function. Signed-off-by: Martin Wimpress <code@wimpress.io>
Stop RowPool workers if a later setup step fails, preventing goroutine leaks. Use named return with error guard to clean up resources before returning an error, since the caller only defers Close once Initialize returns successfully. Signed-off-by: Martin Wimpress <code@wimpress.io>
There was a problem hiding this comment.
3 issues found across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…Warnings race - Guard samplesPerFrame divisions against zero/negative rates to prevent divide-by-zero panic in Pass 1 estimate and infinite loop in analyzer - Move assetWarnings from shared slice to RenderComplete message field to eliminate data race between goroutines; caller reads warnings from final model after p.Run() returns Signed-off-by: Martin Wimpress <code@wimpress.io>
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: The PR modifies over 1000 lines across 20+ files touching core logic in audio analysis, encoding, frame rendering, streaming I/O, and UI progress, which carries significant risk of regression even though the AI review found no issues.
Re-trigger cubic
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
1 participant
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.
No description provided.