Expose functionality for client side applications

[joa]

Clients receiving data from an SRT socket need at minimum functionality that exposes the srctime of a packet as well as the SRT clock. With this change in place one can write code that will replay the packets as they were coming in.

I've tested this using srtgo with v1.4.3 from Haivision/srt and expect it to work with v1.4.1 as well.

[iSchluff]

I think it would probably be better to introduce a separate read call for that like the c-api does it. That way we can still have the basic Read conform to reader at some point.

[jeoliva]

Thanks @joa, I find this feature very interesting.

Regarding the proposed changes I agree with @iSchluff. We are going to change Read/Write methods so they conform to io.Reader and io.Writer interfaces then, @joa, please use a separated read call for this functionality. Other than that proposed change looks good to me.

[jeoliva]

By the way, I was expecting comments about Read/Write related change (io.Reader/io.Writer) but, as nothing arrived, I am going to publish the pending change.

[joa]

Cool. Given the changes to make SrtSocket conform to io.Reader I will adapt the MR. Do you have a preference for the method name?

[jeoliva]

What about ReadMsg? It would be nice also that:

• It returns not just srctime but a copy of the structure returned by srt_recvmsg2 (msgttl, inorder, srctime, pktseq, msgno)
• We also provides an implementation of SendMsg that accepts same parameters supported by srt_sendmsg2 (msgttl, inorder, srctime, pktseq, msgno).

What do you think?

[joa]

Hey folks, let me know what you think. I did add SRT_MSGCTRL as its own struct as well as providing ReadMsg which is used by Read to reduce boilerplate.

[iSchluff]

I think having a ReadMsg call is great, but this requires an extra allocation on every read, so I don't like Read calling ReadMsg so much.

Maybe just deduplicate the epoll portion into a function and keep the actual read calls separate?
Or have the user pass in a pointer to the ctrl struct and avoid the copy if it is nil.

[joa]

@iSchluff Do you think that Read is an actual use case for the SRT library? I may have a biased view but I don't see it used much for signals that aren't live.

However I did produce a benchmark with the changes as you proposed. There is one additional allocation. In general, there's no mensurable difference in performance.

go test -bench=. -benchtime=20s
goos: linux
goarch: amd64
pkg: github.com/haivision/srtgo
cpu: AMD Ryzen Threadripper 3960X 24-Core Processor
BenchmarkSrtSocketRead-12                  23623           1007205 ns/op              16 B/op          2 allocs/op
BenchmarkSrtSocketReadMsg-12               23668           1005299 ns/op              64 B/op          3 allocs/op
PASS
ok      github.com/haivision/srtgo      68.641s

[iSchluff]

I think read is useful even for live because not everybody needs the srt-timestamps. Your benchmark is a bit misleading because srt limits the rate you can read at. But I tested a version without calling srt and apparently the ctrl struct is allocated on the stack so thats fine.

goarch: amd64
pkg: github.com/haivision/srtgo
cpu: Intel(R) Core(TM) i7-8665U CPU @ 1.90GHz
BenchmarkRead-8    	59029542	        17.03 ns/op	       0 B/op	       0 allocs/op
BenchmarkRead2-8   	69096530	        16.99 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/haivision/srtgo	2.231s

var tmp = make([]byte, 1316)
var tmpCtrl = C.SRT_MSGCTRL{}

func read(b []byte) (n int, ctrl SrtMsgCtrl, err error) {
	n = copy(b, tmp)
	ctrl = newSrtMsgCtrl(&tmpCtrl)
	return
}

func read2(b []byte, ctrl *SrtMsgCtrl) (int, error) {
	n := copy(b, tmp)
	ctrl.MsgTTL = int(tmpCtrl.msgttl)
	ctrl.InOrder = int(tmpCtrl.inorder)
	ctrl.Boundary = int(tmpCtrl.boundary)
	ctrl.SrcTime = int64(tmpCtrl.srctime)
	ctrl.PktSeq = int32(tmpCtrl.pktseq)
	ctrl.MsgNo = int32(tmpCtrl.msgno)
	return n, nil
}

[joa]

Your benchmark is a bit misleading because srt limits the rate you can read at.

I think we can agree to disagree here. Real world performance is more important IMHO. Further does an artificial benchmark not include all the side-effects that will have an influence on def-use chains, inlining decisions and so on. The compiler will perform different optimizations here. I don't know if the struct is in this particular case only stack allocated because your methods are small enough so they are inlined and dead code elimination will remove everything. In fact, I think you benchmark effectively only the method call overhead plus copy since your code doesn't produce any additional observable side effects. It would be worth a try to see how the following performs for you on the same machine:

func read(b []byte) (n int, err error) {
	n = copy(b, tmp)
	return
}

I did change the SrtMsgCtrl parsing also during the benchmarks. The signature is also more in line with that you've shown so that escape analysis can potentially prove the struct doesn't need to be heap allocated.

func newSrtMsgCtrl(res *SrtMsgCtrl, ctrl *C.SRT_MSGCTRL) { [...] }

I will resolve conflicts and add the remaining change.

[gizahNL]

Why not let the calling application hand the struct over as a pointer, that way it could statically alloc it once, and reuse it forever, stack allocs are still a form of allocations, and I prefer as close as possible to 0 when I'm pushing hundreds of mbits/s through my applications as that is in the tens of thousands of packets per second.
The C library won't fill the pointer if it's a nil pointer, so a nil pointer can simply be passed safely.

Also for clarity: could you reorder the functions in the PR? It would make the diff more readable.