Add render-waveforms CLI#249
Conversation
565a0ae to
87f30d5
Compare
|
Hi Robin, thanks a lot! I haven't digged much into anlz yet so it's very convenient. Haven't read the changes yet but at first glance it seems weird to have a commit that touches the changelog. No one should ever have the need to manually edit the changelog, as it's auto-generated from the commit summaries that follow the conventions of the project. |
Extract the waveform SVG renderer into a dedicated module, wire a new render-waveforms subcommand, extend ANLZ waveform parsing for the renderer, migrate SVG emission to xmlwriter, and fix PWV2 tiny-preview decoding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
87f30d5 to
0a5e973
Compare
|
That was an over-zealous Copilot agent trying to update the changelog. I've tidied up the branch. |
|
Cool, thanks. Something else that I'd be very grateful for is if you would use conventional commit summary format (as specified in the contribution guide) so the git hook is able to automatically update the changelog. In your last PR, I had to edit the commit summaries myself because of this. To facilitate that task and also automatically run clippy and other checks at commit time, you might want to install the pre-commit hooks (again, this is explained in the contribution guide). Again, thanks for the contribution and I'll get in touch when I'm able to properly review the code. |
acrilique
left a comment
There was a problem hiding this comment.
Thank you so much! I left some comments and was able to test. I do see rendering to SVG as a cool feature for the cli
| serde = { version = "1.0", features = ["derive"] } | ||
| chrono = "0.4" | ||
| fallible-iterator = "0.3.0" | ||
| xmlwriter = "0.1" |
There was a problem hiding this comment.
I see no need for this as we already have quick-xml as a dependency and it can achieve the same task with code that is very similar
| #[arg(long)] | ||
| no_ext: bool, | ||
| /// Skip reading the sibling `.2EX` file. | ||
| #[arg(long)] | ||
| no_2ex: bool, |
There was a problem hiding this comment.
Maybe for completeness we could add a no_dat argument?
| WaveformPreview, | ||
| WaveformBluePreview, | ||
| /// Smaller version of the fixed-width monochrome preview of the track waveform (for the | ||
| /// CDJ-900). | ||
| #[brw(magic = b"PWV2")] | ||
| TinyWaveformPreview, | ||
| WaveformBlueTinyPreview, | ||
| /// Variable-width large monochrome version of the track waveform. | ||
| /// | ||
| /// Used in `.EXT` files. | ||
| #[brw(magic = b"PWV3")] | ||
| WaveformDetail, | ||
| WaveformBlueDetail, | ||
| /// Fixed-width colored preview of the track waveform. | ||
| /// | ||
| /// Used in `.EXT` files. | ||
| #[brw(magic = b"PWV4")] | ||
| WaveformColorPreview, | ||
| WaveformRGBPreview, | ||
| /// Variable-width large colored version of the track waveform. | ||
| /// | ||
| /// Used in `.EXT` files. | ||
| #[brw(magic = b"PWV5")] | ||
| WaveformColorDetail, | ||
| WaveformRGBDetail, | ||
| /// Fixed-width 3-band preview of the track waveform. |
There was a problem hiding this comment.
Why change the names? The previous ones seem better to me. And also for me it makes it a bit harder to find the actual code changes in the PR, so reviewing is harder
| /// Red channel intensity, corresponding to bass. | ||
| pub red_bass: u8, | ||
| /// Green channel intensity, corresponding to mids. | ||
| pub green_mids: u8, | ||
| /// Blue channel intensity, corresponding to highs. | ||
| pub blue_highs: u8, |
There was a problem hiding this comment.
Also here, why put the color (which is only related to the rendering, so it's only tied to the cli app and not to the library) in the name? I think it is not an improvement over what we previously had
| pub const fn red_bass(&self) -> u8 { | ||
| self.red_bass | ||
| } | ||
|
|
||
| /// Green channel intensity, corresponding to mids. | ||
| pub const fn green_mids(&self) -> u8 { | ||
| self.green_mids | ||
| } | ||
|
|
||
| /// Blue channel intensity, corresponding to highs. | ||
| pub const fn blue_highs(&self) -> u8 { | ||
| self.blue_highs | ||
| } | ||
|
|
||
| /// Coarse 5-bit column height. | ||
| pub const fn height(&self) -> u8 { | ||
| self.height | ||
| } | ||
|
|
||
| /// Raw low-order fine-height bits in on-disk order. | ||
| pub const fn low_bits(&self) -> u8 { | ||
| self.low_bits | ||
| } |
There was a problem hiding this comment.
Here you also have the chance to make the PR diff smaller by just sticking to the previous convention, i.e. making the struct fields public
| /// Sound energy in the mid of the frequency range. | ||
| pub energy_mid_third_freq: u8, | ||
| /// Sound energy in the top of the frequency range. | ||
| pub energy_top_third_freq: u8, | ||
| /// Sound energy in the bottom third of the frequency range. | ||
| pub energy_bottom_third_freq: u8, | ||
| /// Low / bass band intensity. | ||
| pub low: u8, | ||
| /// Mid band intensity. | ||
| pub mid: u8, | ||
| /// High band intensity. | ||
| pub high: u8, |
There was a problem hiding this comment.
According to the docs in the link above, "The three-band waveform preview entries are one-byte height values representing the mid-range, high, and low frequencies, in that order."
| pub struct Waveform3BandDetailColumn { | ||
| /// Sound energy in the mid of the frequency range. | ||
| pub energy_mid_third_freq: u8, | ||
| /// Sound energy in the top of the frequency range. | ||
| pub energy_top_third_freq: u8, | ||
| /// Sound energy in the bottom third of the frequency range. | ||
| pub energy_bottom_third_freq: u8, | ||
| /// Low / bass band intensity. | ||
| pub low: u8, | ||
| /// Mid band intensity. | ||
| pub mid: u8, | ||
| /// High band intensity. | ||
| pub high: u8, |
| /// Per-band gain calibration for the 3-band player waveform. | ||
| /// | ||
| /// Used in `.2EX` files. | ||
| #[binrw] | ||
| #[derive(Debug, PartialEq, Eq, Clone, Copy)] | ||
| #[br(import(header: Header))] | ||
| pub struct Waveform3BandCalibration { | ||
| /// Observed constant header field (`0x0000` in all current fixtures). | ||
| #[br(temp)] | ||
| #[br(assert(header.remaining_size() == 2))] | ||
| #[br(assert(unknown == 0))] | ||
| #[bw(calc = 0u16)] | ||
| pub unknown: u16, | ||
| /// Gain applied to the low / blue waveform band. | ||
| #[br(assert(header.content_size() == 6))] | ||
| pub low_gain: u16, | ||
| /// Gain applied to the mid / yellow waveform band. | ||
| pub mid_gain: u16, | ||
| /// Gain applied to the high / white waveform band. | ||
| pub high_gain: u16, | ||
| } | ||
|
|
There was a problem hiding this comment.
This one isn't documented yet, right?
|
Hey @RobinMcCorkell , just pinging in case you missed my review. |
Summary
Validation