-
Notifications
You must be signed in to change notification settings - Fork 9
Add ma_decoder bindings #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ma_decoder bindings #10
Conversation
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.
|
As mentioned previously, most of the decoder functions are not possible to be tested because they need audio files, and it doesn't looks like a good idea to include the testing audio file into the library, so I have written some demos to demonstrate how to use my newly ported Decoder and DataConverter type, along with some tests to ensure everything works fine: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces decoder and data converter types to zaudio, enabling low-level audio processing capabilities. The work adds comprehensive bindings for miniaudio's decoder and data converter functionality, allowing direct access to PCM frame data for custom audio processing applications.
Key changes:
- Added
Decodertype with methods for reading PCM frames, seeking, and format information - Added
DataConvertertype for audio format conversion and resampling - Added supporting enums
DitherModeandEncodingFormat
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/zaudio.zig | Adds Decoder and DataConverter opaque types with comprehensive method bindings |
| src/zaudio.c | Implements C wrapper functions for decoder and data converter creation/destruction |
| README.md | Updates feature list to include Decoder and DataConverter types |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Re error handling in C callbacks, there's indeed no way to emit Zig errors and callback implementations should handle all possible errors internally. Logging out errors that occur in callbacks is certainly reasonable. |
|
Thanks, for that case, I will note that down into my example, and for the time being, let me have a look if I need to correct something based on the copilot review. |
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.
|
The suggestion from the Copilot is valid because after comparing the existing code, they are actually some coding and naming mistake. With the latest update, I have updated the code and comments such that the naming conventions and the format aligns to the existing code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Nice work. Just some nitpicks about overly-verbose comments.
src/zaudio.zig
Outdated
| // these functions were originally located under vfs, but they seems to have no correlation to the type, | ||
| // while decoder require such type in order to make it work, so I will temporary locate these functions in here: | ||
| pub const readProc = *const fn (user_data: ?*anyopaque, buffer_out: ?*anyopaque, bytes_to_read: usize, bytes_read: *usize) callconv(.c) Result; | ||
| pub const seekProc = *const fn (user_data: ?*anyopaque, offset: i64, origin: Vfs.SeekOrigin) callconv(.c) Result; | ||
| pub const tellProc = *const fn (user_data: ?*anyopaque, cursor: ?*i64) callconv(.c) Result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, fine to move these. Let's delete the comments.
src/zaudio.zig
Outdated
| // here are the init functions, but after observed other examples | ||
| // I will skip the _w variant until there is a solution to handle wchar_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments not useful beyond code review. Please delete.
src/zaudio.zig
Outdated
| } | ||
| extern fn zaudioDecoderCreateFromFile(file_path: [*:0]const u8, config: *const Config, out_handle: ?*?*Decoder) Result; | ||
|
|
||
| // The remaing related functions for manipulate the samples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear comment, please delete.
|
Re tests: it would be nice to increase coverage but we can follow up after this PR. |
|
Thanks! I have removed those comments. For the testing, I do have some ideas though: Testing Data Converter should be simple since it only requires an anyopaque type of an array, so we could construct a [256]u8 array containing upward saw wave to these the conversion process. However, testing the decoder without the audio file is a bit more complicated since I tried to feed a raw sample into the decoder but it didn't work because it only support either The two solutions above should eliminate the use of sample files so that we can fit the test within a single test block. |
As mentioned in this issues, I was working on the decoder type so that we can process audio at low level which is useful for custom decoders or more complicated audio applications due to the ability of access the output buffer directly with the function "readPCMFrames()" suggested by the official miniaudio documentation. My Resampler_Decoder_Prototype branch has a working prototype currently.
The current version is subjected to change because I have some questions related to the my current implementation and the callback function:
Since the current callback function from device use
callconv(.c) voidas return type which it doesn't support the zig error, but due to the current coding convention to handle the Result type:This makes decoder has to explicitly handle the error in the callback function like shown:
I tried to change the return type of the callback function as
!void, and trying to cast the function with pointer cast when I am creating the device like shown:Despite the lack of compile error, the device can't trigger the callback at run time. Should we keep the explicit error handling in the callback function, or a better ways to handle error inside such functions?
In addition, since testing the decoder requires sample files to begin with, besides a separate project for testing the function externally, what kind of tests can be included within the library?