separate out a data source and a renderer #163
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors FDWaveformView to separate data source and rendering concerns through protocol-based abstractions. It introduces FDWaveformDataSource for providing audio samples and FDWaveformRenderer for visualization, enabling support for custom data sources beyond file-based audio (e.g., procedural audio, streaming).
Key Changes
- Introduces
FDWaveformDataSourceprotocol withreadSamples(in:)method for flexible sample provision - Adds
FDWaveformRendererprotocol (though not yet integrated into the main view) - Refactors
FDAudioContextto implementFDWaveformDataSourceand addsreadSamplesimplementation - Extracts
FDWaveformTypeenum to separate file with improved documentation - Adds example
SineWaveSourcedemonstrating procedural audio generation
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
Sources/FDWaveformView/FDWaveformView.swift |
Adds dataSource property, refactors to use _dataSource internally, improves code formatting |
Sources/FDWaveformView/FDWaveformDataSource.swift |
New protocol defining data source contract for audio samples |
Sources/FDWaveformView/FDWaveformRenderer.swift |
New protocol for pluggable rendering strategies |
Sources/FDWaveformView/FDWaveformType.swift |
Extracted enum with improved documentation for linear/logarithmic scaling |
Sources/FDWaveformView/FDCoreGraphicsRenderer.swift |
New concrete renderer implementations (line and bar styles) |
Sources/FDWaveformView/FDWaveformRenderOperation.swift |
Updated to accept FDWaveformDataSource, adds sliceDataSource method |
Sources/FDWaveformView/FDAudioContext.swift |
Implements FDWaveformDataSource, adds readSamples method with AVAssetReader |
Example/Sources/SineWaveSource.swift |
New example data source generating procedural sine waves |
Example/Sources/ContentView.swift |
Updated UI to demonstrate data source switching and new features |
Example/Example.xcodeproj/project.pbxproj |
Project configuration updates for new files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var maxInRange: Float = 0 | ||
| let rangeCount = vDSP_Length(endIndex - startIndex) | ||
| vDSP_maxv( | ||
| Array(processedSamples[startIndex..<endIndex]), 1, &maxInRange, rangeCount) |
There was a problem hiding this comment.
Creating a new array with Array(processedSamples[startIndex..<endIndex]) on every iteration is inefficient. Consider using vDSP_maxv with a stride and pointer arithmetic to avoid the allocation, or use vDSP_maxmgv for finding max magnitude.
| Array(processedSamples[startIndex..<endIndex]), 1, &maxInRange, rangeCount) | |
| &processedSamples[startIndex], 1, &maxInRange, rangeCount) |
| waveformType.process(normalizedSamples: &processedSamples) | ||
|
|
||
| // Take absolute values for amplitude | ||
| vDSP_vabs(processedSamples, 1, &processedSamples, 1, vDSP_Length(processedSamples.count)) |
There was a problem hiding this comment.
Incorrect processing order: waveformType.process() expects samples in range 0...1 (absolute amplitude) according to its documentation, but it's being called on the raw signed samples (-1.0 to 1.0) before vDSP_vabs(). The correct order should be: 1) vDSP_vabs() to get absolute values, 2) then waveformType.process() to apply log scaling. This will cause incorrect dB calculations for logarithmic rendering.
| waveformType.process(normalizedSamples: &processedSamples) | |
| // Take absolute values for amplitude | |
| vDSP_vabs(processedSamples, 1, &processedSamples, 1, vDSP_Length(processedSamples.count)) | |
| // Take absolute values for amplitude | |
| vDSP_vabs(processedSamples, 1, &processedSamples, 1, vDSP_Length(processedSamples.count)) | |
| // Process samples through the waveform type (linear/logarithmic) | |
| waveformType.process(normalizedSamples: &processedSamples) |
| import AVFoundation | ||
| import Accelerate | ||
| import MediaPlayer | ||
| // | ||
| // Copyright 2013 - 2017, William Entriken and the FDWaveformView contributors. | ||
| // Copyright William Entriken and the FDWaveformView contributors. | ||
| // | ||
| import UIKit |
There was a problem hiding this comment.
The imports are placed before the copyright comment. By convention, copyright headers should appear before imports. Consider moving the copyright comment to the top of the file.
| import AVFoundation | ||
| import Accelerate | ||
| // | ||
| // Copyright 2013 - 2017, William Entriken and the FDWaveformView contributors. | ||
| // Copyright William Entriken and the FDWaveformView contributors. | ||
| // | ||
| import UIKit |
There was a problem hiding this comment.
The imports are placed before the copyright comment. By convention, copyright headers should appear before imports. Consider moving the copyright comment to the top of the file.
|
|
||
| // Notify delegate | ||
| delegate?.waveformViewDidLoad?(self) |
There was a problem hiding this comment.
Calling waveformViewDidLoad? in resetForNewDataSource() is inconsistent with the audioURL setter behavior. When setting audioURL, the delegate methods waveformViewWillLoad? and waveformViewDidLoad? are called around the async loading operation. For dataSource, there's no loading operation, so calling waveformViewDidLoad? without a corresponding waveformViewWillLoad? call creates an asymmetric API behavior. Consider either removing this call or creating separate delegate methods for data source changes.
| // Notify delegate | |
| delegate?.waveformViewDidLoad?(self) |
| let int16Count = sampleBuffer.count / MemoryLayout<Int16>.size | ||
| var floatSamples = [Float](repeating: 0.0, count: int16Count) | ||
|
|
||
| sampleBuffer.withUnsafeBytes { bytes in | ||
| guard let samples = bytes.bindMemory(to: Int16.self).baseAddress else { return } | ||
| vDSP_vflt16(samples, 1, &floatSamples, 1, vDSP_Length(int16Count)) | ||
| } | ||
|
|
||
| // Normalize to [-1, 1] range | ||
| var scalar: Float = 1.0 / Float(Int16.max) | ||
| vDSP_vsmul(floatSamples, 1, &scalar, &floatSamples, 1, vDSP_Length(int16Count)) |
There was a problem hiding this comment.
The Int16 to Float conversion and normalization could overflow or produce unexpected results if sampleBuffer.count is not a multiple of MemoryLayout<Int16>.size. Consider adding a check to ensure proper alignment and handling of any remainder bytes.
| let visibleSamples = Int(Double(totalSamples) / zoomFactor) | ||
| let center = totalSamples / 2 | ||
| let start = max(0, center - visibleSamples / 2) | ||
| let end = min(totalSamples, start + visibleSamples) | ||
| uiView.zoomSamples = start..<end |
There was a problem hiding this comment.
Potential for creating an empty range: if zoomFactor is very large, visibleSamples could be 0 or very small, potentially creating start >= end. Consider adding a minimum visible sample count (e.g., max(10, Int(Double(totalSamples) / zoomFactor))) to prevent empty ranges.
| private func resetForNewDataSource() { | ||
| waveformImage = nil | ||
| zoomSamples = 0..<self.totalSamples | ||
| highlightedSamples = nil | ||
| inProgressWaveformRenderOperation = nil | ||
| cachedWaveformRenderOperation = nil | ||
| renderForCurrentAssetFailed = false | ||
|
|
||
| setNeedsDisplay() | ||
| setNeedsLayout() | ||
|
|
||
| // Notify delegate | ||
| delegate?.waveformViewDidLoad?(self) | ||
| } |
There was a problem hiding this comment.
The resetForNewDataSource() method duplicates most of the logic from the audioContext didSet observer (lines 165-174). Consider refactoring to use a shared reset method to avoid code duplication and potential inconsistencies.
| } | ||
| } | ||
|
|
||
| /// Legacy initializer for backward compatibility |
There was a problem hiding this comment.
The comment says "Legacy initializer for backward compatibility" but this initializer is not marked as deprecated. Consider adding @available(*, deprecated, message: "Use init(dataSource:imageSize:sampleRange:format:completionHandler:) instead") to guide users toward the new API.
| /// Legacy initializer for backward compatibility | |
| /// Legacy initializer for backward compatibility | |
| @available(*, deprecated, message: "Use init(dataSource:imageSize:sampleRange:format:completionHandler:) instead") |
|
|
||
|
|
||
|
|
||
| // Mark - Private vars |
There was a problem hiding this comment.
Typo in MARK comment: should be // MARK: (all caps) to be recognized by Xcode's navigation system.
| // Mark - Private vars | |
| // MARK: - Private vars |
No description provided.