Wavbrro integration #66
Conversation
| let compressed_data = compress_data(&data, arguments); | ||
|
|
||
| //write | ||
| file_path.set_extension("bro"); |
There was a problem hiding this comment.
is bro supposed to indicate compressed while wbro indicates uncompressed?
Can we pick a name that indicates this better?
Maybe .compressed.bro/.uncompressed.bro or .bro.compressed/.bro.uncompressed?
There was a problem hiding this comment.
Naming is the worst part of this project. BRO is compressed, WBRO is because it is a WAV format adaptation for BRO. But any and all suggestions around naming are very very welcome!
| @@ -57,6 +57,14 @@ impl WavBrro { | |||
| self.chunks.push(Vec::with_capacity(MAX_CHUNK_SIZE)); | |||
| } | |||
|
|
|||
| // Receives a slice of f64 and writes in it's internal structure | |||
| fn load_slice(&mut self, data: &[f64]) { | |||
There was a problem hiding this comment.
This should be a constructor instead, dealing with mutable state is tricky:
fn from_slice(data: &[f64) -> SelfThere was a problem hiding this comment.
Yes, I agree. I found that and was waiting for your feedback. I'll move this into a constructor.
There was a problem hiding this comment.
As we discussed at the meeting with Carlos, WavBrro and bro_reader now have different behaviors for files of the wrong type: WavBrro panics, while bro_reader skips them. This needs to be fixed in the next pull request (PR). I can take care of it.
Otherwise, I agree with the previous feedback.
Also, it now seems to me that we have a mess with naming conventions: 'bro_reader' uses snake_case, while 'WavBrro' uses PascalCase.
This makes sense since WavBrro is OOP, but bro_reader is not. So, it seems to me that in the next PR, it might be worth rewriting bro_reader in an OOP style?
This PR integrates the WAVBRRO format into the compressor.
Also refactored the integration tests to now run against the new format.
As expected, the nasty decompression bug to the wav format is gone with the new format.