Skip to content
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

Reason for using sized integer types in StreamInfo and frame.Header #42

Open
BenWhetton opened this issue Dec 3, 2020 · 2 comments
Open
Labels

Comments

@BenWhetton
Copy link

Hi there! I've been playing around with a small audio streaming project in go using this library. Thanks for the great work :)

I was wondering why you chose to use unsigned and sized integer types for all of the values in StreamInfo? Did you find that this provided a measurable performance benefit?
My expectation would be that using types other than int would provide little to no performance benefits on a 32bit or 64bit architecture. The go documentation recommends generally using int unless there is a specific reason not to.

It makes working with the library quite tedious because of the high number of conversions between integer types required when performing logic or calculations with the StreamInfo and frame.Header fields. One of the first things I have done is write my own equivalent type with ints. I have a strong aversion to boilerplate unless it serves a purpose so it'd be good to understand what drove this decision.

I'd be happy to submit a pull request if you think that this would be a good change. However it would be a breaking change so perhaps best bundled with other breaking changes for a future major release.

@wader
Copy link

wader commented Dec 3, 2020

I would guess the reason is to match how big the different fields are in the standard? the option with int would be to silently truncate or give som error on to big values i think?

@mewmew
Copy link
Member

mewmew commented Dec 4, 2020

Hi @BenWhetton,

Hi there! I've been playing around with a small audio streaming project in go using this library. Thanks for the great work :)

Happy to hear you've been enjoying the library. We sure had a lot of fun writing the FLAC decoder :)

As for the API, some parts are starting to show its age. However, it should still be a rather faithful decoder matching the official FLAC specification.

I would guess the reason is to match how big the different fields are in the standard? the option with int would be to silently truncate or give som error on to big values i think?

@wader yes, this was along the reasoning that my brother @karlek and I had when we designed the API.

As @wader pointed out, the API is mostly designed to match the spec, rather than being the most user friendly. I agree with you that it is definitely easier to be working with int types where applicable.

The intention is to improve the user friendliness of the API in v2.0 (see #33). In particular, the intention is to split the mewkiz/flac package into a low-level package that is faithful to the official FLAC specification and matches it with close precision. Then a high-level package will be created on top of the low-level package, which will be the interface for most users. The high-level API would likely use int types rather than e.g. uint32.

As you can see in #33. the specifics have not yet been fully detailed. E.g. what would the high-level API look like? The high-level flac package should definitely intend to become the package used by users and other libraries; specifically other audio and music decoding libraries such as faiface/beep/flac, pipelined/flac, azul3d.org/engine/audio/flac, etc.

Feel free to propose a proof of concept API in #33 so we can share a discussion of what it may look like.

Wish you all the best and a good winter ahead.

Cheers,
mewmew & karlek

@mewmew mewmew added the v2.0 label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants