Skip to content

refactor: eliminate dead code and extract common patterns#27

Open
flexiondotorg wants to merge 7 commits intomainfrom
refactor
Open

refactor: eliminate dead code and extract common patterns#27
flexiondotorg wants to merge 7 commits intomainfrom
refactor

Conversation

@flexiondotorg
Copy link
Contributor

Summary

This PR addresses three major refactoring objectives: modernising stdlib usage patterns, removing 14 instances of dead code that accumulated during development, and extracting repeated logic into shared helper functions across the codebase. The changes reduce code complexity, improve maintainability, and eliminate unused exports that cluttered the API surface.

Changes

  • Replace unsafe.Pointer float32 conversions with math.Float32bits and math.Float32frombits from the standard library
  • Use cmplx.Abs for complex number magnitude calculations instead of manual computation
  • Remove dead code: SharedAudioBuffer (390 lines), ApplyHanning, WriteFrame, convertRGBToYUV, GetEncoderStatus, Frame.Release, AudioFlushProgress, 10 CLI style functions, and minor unused exports (net -1,306 lines)
  • Extract audio buffer logic into buffer.go: FillFFTBuffer and ReadNextFrame helpers eliminate duplication in FFT analysis
  • Extract FFmpeg boilerplate into ffmpeg_common.go: openAudioFormatCtx consolidates format context initialisation across decoder and analyser
  • Extract renderer helpers into text.go: loadImageData, newTextDrawer, measureStringWidth, measureTextBounds eliminate duplication in asset loading and text measurement
  • Extract standalone helpers: outputChannels(), writeRow/writeBarRow closures, and mustAnalyze test helper

Testing

  • Existing test suite validates refactored code paths and buffer operations
  • New tests in buffer_test.go cover extracted buffer helpers
  • Audio analyser tests updated to use refactored patterns
  • CI/CD pipeline confirms no functional regressions

…h stdlib

- Replace 4 unsafe.Pointer float32 casts in ffmpeg_decoder.go with
  math.Float32frombits()
- Replace 3 unsafe.Pointer float32 casts in encoder.go with
  math.Float32bits() + binary.LittleEndian
- Remove unnecessary unsafe import from ffmpeg_decoder.go
- Improve code clarity by leveraging stable stdlib functions available
  since Go 1.0

Signed-off-by: Martin Wimpress <code@wimpress.io>
Replace manual magnitude calculations with math/cmplx.Abs:
- fft.go: Replace sqrt(real²+imag²) with cmplx.Abs
- analyzer.go: Replace sqrt(real²+imag²) with cmplx.Abs
- Add math/cmplx imports where needed

Improves clarity and uses stdlib functionality available since Go 1.0.

Signed-off-by: Martin Wimpress <code@wimpress.io>
- Delete unused file internal/audio/shared_buffer.go (240 lines)
- Remove unused functions: ApplyHanning, SeekToSample, NumChannels,
  WriteFrame, convertRGBToYUV, EncoderDescription,
  IsHardwareAccelerated, GetVideoFramesEncoded, GetEncoderStatus,
  Frame.Release, AudioFlushProgress, and 10 CLI style functions
- Remove unused imports: "io" from decoder, "strings" from hwaccel,
  "sync" from renderer
- Delete 14 test functions and benchmarks depending on removed code
- Rewrite 6 tests to use live code paths

Signed-off-by: Martin Wimpress <code@wimpress.io>
Extract FillFFTBuffer and ReadNextFrame helper functions from
AnalyzeAudio and runPass2 to eliminate duplicate read-loop logic. Both
functions now call shared implementations in buffer.go with full unit
test coverage.

Signed-off-by: Martin Wimpress <code@wimpress.io>
Create `openAudioFormatCtx` in ffmpeg_common.go to consolidate the
duplicated stream-search logic that appeared in both NewFFmpegDecoder
and GetMetadata. Both callers now invoke the shared function instead of
maintaining identical 7-line loops inline.

Signed-off-by: Martin Wimpress <code@wimpress.io>
…ment

Consolidate two recurring code patterns from the renderer package:

- Extract loadImageData helper to handle filesystem-vs-embed branching
  for both LoadBackgroundImage and loadThumbnailBackground callers
- Extract text drawing helpers (newTextDrawer, measureStringWidth,
  measureTextBounds) from assets.go and thumbnail.go into text.go
- Replace duplicated &font.Drawer{} construction and text measurement
  boilerplate with reusable functions

Signed-off-by: Martin Wimpress <code@wimpress.io>
- Add outputChannels() method to consolidate audio channel defaulting
  logic used in three encoder functions
- Extract writeRow and writeBarRow closures in progress.go to replace
  ten repetitive fmt.Fprintf blocks in completion table rendering
- Add mustAnalyze(t) test helper to replace six instances of duplicated
  AnalyzeAudio setup boilerplate in analyzer_test.go

Signed-off-by: Martin Wimpress <code@wimpress.io>
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 28 files

Confidence score: 3/5

  • There is some merge risk: ReadNextFrame in cmd/jivefire/main.go now allocates per frame inside the render loop, which is a concrete performance regression from the prior no-allocation audio path.
  • The test issue in internal/audio/buffer_test.go lowers confidence because TestReadNextFramePartialAtEOF may pass without proving partial-frame EOF behavior, so future regressions could slip through.
  • Given the moderate severity and high confidence on both findings, this looks manageable but worth addressing before relying on performance-sensitive behavior.
  • Pay close attention to cmd/jivefire/main.go and internal/audio/buffer_test.go - render-loop allocation regressions and incomplete EOF partial-read assertions are the main risk areas.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/audio/buffer_test.go">

<violation number="1" location="internal/audio/buffer_test.go:114">
P2: `TestReadNextFramePartialAtEOF` can pass without ever asserting that a partial frame was returned, so it may miss regressions in EOF partial-read behavior.</violation>
</file>

<file name="cmd/jivefire/main.go">

<violation number="1" location="cmd/jivefire/main.go:547">
P2: `ReadNextFrame` introduces per-frame slice allocations in the render loop, regressing the previous no-allocation audio read path.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

// Drain most of the file
for {
samples, err := ReadNextFrame(reader, 4096)
if errors.Is(err, io.EOF) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: TestReadNextFramePartialAtEOF can pass without ever asserting that a partial frame was returned, so it may miss regressions in EOF partial-read behavior.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/audio/buffer_test.go, line 114:

<comment>`TestReadNextFramePartialAtEOF` can pass without ever asserting that a partial frame was returned, so it may miss regressions in EOF partial-read behavior.</comment>

<file context>
@@ -0,0 +1,156 @@
+	// Drain most of the file
+	for {
+		samples, err := ReadNextFrame(reader, 4096)
+		if errors.Is(err, io.EOF) {
+			break
+		}
</file context>

cli.PrintError(fmt.Sprintf("error reading audio: %v", err))
p.Quit()
return
newSamples, readErr := audio.ReadNextFrame(reader, samplesPerFrame)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: ReadNextFrame introduces per-frame slice allocations in the render loop, regressing the previous no-allocation audio read path.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/jivefire/main.go, line 547:

<comment>`ReadNextFrame` introduces per-frame slice allocations in the render loop, regressing the previous no-allocation audio read path.</comment>

<file context>
@@ -556,31 +544,15 @@ func runPass2(p *tea.Program, inputFile string, outputFile string, channels int,
-				cli.PrintError(fmt.Sprintf("error reading audio: %v", err))
-				p.Quit()
-				return
+		newSamples, readErr := audio.ReadNextFrame(reader, samplesPerFrame)
+		if readErr != nil {
+			if errors.Is(readErr, io.EOF) {
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant