From 4b96e6fc50304f9989a5a38fd697f7670a74deea Mon Sep 17 00:00:00 2001 From: Hraban Luyat Date: Sun, 14 Aug 2016 13:26:17 +0100 Subject: [PATCH] Fix go pointers to CGo (go 1.6) --- stream.go | 40 ++++++++++++++++++++++++++++++---- stream_test.go | 7 ++---- streams_map.go | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 streams_map.go diff --git a/stream.go b/stream.go index af6cff4..2903d95 100644 --- a/stream.go +++ b/stream.go @@ -13,19 +13,29 @@ import ( // Uses the same signature as Go, no need for proxy int go_readcallback(void *p, unsigned char *buf, int nbytes); + */ import "C" type Stream struct { + id uintptr oggfile *C.OggOpusFile read io.Reader // Preallocated buffer to pass to the reader buf []byte } +var streams = newStreamsMap() + //export go_readcallback func go_readcallback(p unsafe.Pointer, cbuf *C.uchar, cmaxbytes C.int) C.int { - stream := (*Stream)(p) + streamId := uintptr(p) + stream := streams.Get(streamId) + if stream == nil { + // This is bad + return -1 + } + maxbytes := int(cmaxbytes) if maxbytes > cap(stream.buf) { maxbytes = cap(stream.buf) @@ -33,8 +43,15 @@ func go_readcallback(p unsafe.Pointer, cbuf *C.uchar, cmaxbytes C.int) C.int { // Don't bother cleaning up old data because that's not required by the // io.Reader API. n, err := stream.read.Read(stream.buf[:maxbytes]) - if (err != nil && err != io.EOF) || n == 0 { - return 0 + // Go allows returning non-nil error (like EOF) and n>0, libopusfile doesn't + // expect that. So return n first to indicate the valid bytes, let the + // subsequent call (which will be n=0, same-error) handle the actual error. + if n == 0 && err != nil { + if err == io.EOF { + return 0 + } else { + return -1 + } } C.memcpy(unsafe.Pointer(cbuf), unsafe.Pointer(&stream.buf[0]), C.size_t(n)) return C.int(n) @@ -67,11 +84,22 @@ func (s *Stream) Init(read io.Reader) error { if read == nil { return fmt.Errorf("Reader must be non-nil") } + s.read = read s.buf = make([]byte, maxEncodedFrameSize) + s.id = streams.NextId() var errno C.int + + // Immediately delete the stream after .Init to avoid leaking if the + // caller forgets to (/ doesn't want to) call .Delete(). No need for that, + // since the callback is only ever called during a .Read operation; just + // Save and Delete from the map around that every time a reader function is + // called. + streams.Save(s) + defer streams.Del(s) oggfile := C.op_open_callbacks( - unsafe.Pointer(s), + // "C code may not keep a copy of a Go pointer after the call returns." + unsafe.Pointer(s.id), &callbacks, nil, 0, @@ -100,6 +128,8 @@ func (s *Stream) Read(pcm []int16) (int, error) { if len(pcm) == 0 { return 0, nil } + streams.Save(s) + defer streams.Del(s) n := C.op_read( s.oggfile, (*C.opus_int16)(&pcm[0]), @@ -121,6 +151,8 @@ func (s *Stream) ReadFloat32(pcm []float32) (int, error) { if len(pcm) == 0 { return 0, nil } + streams.Save(s) + defer streams.Del(s) n := C.op_read_float( s.oggfile, (*C.float)(&pcm[0]), diff --git a/stream_test.go b/stream_test.go index a2c0203..20eb4a2 100644 --- a/stream_test.go +++ b/stream_test.go @@ -64,10 +64,7 @@ func mustOpenStream(t *testing.T, r io.Reader) *Stream { func opus2pcm(t *testing.T, fname string, buffersize int) []int16 { reader := mustOpenFile(t, fname) - stream, err := NewStream(reader) - if err != nil { - t.Fatalf("Error while creating opus stream: %v", err) - } + stream := mustOpenStream(t, reader) return readStreamPcm(t, stream, buffersize) } @@ -115,7 +112,7 @@ func TestStream(t *testing.T) { } d := maxDiff(opuspcm, wavpcm) // No science behind this number - const epsilon = 12 + const epsilon = 18 if d > epsilon { t.Errorf("Maximum difference between decoded streams too high: %d", d) } diff --git a/streams_map.go b/streams_map.go new file mode 100644 index 0000000..ef666c0 --- /dev/null +++ b/streams_map.go @@ -0,0 +1,58 @@ +package opus + +import ( + "sync" + "sync/atomic" +) + +// A map of simple integers to the actual pointers to stream structs. Avoids +// passing pointers into the Go heap to C. +// +// As per the CGo pointers design doc for go 1.6: +// +// A particular unsafe area is C code that wants to hold on to Go func and +// pointer values for future callbacks from C to Go. This works today but is not +// permitted by the invariant. It is hard to detect. One safe approach is: Go +// code that wants to preserve funcs/pointers stores them into a map indexed by +// an int. Go code calls the C code, passing the int, which the C code may store +// freely. When the C code wants to call into Go, it passes the int to a Go +// function that looks in the map and makes the call. An explicit call is +// required to release the value from the map if it is no longer needed, but +// that was already true before. +// +// - https://github.com/golang/proposal/blob/master/design/12416-cgo-pointers.md +type streamsMap struct { + sync.RWMutex + m map[uintptr]*Stream + counter uintptr +} + +func (sm *streamsMap) Get(id uintptr) *Stream { + sm.RLock() + defer sm.RUnlock() + return sm.m[id] +} + +func (sm *streamsMap) Del(s *Stream) { + sm.Lock() + defer sm.Unlock() + delete(sm.m, s.id) +} + +// NextId returns a unique ID for each call. +func (sm *streamsMap) NextId() uintptr { + return atomic.AddUintptr(&sm.counter, 1) +} + +func (sm *streamsMap) Save(s *Stream) { + sm.Lock() + defer sm.Unlock() + sm.m[s.id] = s +} + +func newStreamsMap() *streamsMap { + return &streamsMap{ + counter: 0, + m: map[uintptr]*Stream{}, + } +} -- 2.48.1