Conversation
jcavar
force-pushed
the
feature/spectrogram
branch
5 times, most recently
from
3b85663 to
6c60add
Compare
jcavar
force-pushed
the
feature/spectrogram
branch
from
6c60add to
7088df6
Compare
hhrvoic
left a comment
hhrvoic
left a comment
There was a problem hiding this comment.
Looks good :)
Added some minor comments about force unwrapping
| var spectrogramData = [Float]() | ||
|
|
||
| let mono = mixToMono() | ||
| let data = mono.floatChannelData![0] |
There was a problem hiding this comment.
Suggest to have safe access here without force unwrapping and .first() and handle the optional in the way you think it would be the best fit for this method (throwing, returning empty array or something else)
There was a problem hiding this comment.
The problem is that swift-snapshot-testing doesn't have a good way to handle errors. Eventually, anything that we throw out of here, will need to be ignored in the high-level handling code.
I think I would rather embrace our assumptions that these will always be as we expect them and adjust implementation if they turned out to be incorrect for some cases.
| static func spectrogramImage(data: [Float], width: Int, height: Int) -> CGImage { | ||
| let rgbImageFormat = vImage_CGImageFormat( | ||
| bitsPerComponent: 32, | ||
| bitsPerPixel: 32 * 3, | ||
| colorSpace: CGColorSpaceCreateDeviceRGB(), | ||
| bitmapInfo: CGBitmapInfo( | ||
| rawValue: kCGBitmapByteOrder32Host.rawValue | | ||
| CGBitmapInfo.floatComponents.rawValue | | ||
| CGImageAlphaInfo.none.rawValue))! | ||
|
|
||
| /// RGB vImage buffer that contains a vertical representation of the audio spectrogram. | ||
| let redBuffer = vImage.PixelBuffer<vImage.PlanarF>(width: height, height: width) | ||
| let greenBuffer = vImage.PixelBuffer<vImage.PlanarF>(width: height, height: width) | ||
| let blueBuffer = vImage.PixelBuffer<vImage.PlanarF>(width: height, height: width) | ||
| let rgbImageBuffer = vImage.PixelBuffer<vImage.InterleavedFx3>(width: height, height: width) | ||
| var data = data | ||
| data.withUnsafeMutableBufferPointer { | ||
| let planarImageBuffer = vImage.PixelBuffer( | ||
| data: $0.baseAddress!, | ||
| width: height, | ||
| height: width, | ||
| byteCountPerRow: height * MemoryLayout<Float>.stride, | ||
| pixelFormat: vImage.PlanarF.self | ||
| ) | ||
|
|
||
| multidimensionalLookupTable.apply( | ||
| sources: [planarImageBuffer], | ||
| destinations: [redBuffer, greenBuffer, blueBuffer], | ||
| interpolation: .half | ||
| ) | ||
|
|
||
| rgbImageBuffer.interleave( | ||
| planarSourceBuffers: [redBuffer, greenBuffer, blueBuffer] | ||
| ) | ||
| } | ||
| return rgbImageBuffer.makeCGImage(cgImageFormat: rgbImageFormat)! | ||
| } |
There was a problem hiding this comment.
Also here, maybe to think about improving force unwrapping
There was a problem hiding this comment.
Similar to my other comment, I don't feel like we would gain much, since the error that we throw from here would have to be ignored in the upstream code
| // Lookup table resolution: 32 entries provides good color accuracy while keeping memory usage low | ||
| // Higher values (64, 128) would provide smoother gradients but increase memory and initialization time | ||
| // Lower values (16, 8) would be faster but produce visible banding in spectrograms |
| .foregroundColor(.secondary) | ||
| } | ||
|
|
||
| private func formatTime(_ time: Double, totalDuration: Double) -> String { |
There was a problem hiding this comment.
Regarding style, could be static func (extension on String), similar to other extensions below
| private func createBuffer(from samples: [Float]) -> AVAudioPCMBuffer { | ||
| let format = AVAudioFormat(standardFormatWithSampleRate: 32768, channels: 1)! | ||
| let buffer = AVAudioPCMBuffer(pcmFormat: format, frameCapacity: AVAudioFrameCount(samples.count))! | ||
| let channelData = buffer.floatChannelData![0] | ||
| samples.enumerated().forEach { index, sample in | ||
| channelData[index] = sample | ||
| } | ||
| buffer.frameLength = AVAudioFrameCount(samples.count) | ||
| return buffer | ||
| } |
There was a problem hiding this comment.
Can this be throwing func instead of force unwrapping, I can see benefit cause test methods are already throwing and we will not potentially crash the whole suite?
| /// | ||
| /// Values near zero return dark blue, `0.5` returns red, and `1.0` returns full-brightness green. | ||
| @available(macOS 13, iOS 16, *) | ||
| nonisolated(unsafe) var multidimensionalLookupTable: vImage.MultidimensionalLookupTable = { |
There was a problem hiding this comment.
Does this need to be a global var since I see that we use it only in this file?
2 participants
Implement a strategy to generate spectrogram images.
There are probably few more iterations I would like to do on this, but for now I am happy.
I've compared and tuned these spectrograms by comparing to output of sox: