Added Encoder And Test for Decoder and Data Converter #11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, I didn't carefully read the original implementation and how zaudio bridges the function, created a bunch of mess. With a bit more studies, I think I have more confidence on porting Decoder Type into zaudio, starting with the unimplemented type for the Decoder so that I don't get lost in the midway of creating the Decoder type.
The coding part of DataConverter is now done, but this will require testing; nonetheless, now we have the DataConverter and Resampler.Config, building the decoder should be less difficult. I will test all the type once I have done decoder.
There is nothing interesting in this commit because I am only half way through the Decoder type.
The bridge has been finished for Decoder, but since the zaudio doesn't handle the wchar_t type from the original C implementation, I am not going to port any functions that involve the type unless there are a better one size fits all solutions for all other functions requiring wchar_t as well.
All of the coding related to the decoder is done, but clearly, there will be many errors in the code especially the pointer which I have missed out a bunch of optionals. Thus, I will do a series of testing to ensure everything works fine, and after all, we will finally able to load ourselves some samples at the low level using zaudio.
The library is finally successfully compiled, but clearly this is not enough because we need to test if the library really works in practice; thus, we are going to do some test to ensure the correct of the decoders.
There are many more test to do, but I can't do this on the zaudio level where there is no way to place the callback function within the test block. I will open up a separated project to conduct a series of testing such that to ensure the functions working flawlessly and acting as an example code to overcome with the lacking low-level example for zaudio.
This commit addresses the inconsistent convention for channel_map input parameter for the decoder type, and handle a case where some operation requiring to passing the decoder as datasource for other functions. The decoder is now confirmed to fetch and decoder mp3 successfully in a separated project, but more test are required to be conducted, not to mention that the pull request will be done after zig 0.15.1 has properly been merged.
The original function that involving channel_map has an inconsistent input type comparing with other existing function. In this version, I have replaced the two parameters channel_map and channel_map_cap with a slice for my decoder type. All of the implemented functions for decoder are tested and have worked properly; thus, the next task will be testing about the data converter and add the remaining function I missed from the previous commits.
After the test, it turns out the config init Functions are missing in the type, so I have added it such that we can have a more convenient way to declare the converter.
The GitHub review does reflect the inconsistency of my code, with comparing the existing code from the library. Thus, I have made the change according to the copilot review such that to align the coding standard and format of the existing codebase.
Without encoder, it won't be an complete audio application because we need to export our music after all the effort on creating the tracks. This pr will address the missing encoder type, and the missing tests for decoder which is not possible to be tested on its own due to the requirement of external audio files.
Encoder is more complicated than I thought because there is no init_from_memory variant for the encoder; as a result, I had to manually write the on_write and on_seek function just to export the wave file into a buffer for validation; otherwise, testing encoder will be impossible without generating files. The upcoming task is to continue the test case for the decoder.
Finally, thanks to the encoder type, we have some test coverage to the decoder as well, and it should includes most of the commonly used functions.
Since Decoder and Encoder is similar, if the custom onWrite and onSeek function requires user_data, the Decoder.create() function might also need the user_data because it also have its own callback for decoding files.
I just found that it is not possible to retrieve Writer.Interface.end for linux, and it will return an unknown value that break the buffer writer; thus, for the time being, let me replace that with an in-house implementation just to support the seek behavior for miniaudio.
|
Alright, seems the problem was not from the library, but the Testing type However, although this works for the test and this is the only solution I can get around with the limitation of miniaudio while zig's lazy evaluation should not include this part into the final product, I don't like having a type this large specifically for unit test; thus, I will try to find a better solution from this. |
It turns out @memmove can do the string copy as long as I have slice the buffer with the correct index, and this is not java which I don't need to build a setter to set the cursor, so I have removed the seek function and access the cursor directly, to reduce the amount of unnecessary code.
There was a problem hiding this comment.
Pull Request Overview
This PR adds encoder support to the zaudio library and implements comprehensive testing for encoder, decoder, and data converter functionality. The implementation enables exporting audio to WAV format and provides a complete encode-decode roundtrip test.
Key changes:
- Added complete Encoder API with memory-based, VFS-based, and file-based creation methods
- Modified Decoder.readPCMFrames to return frames read count directly instead of requiring output parameter
- Added extensive test coverage for DataConverter, Encoder, and Decoder with roundtrip validation
- Enabled encoding support by removing MA_NO_ENCODING compilation flag
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/zaudio.zig | Added complete Encoder API, updated Decoder.readPCMFrames signature, and comprehensive tests |
| src/zaudio.c | Implemented C wrapper functions for encoder operations and user data access |
| build.zig | Removed MA_NO_ENCODING flag to enable encoding functionality |
| README.md | Updated documentation with encoder support and low-level API examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/zaudio.zig
Outdated
| }; | ||
|
|
||
| pub const getUserData = zaudioDecoderGetUserData; | ||
| extern fn zaudioDecoderGetUserData(device: *const Encoder) ?*anyopaque; |
There was a problem hiding this comment.
The extern function parameter type should be *const Decoder not *const Encoder since this is in the Decoder section and should match the C implementation.
Suggested change
| extern fn zaudioDecoderGetUserData(device: *const Encoder) ?*anyopaque; | |
| extern fn zaudioDecoderGetUserData(device: *const Decoder) ?*anyopaque; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Oh no, this is a dumb mistake, let me clear this tmr
src/zaudio.c
Outdated
| ){ | ||
| assert(file_path != NULL && config != NULL && out_handle != NULL); | ||
| *out_handle = s_mem.onMalloc(sizeof(ma_encoder), s_mem.pUserData); | ||
| ma_result res = ma_encoder_init_file(file_path, config, out_handle); |
There was a problem hiding this comment.
The function call should pass *out_handle as the last parameter instead of out_handle to match the expected ma_encoder* type, similar to the other encoder creation functions.
Suggested change
| ma_result res = ma_encoder_init_file(file_path, config, out_handle); | |
| ma_result res = ma_encoder_init_file(file_path, config, *out_handle); |
Copilot uses AI. Check for mistakes.
|
Done, the aforementioned error has been corrected. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's me again! This time, as mentioned in the decoder issue, I am going to port the encoder such that not only we can use the library to export the audio into
.wavfile, but it can also make testing decoder possible, and now I have a working solution.Unfortunately, because of the limitation and the design choice of the miniaudio library, testing the encoder requires to declare two additional testing functions
testing_on_writeandtesting_on_seek, and the testing typeTestingEncodedStoragebecause there is nocreateFromMemoryvariant for Encoder while the onSeek function is used in the miniaudio internally for changing the cursor location during writing the target buffer. (which makes the use of ArrayList impossible)In exception to the function that require external files, the test should cover most of the common functions used in
Decoder,EncoderandDataConverter; let me know if you would like to have additional tests.Edit: Oh, seems linux is not happy about the update, let me have a look tmr.