Intro
Today i've read a small article by jfo that was called Makin' wavs with Zig which was about some first-hand experience in Zig, but also about how bugs are not always where you'd expect them.
Looking at the code, i've immediatly saw some things to improve the code to make it more idiomatic. Technically, it only had one flaw here that might also be just lazyness for the sake of learning, so no shame on jfo here.
I've asked jfo if they would be cool with me writing a response on how to improve the code to be more Zig style, and they said yes. So here it is!
The original code
The code is rendering a small wave file that plays a 440 Hz sine wave for three seconds. Nothing special, but actually a quite good task to get a grasp of arithmetics and I/O in a new language:
const std = @import("std");
const File = std.fs.File;
const sin = std.math.sin;
const SAMPLE_RATE: u32 = 44100;
const CHANNELS: u32 = 1;
const HEADER_SIZE: u32 = 36;
const SUBCHUNK1_SIZE: u32 = 16;
const AUDIO_FORMAT: u16 = 1;
const BIT_DEPTH: u32 = 8;
const BYTE_SIZE: u32 = 8;
const PI: f64 = 3.14159265358979323846264338327950288;
fn write_u16(n: u16, file: File) !void {
const arr = [_]u8{ @truncate(u8, n), @truncate(u8, n >> 8) };
_ = try file.write(arr[0..]);
}
fn write_u32(n: u32, file: File) !void {
const arr = [_]u8{ @truncate(u8, n), @truncate(u8, n >> 8), @truncate(u8, n >> 16), @truncate(u8, n >> 24) };
_ = try file.write(arr[0..]);
}
fn write_header(seconds: u32, file: File) !void {
const numsamples: u32 = SAMPLE_RATE * seconds;
_ = try file.write("RIFF");
try write_u32(HEADER_SIZE + numsamples, file);
_ = try file.write("WAVEfmt ");
try write_u32(SUBCHUNK1_SIZE, file);
try write_u16(AUDIO_FORMAT, file);
try write_u16(@truncate(u16, CHANNELS), file);
try write_u32(SAMPLE_RATE, file);
try write_u32(SAMPLE_RATE * CHANNELS * (BIT_DEPTH / BYTE_SIZE), file);
try write_u16(@truncate(u16, (CHANNELS * (BIT_DEPTH / BYTE_SIZE))), file);
try write_u16(@truncate(u16, BIT_DEPTH), file);
_ = try file.write("data");
try write_u32(numsamples * CHANNELS * (BIT_DEPTH / BYTE_SIZE), file);
}
fn sine_wave(seconds: u32, file: File, freq: f64) !void {
var idx: u32 = 0;
while (idx < seconds * SAMPLE_RATE) {
const sample = ((sin(((@intToFloat(f64, idx) * 2.0 * PI) / @intToFloat(f64, SAMPLE_RATE)) * freq) + 1.0) / 2.0) * 255.0;
const arr = [_]u8{@floatToInt(u8, sample)};
_ = try file.write(arr[0..]);
idx += 1;
}
}
pub fn main() !void {
const cwd = std.fs.cwd();
var file = try cwd.createFile("sine.wav", .{});
try write_header(3, file);
try sine_wave(3, file, 440.0);
_ = file.close();
}
Overall i'd say that's pretty solid code, but let's ziggify it!
Improvements
Literal conversion
The first thing that came to my mind when looking at the code was that the constants were typed:
const SAMPLE_RATE: u32 = 44100;
const CHANNELS: u32 = 1;
const HEADER_SIZE: u32 = 36;
const SUBCHUNK1_SIZE: u32 = 16;
const AUDIO_FORMAT: u16 = 1;
const BIT_DEPTH: u32 = 8;
const BYTE_SIZE: u32 = 8;
const PI: f64 = 3.14159265358979323846264338327950288;
This is untypical for Zig code, as we can just store integer and float literals in variables:
-const SAMPLE_RATE: u32 = 44100;
-const CHANNELS: u32 = 1;
-const HEADER_SIZE: u32 = 36;
-const SUBCHUNK1_SIZE: u32 = 16;
-const AUDIO_FORMAT: u16 = 1;
-const BIT_DEPTH: u32 = 8;
-const BYTE_SIZE: u32 = 8;
-const PI: f64 = 3.14159265358979323846264338327950288;
+const SAMPLE_RATE = 44100;
+const CHANNELS = 1;
+const HEADER_SIZE = 36;
+const SUBCHUNK1_SIZE = 16;
+const AUDIO_FORMAT = 1;
+const BIT_DEPTH = 8;
+const BYTE_SIZE = 8;
+const PI = 3.14159265358979323846264338327950288
This now allows us to remove one workaround in the code later:
- const sample = ((sin(((@intToFloat(f64, idx) * 2.0 * PI) / @intToFloat(f64, SAMPLE_RATE)) * freq) + 1.0) / 2.0) * 255.0;
+ const sample = ((sin(((@intToFloat(f64, idx) * 2.0 * PI) / SAMPLE_RATE) * freq) + 1.0) / 2.0) * 255.0;
As you can see, there's no need to use @intToFloat anymore for our SAMPLE_RATE, as a comptime_int type can simply coerce to any integer or floating point type, so we removed a potential panic and runtime lossy cast into a compile time cast.
defer
The next thing was in the main function:
pub fn main() !void {
const cwd = std.fs.cwd();
var file = try cwd.createFile("sine.wav", .{});
+ defer file.close();
+
try write_header(3, file);
try sine_wave(3, file, 440.0);
- _ = file.close();
}
By using defer, we can make sure the file is closed in all cases, even if write_header or sine_wave fail and return earlier. Also close() can't return values in Zig, so we don't have to ignore the return value here. Little bit cleaner, definitly more correct!
Introducing File.Writer
-fn write_u16(n: u16, file: File) !void {
+fn write_u16(n: u16, file: File.Writer) !void {
const arr = [_]u8{ @truncate(u8, n), @truncate(u8, n >> 8) };
try file.writeAll(arr[0..]);
}
-fn write_u32(n: u32, file: File) !void {
+fn write_u32(n: u32, file: File.Writer) !void {
const arr = [_]u8{ @truncate(u8, n), @truncate(u8, n >> 8), @truncate(u8, n >> 16), @truncate(u8, n >> 24) };
try file.writeAll(arr[0..]);
}
-fn write_header(seconds: u32, file: File) !void {
+fn write_header(seconds: u32, file: File.Writer) !void {
const numsamples: u32 = SAMPLE_RATE * seconds;
- _ = try file.write("RIFF");
+ try file.writeAll("RIFF");
try write_u32(HEADER_SIZE + numsamples, file);
- _ = try file.write("WAVEfmt ");
+ try file.writeAll("WAVEfmt ");
try write_u32(SUBCHUNK1_SIZE, file);
try write_u16(AUDIO_FORMAT, file);
try write_u16(@truncate(u16, CHANNELS), file);
@@ -33,16 +33,16 @@ fn write_header(seconds: u32, file: File) !void {
try write_u32(SAMPLE_RATE * CHANNELS * (BIT_DEPTH / BYTE_SIZE), file);
try write_u16(@truncate(u16, (CHANNELS * (BIT_DEPTH / BYTE_SIZE))), file);
try write_u16(@truncate(u16, BIT_DEPTH), file);
- _ = try file.write("data");
+ try file.writeAll("data");
try write_u32(numsamples * CHANNELS * (BIT_DEPTH / BYTE_SIZE), file);
}
-fn sine_wave(seconds: u32, file: File, freq: f64) !void {
+fn sine_wave(seconds: u32, file: File.Writer, freq: f64) !void {
var idx: u32 = 0;
while (idx < seconds * SAMPLE_RATE) {
const sample = ((sin(((@intToFloat(f64, idx) * 2.0 * PI) / SAMPLE_RATE) * freq) + 1.0) / 2.0) * 255.0;
const arr = [_]u8{@floatToInt(u8, sample)};
- _ = try file.write(arr[0..]);
+ try file.writeAll(arr[0..]);
idx += 1;
}
}
@@ -52,6 +52,6 @@ pub fn main() !void {
var file = try cwd.createFile("sine.wav", .{});
defer file.close();
- try write_header(3, file);
- try sine_wave(3, file, 440.0);
+ try write_header(3, file.writer());
+ try sine_wave(3, file.writer(), 440.0);
}
So here we replaced the instances of File in our data rendering code with File.Writer, a instance of the std.io.Writer generic. A Writer (or respectivly Reader for input) is a generic input/output abstraction that implements a lot of common i/o tasks like writing a full sequence of bytes, erroring when a EOF condition happens.
So we replace the calls to file.write with a call to reader.writeAll(). As you can see, writeAll() does not return a length value of the bytes we've written, as the value will call write() as often as necessary to write all bytes, and returning an EndOfStream error when nothing could be written. This makes sure our wave file is actually fully written.
This is the flaw i mentioned earlier, as this is a bug that might be hard to debug later on and is a mistake very often done in other programming languages.
Utilizing File.Writer
A std.io.Writer provides several means to put integers into our data stream. The most commonly used functions in my code are writeIntLittle() and writeByte which can help the code here as well:
-fn write_u16(n: u16, file: File.Writer) !void {
- const arr = [_]u8{ @truncate(u8, n), @truncate(u8, n >> 8) };
- _ = try file.write(arr[0..]);
-}
-
-fn write_u32(n: u32, file: File.Writer) !void {
- const arr = [_]u8{ @truncate(u8, n), @truncate(u8, n >> 8), @truncate(u8, n >> 16), @truncate(u8, n >> 24) };
- _ = try file.write(arr[0..]);
-}
-
fn write_header(seconds: u32, file: File.Writer) !void {
const numsamples: u32 = SAMPLE_RATE * seconds;
try file.writeAll("RIFF");
- try write_u32(HEADER_SIZE + numsamples, file);
+ try file.writeIntLittle(u32, HEADER_SIZE + numsamples);
try file.writeAll("WAVEfmt ");
- try write_u32(SUBCHUNK1_SIZE, file);
- try write_u16(AUDIO_FORMAT, file);
- try write_u16(@truncate(u16, CHANNELS), file);
- try write_u32(SAMPLE_RATE, file);
- try write_u32(SAMPLE_RATE * CHANNELS * (BIT_DEPTH / BYTE_SIZE), file);
- try write_u16(@truncate(u16, (CHANNELS * (BIT_DEPTH / BYTE_SIZE))), file);
- try write_u16(@truncate(u16, BIT_DEPTH), file);
+ try file.writeIntLittle(u32, SUBCHUNK1_SIZE);
+ try file.writeIntLittle(u16, AUDIO_FORMAT);
+ try file.writeIntLittle(u16, @truncate(u16, CHANNELS));
+ try file.writeIntLittle(u32, SAMPLE_RATE);
+ try file.writeIntLittle(u32, SAMPLE_RATE * CHANNELS * (BIT_DEPTH / BYTE_SIZE));
+ try file.writeIntLittle(u16, @truncate(u16, (CHANNELS * (BIT_DEPTH / BYTE_SIZE))));
+ try file.writeIntLittle(u16, @truncate(u16, BIT_DEPTH));
try file.writeAll("data");
- try write_u32(numsamples * CHANNELS * (BIT_DEPTH / BYTE_SIZE), file);
+ try file.writeIntLittle(u32, numsamples * CHANNELS * (BIT_DEPTH / BYTE_SIZE));
}
fn sine_wave(seconds: u32, file: File.Writer, freq: f64) !void {
var idx: u32 = 0;
while (idx < seconds * SAMPLE_RATE) {
const sample = ((sin(((@intToFloat(f64, idx) * 2.0 * PI) / SAMPLE_RATE) * freq) + 1.0) / 2.0) * 255.0;
- const arr = [_]u8{@floatToInt(u8, sample)};
- try file.writeAll(arr[0..]);
+ try file.writeByte(@floatToInt(u8, sample));
idx += 1;
}
}
The functions write_u32 and write_u16 were implementing the writeIntLittle function for the types u16 and u32 manually, so we can just delete them and replace all calls to them with writeIntLittle. Removes some code, which is good, and also makes the intent of writing a little-endian integer more clear.
There are also three other integer-writing functions:
-
writeIntBig, which will write a big-endian integer -
writeIntNative, which will just write the integer as it is represented in memory - and
writeIntForeign, which will swap the bytes before writing them.
Usually, when writing formats and protocoles, writeIntBig and writeIntLittle should be preferred over just dumping the memory of structures into a file as you make sure that the data is actually loadable on a machine with different endianess.
Killing a silent killer
The code uses a quite "dangerous" function: @truncate. This will always cut excess bits off an integer without checking if those bits were 0. This means it's a lossy cast without safety checks. Sometimes, this is a wanted effect, but often, it's better to use @intCast to narrow integers, as this uses runtime safety.
In our case, we can do even better:
try file.writeAll("WAVEfmt ");
try file.writeIntLittle(u32, SUBCHUNK1_SIZE);
try file.writeIntLittle(u16, AUDIO_FORMAT);
- try file.writeIntLittle(u16, @truncate(u16, CHANNELS));
+ try file.writeIntLittle(u16, CHANNELS);
try file.writeIntLittle(u32, SAMPLE_RATE);
try file.writeIntLittle(u32, SAMPLE_RATE * CHANNELS * (BIT_DEPTH / BYTE_SIZE));
- try file.writeIntLittle(u16, @truncate(u16, (CNNELS * (BIT_DEPTH / BYTE_SIZE))));
- try file.writeIntLittle(u16, @truncate(u16, BIT_DEPTH));
+ try file.writeIntLittle(u16, (CHANNELS * (BIT_DEPTH / BYTE_SIZE)));
+ try file.writeIntLittle(u16, BIT_DEPTH);
try file.writeAll("data");
try file.writeIntLittle(u32, numsamples * CHANNELS * (BIT_DEPTH / BYTE_SIZE));
Wait?! Why don't we cast these integers at all? Remember the section above where we removed the types from our constants? This made the constants of type comptime_int, which we can perform computations at compile time with.
So writing 8 / 2 in Zig is the same as writing 4, the compiler backend will never see a division at all. Thus, we can also use computations with our constants and the Zig compiler will handle them the same way as if we'd have the computed number as text. And assigning a number that fits in our integer doesn't need any coercion, casting or truncating at all.
This will also have another very nice benefit: If, for example, (CHANNELS * (BIT_DEPTH / BYTE_SIZE)) will ever be out of range for a u16, we'll get a compile error.
Before this change, if we would've used 256 channels and a bit depth of 256, with a byte size of 1, we would've written a 0 there. Now we get a compile error \o/.
Performance tuning
This is a very unintuitive optimization and there are discussions in the community about the behaviour and intent of these functions:
- const sample = ((sin(((@intToFloat(f64, idx) * 2.0 * PI) / SAMPLE_RATE) * freq) + 1.0) / 2.0) * 255.0;
+ const sample = ((@sin((freq * @intToFloat(f64, idx) * (2.0 * PI / @as(comptime_float, SAMPLE_RATE)))) + 1.0) / 2.0) * 255.0;
Huh, what's that? Why do we replace std.math.sin with the builtin @sin?
The builtin is actually the cooler variant. It can be lowered to a hardware instruction, while std.math.sin is always a software implementation.
This might also answer this question from the original article:
Once I fixed the issue, first with the bytes writing correctly (which despite not being identical to the rust version was perfectly fine and sounded fine!) and [...]
As the software implementation of the stdlib might yield slightly different results as the hardware impl. sin isn't exact science in floating point world anyways.
Ziggify the loop!
This is a tiny nitpick about the loop iterator variable:
- while (idx < seconds * SAMPLE_RATE) {
+ while (idx < seconds * SAMPLE_RATE) : (idx += 1) {
const sample = ((@sin((freq * @intToFloat(f64, idx) * (2.0 * PI / @as(comptime_float, SAMPLE_RATE)))) + 1.0) / 2.0) * 255.0;
try file.writeByte(@floatToInt(u8, sample));
- idx += 1;
}
Zigs while loop has a continuation syntax which can be used to trigger effects before the evaluation of the loop condition without putting it to the end of the loop. This is a purely syntactical change, it's just more common to write a loop like this.
Conclusion
This concludes all the functional changes i'd do to the code. Personally, i'd change the function names according to the zig style guide, so using sine and writeHeader, but that's purely personal taste. I've also moved main further to the top so it's easier to find when looking at the code.
But enough talking, let's look at the final code:
const std = @import("std");
const File = std.fs.File;
const SAMPLE_RATE = 44100;
const CHANNELS = 1;
const HEADER_SIZE = 36;
const SUBCHUNK1_SIZE = 16;
const AUDIO_FORMAT = 1;
const BIT_DEPTH = 8;
const BYTE_SIZE = 8;
const PI = 3.14159265358979323846264338327950288;
pub fn main() !void {
const cwd = std.fs.cwd();
var file = try cwd.createFile("sine.wav", .{});
defer file.close();
try writeHeaders(3, file.writer());
try renderSineWave(3, file.writer(), 440.0);
}
fn writeHeaders(seconds: u32, file: File.Writer) !void {
const numsamples: u32 = SAMPLE_RATE * seconds;
try file.writeAll("RIFF");
try file.writeIntLittle(u32, HEADER_SIZE + numsamples);
try file.writeAll("WAVEfmt ");
try file.writeIntLittle(u32, SUBCHUNK1_SIZE);
try file.writeIntLittle(u16, AUDIO_FORMAT);
try file.writeIntLittle(u16, CHANNELS);
try file.writeIntLittle(u32, SAMPLE_RATE);
try file.writeIntLittle(u32, SAMPLE_RATE * CHANNELS * (BIT_DEPTH / BYTE_SIZE));
try file.writeIntLittle(u16, (CHANNELS * (BIT_DEPTH / BYTE_SIZE)));
try file.writeIntLittle(u16, BIT_DEPTH);
try file.writeAll("data");
try file.writeIntLittle(u32, numsamples * CHANNELS * (BIT_DEPTH / BYTE_SIZE));
}
fn renderSineWave(seconds: u32, file: File.Writer, freq: f64) !void {
var idx: u32 = 0;
while (idx < seconds * SAMPLE_RATE) : (idx += 1) {
const sample = ((@sin((freq * @intToFloat(f64, idx) * (2.0 * PI / @as(comptime_float, SAMPLE_RATE)))) + 1.0) / 2.0) * 255.0;
try file.writeByte(@floatToInt(u8, sample));
}
}
And that's it! I hope i could help some people to get started with Zig and might write some more idiomatic Zig code.
And now go, write Zig!
PS.:
As Andrew noted in the comments, using std.math.pi is also the better option than using a self-defined constant. It utilized the full 128 bit float precision from comptime_float, so you're definitly not loosing any precious precision here!
Top comments (7)
One more thing!
std.math.piI don't understand the benefit of leaving off the types of the const's. Fine, now they are comptime; you drop one cast (SAMPLE_RATE; saying a comptime_int can be treated as comptime_float and comptime_int), but then you have to re-add the cast in another place (to make sure the SAMPLE_RATE is indeed being treated as float). It is a bit non-obvious to me why the 2nd place asked for the cast while you could drop it in the first place. Both places have the comptime_int as divisor with the dividend being a float already, aren't they?
Also I like to state expected bit-widths surrounding my I/O - involved data types for binary I/O. If you have a u32 and write a single byte because that's all the value needs for the given value (e.g., write a u32: 127 as u7: 127 aka 1 byte), nobody but your attention will catch it. Similarly with the repetitive explicit type in writeIntLittle: at places you convert, at others you don't. It is non-obvious from the language where you are using a conversion and where you aren't.
Finally, I disagree about the usage of @truncate -- this is used to select a single byte from the multi-byte constant; a perfectly valid use-case that shall happily ignore non-zero bits in the rest of the bit-field. What is your actual problem with the usage of @truncate in this use-case?
TIA for enlightenment.
@truncatewill silently remove upper bits, which means that when you do arithmetic like intry file.writeIntLittle(u16, (CHANNELS * (BIT_DEPTH / BYTE_SIZE)));will just compile, even if it tries to put65536into the var (the program will silently emit 0) whereas without a truncate and no explicit types, the computation would still yield65536, but with this error:Which is unarguably better than having the program compile, and emit invalid files for weeks before someone notices it. Also computing values with
comptime_intis way easier than using fixed types. For example, with(3 * a / 4)you can compute a precise 75% value of that integera, but if you're usinga: u16andais 40000, it will overflow:whereas using a comptime_int value, it will just work.
Thanks for your explanations!
The thing I still don't get is the following: comptime_int will allow you to do computations with results or intermediate results out of scope of the ultimate goal type (in our case, u16). You will notice that at the point where you finally leave comptime_int realm and try to fit the value into u16. You will not notice otherwise.
What is the harm in trying to provoke the error as early as possible instead of deferring it to the latest possible point (when you finally explicitly coerce to u16)?
Isn't it advisable to trigger an error as early as possible, hence decorate your const's with the target type? You've given an example of scaling down by 3/4, what if I need/want to store the intermediate result as well and drive further (comptime) logic with it? In the end, I suppose, things wouldn't compile; I just don't see the benefit of not stating explicitly the domain of values I intend a const (or comptime var, for that matter) to take.
You seem to "sell" this with "I can go temporarily out of domain with comptime_int" where I wouldn't "buy" this exactly because of that "feature".
Again, I think in the end it doesn't matter this way or another, because sooner or later (without @truncate) you'll run into an error - but you say it is more idiomatic to allow temporary values to be out of domain, which is the thing that confuses me.
Just to clarify my thinking: Imho constants should be untyped as their implicit type is actually a integer that only allows a single value (which is the value of the constant) and thus can be stored in 0 bits. I guess when Zig gets the
@Int(min,max)type, a constantxwill just be of the type@Int(x,x), which can be stored in 0 bits, but has a value that can exceed any reasonably storable number. This allows assigning a constantx=7to both a@Int(0,7)as well as to a@Int(7,14)I can imagine why you wouldn't want to go any deeper into the
std.io.Writerinterface, and how it's generally used, but might be good to add a small note that you usually don't accept specific instantiations likeFile.Writer.Great, thanks.
Also softare -> software