audio icon indicating copy to clipboard operation
audio copied to clipboard

Basic reflect types for package consumers to use

Open asciifaceman opened this issue 5 years ago • 6 comments

This feature derived from conversation in https://github.com/go-audio/wav/issues/20

I am using reflective types but performing no reflection, there should be no performance difference or compatibility issues, outside of projects conforming to the interface.

The intention for these changes would be for the Write() encoder function to be able to ask the type so it can utilize AsIntBuffer only when required - allowing Write() to accept the buf Interface instead of type so it can accept any type but then silently convert to int before passing it on.

This is meant to be an early step to support any type in write without actually writing any type.

asciifaceman avatar May 04 '19 06:05 asciifaceman

Codecov Report

Merging #19 into master will increase coverage by 10.2%. The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   29.94%   40.14%   +10.2%     
==========================================
  Files           4        4              
  Lines         521      543      +22     
==========================================
+ Hits          156      218      +62     
+ Misses        346      300      -46     
- Partials       19       25       +6
Impacted Files Coverage Δ
float_buffer.go 81.25% <100%> (+0.98%) :arrow_up:
int_buffer.go 40% <100%> (+2.26%) :arrow_up:
pcm_buffer.go 18.85% <25%> (+18.85%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7b2a6ca...eb30983. Read the comment docs.

codecov-io avatar May 04 '19 06:05 codecov-io

I think that's a good approach but it also comes with a lot of challenges. The main one being that any function receiving a buffer might have to do the check and convert the date, so yo might switch back and forth many times within the processing of a file. The other issue is the allocation of lots of temporary buffers (but we can work around that issue by seeing a scratch buffer). I took a similar approach in my initial version: https://github.com/mattetti/audio/blob/master/pcm_buffer.go

The one thing I am not a fan of is to bring an entire package just to get a type lookup table. I think we'd be better off using an enum of constants here.

mattetti avatar May 04 '19 13:05 mattetti

I have to review what I did, but I'm thinking I might be able to implement or use our own iota map (don't have code in front of me right now). I'll check later and see, I used reflect only because it's stdlib and quick but it may make sense to wrap the types anyways

On Sat, May 4, 2019, 6:46 AM Matt Aimonetti [email protected] wrote:

I think that's a good approach but it also comes with a lot of challenges. The main one being that any function receiving a buffer might have to do the check and convert the date, so yo might switch back and forth many times within the processing of a file. The other issue is the allocation of lots of temporary buffers (but we can work around that issue by seeing a scratch buffer). I took a similar approach in my initial version: https://github.com/mattetti/audio/blob/master/pcm_buffer.go

The one thing I am not a fan of is to bring an entire package just to get a type lookup table. I think we'd be better off using an enum of constants here.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/go-audio/audio/pull/19#issuecomment-489328576, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMLNBZYGYRFCQZYAI5NVTLPTWHUZANCNFSM4HKYHBCQ .

asciifaceman avatar May 04 '19 19:05 asciifaceman

DeepEqual on two slices is an O (n) operation so it may bring a strong impact to the performance.

chfanghr avatar May 05 '19 01:05 chfanghr

Deep equal is only used in tests as you can see it was already being used by the maintainers. Otherwise only the reflect kind structure was being used and I'm going to replace it with an enum

On Sat, May 4, 2019, 6:10 PM 方泓睿 [email protected] wrote:

DeepEqual on two slices is an O (n) operation so it may bring a strong impact to the performance.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/go-audio/audio/pull/19#issuecomment-489377830, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMLNB4UB5VILI23UZAYKZ3PTYXZLANCNFSM4HKYHBCQ .

asciifaceman avatar May 05 '19 01:05 asciifaceman

Oh sorry I didn't see that it's a test.

chfanghr avatar May 05 '19 01:05 chfanghr