Send nil interceptor attributes on RTP writes

[kcaffrey]

Currently an interceptor.Attributes map is allocated for each RTP write. However, interceptors only strictly need to allocate an attributes map when wanting to set a value. None of the default interceptors currently allocate a new map when passed nil on the write path. The gcc.SendSideBWE interceptor, when TWCC is negotiated, does allocate attributes for every outbound packet. Thus, by sending nil instead of allocating a new map, we can save one allocation per WriteRTP()

All interceptors in pion/interceptor are already safe with nil attributes on the write path, and pion/interceptor#431 made GetRTPHeader() and GetRTCPPackets() safe to call on nil attributes. Users with custom interceptors should ensure that attributes is always checked for nil before calling Set().

The benchmark added in this package shows a reduction of allocations for a write+read round trip from 8 allocs/op to 7 allocs/op.

Running the benchmark with pion/ice#943 (not yet merged) and pion/srtp#378 (merged but not yet released) results in 1 alloc/op. The remaining allocation is in the replay detector. I plan to make a parallel API for the replay detector to have Check(seq) and Accept(seq) to avoid the closure allocation. Combined with this PR, once srtp is updated to include that, there will be zero allocations in the hot path of reading and writing an RTP packet.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.99%. Comparing base (a21f980) to head (2af5b8a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3483      +/-   ##
==========================================
- Coverage   86.03%   85.99%   -0.04%     
==========================================
  Files          81       81              
  Lines       10442    10442              
==========================================
- Hits         8984     8980       -4     
- Misses       1017     1022       +5     
+ Partials      441      440       -1     

Flag	Coverage Δ
go	85.99% <100.00%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JoTurk]

@kcaffrey

Do you mean add parallel methods?

We can add private methods and switch our interceptors to them. Until we move to the next major release.

One small point of disagreement I have here is that adding a setting engine option does avoid back-compat breakages. All existing code will continue to work as-is

very few users create custom interceptors and i think all the users that create custom interceptors are either aware to handle nil attribute or fix it if it panics, making this an op-tin will help very a few users. and just add more noise to the already noisy setting engine.
Most users use default interceptors, custom interceptors are currently in a weird position where normal users don't define them because they aren't super useful and power users skip them and use custom buffer to have more control (what we do at my work).

Built-in interceptors already pass nill down the chain on some errors https://github.com/pion/interceptor/blob/main/pkg/nack/generator_interceptor.go#L114 and we handle nil before read, so it's not really unexpected or a new behavior.

I think it's just a better trade off to do it without a setting engine.

[kcaffrey]

pion/interceptor#431 makes the cache methods nil-safe. After that merges and gets released, I'll update this PR to send nil attributes on the write path with no setting engine option.

[kcaffrey]

@JoTurk I have updated this PR according to our discussion. PTAL, thanks!

[JoTurk]

Thank you so much :)