liamappelbe / wav

Dart package for reading and writing wav files
Apache License 2.0
18 stars 5 forks source link

Raw stream reader #8

Closed jamescrosswell closed 1 year ago

jamescrosswell commented 1 year ago

Hey Liam,

I'm not sure if you're interested in pull requests. I'm also not sure if what I've put together is in your preferred "coding style". I could rejig some things if they rub you the wrong way.

But... basically I was looking for something that would let me ingest raw audio data from a mic or something. It wouldn't be in a file and wouldn't have any header information. So I teased some of the bits of your existing code apart. The example I put together probably illustrates this best.

dart raw.dart guitar.raw pcm16bit 1 44100

I did some things you might not like, but I'm not very opinionated about them. For one thing, I put the code in quite a few different files (rather than one big file). One reason for that is that I was thinking I could take a small subset of the package that I'd actually need to use (the bit that reads the raw audio data) and leave all the rest. Probably it would make sense to actually move this stuff out to a separate raw_audio package or something in that case (which was used by wav). That would need a bit more thinking/work, I think...

It would be pretty easy to merge wav_header_reader.dart, wav_data_reader.dart and positional_byte_reader.dart all back into a single file again...which would arguably be simpler (I wavered on that point).

In any case, let me know if you have any interest in bringing some of these changes into your repo. If not, no worries at all... happy to keep them in my fork.

Thanks heaps for sharing all your original work btw - awesome job!

liamappelbe commented 1 year ago

Hi James,

I'm definitely open to PRs, and I don't have a problem with splitting up the files. But this is a big change, so it's a bit hard to follow in this form. If you want to land the change, it'd be better to do one PR that only moves code around, then one or more PRs that make the changes you're interested in, so that the diffs make more sense.

Let me see if I've understood the change, from reading example/raw.dart. You've got "raw" audio data, which is just a bunch of samples encoded using one of the wav encodings, but no header. So you want to split out the util functions I wrote for reading those samples, and use them independently of the Wav reader? If that's all, then I feel like some of these changes are a bit over complicated, but the overall idea seems sound.

I wonder if this could be solved more simply by moving all the innards of the wav reader to a function like this:

// Converts the input bytes into output samples according to the format.
//
// Output should be a List of Float64Lists of the same size. The number of
// Float64Lists determines the number of channels. The length of the
// Float64Lists determines how many samples will be read.
//
// Returns how many *bytes* were read from the input, or -1 if the input
// was too small.
int readWavSamples(Uint8List input, WavFormat format, List<Float64List> output);

This would support your example, and writing the function in this kinda weird way should also let you support streaming efficiently (but feel free to suggest other APIs). Constructing a separate WavHeader object seems a bit clunky (the reader code doesn't need to know the samples per second). For symmetry it would probably also make sense to pull out the sample writer code similarly: int writeWavSamples(List<Float64List> input, WavFormat format, Uint8List output)

jamescrosswell commented 1 year ago

Hi @liamappelbe ,

Thanks for taking the time to reply... apologies for the delay - I'm only really doing Flutter stuff in my weekends and spare time.

If you want to land the change, it'd be better to do one PR that only moves code around, then one or more PRs that make the changes you're interested in, so that the diffs make more sense.

That makes sense. I think what I shared with you before was a bit of a hack and dash POC anyway.

Maybe I can put together a fairly small symmetrical change (consistent code structure across both the read and the write functionality)... push that through to get your comments, until we're happy with it and then increment from there.

Constructing a separate WavHeader object seems a bit clunky (the reader code doesn't need to know the samples per second)

Yes I thought about that when I pulled it out. I think you're right - probably get rid of it.

In any case, I'll put together something a bit smaller and more considered and see if we can land on something that doesn't overly complicate the rest of the code for the more common use cases.