Binary formatters in packetdump

[aalekseevx]

Description

This PR adds new packet dumper options that let you use RTP/RTCP formatters returning []byte instead of strings. This makes it easier to create dumpers that save RTP data in binary formats like PCAP. The formatter and filter APIs are also updated to handle one RTCP packet at a time, making them consistent with how RTP is handled.

[codecov]

Codecov Report

Attention: Patch coverage is 70.73171% with 24 lines in your changes missing coverage. Please review.

Project coverage is 71.21%. Comparing base (6022ad6) to head (c0efe5f).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/packetdump/packet_dumper.go	64.17%	15 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   71.38%   71.21%   -0.17%     
==========================================
  Files          80       80              
  Lines        4483     4551      +68     
==========================================
+ Hits         3200     3241      +41     
- Misses       1151     1168      +17     
- Partials      132      142      +10     

Flag	Coverage Δ
go	71.21% <70.73%> (-0.04%)	⬇️
wasm	69.39% <70.73%> (-0.01%)	⬇️

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

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

[JoeTurki]

It looks good to me, and I like the change, but I'm not confident enough to approve a PR like this with all the deprecations and a new API. Maybe wait for input from others, though it does make sense to me

[mengelbart]

This looks generally good to me. The only concern I have is that setting both the deprecated and the binary formatters seems to print both results to the same output. I would be happy to remove string formatters completely since the interceptors package is still v0 and since the new API can do everything the old one could, it should be easy to upgrade.

[aalekseevx]

The code would be much simpler if we could break backward compatibility, but I'm not sure whether this is common practice in pion/interceptor. @JoeTurki, @Sean-Der, what do you think? I see a few usages in open source:

https://github.com/search?type=code&q=github.com%2Fpion%2Finterceptor%2Fpkg%2Fpacketdump

[mengelbart]

I think I broke backward compatibility before, but that was a while ago, so maybe @Sean-Der could confirm if this is still ok. About the open source usages: all except the ducksoup one are my projects or forks of my projects, and as far as I know, most of them are no longer used and maintained.

[aalekseevx]

@mengelbart, I’ve added validation to ensure that both formatters are not used simultaneously. Ready to remove the deprecated API if this decision is approved.

[JoeTurki]

If I got to vote, I'd vote against hard deprecating (removing) APIs unless we're doing a major version bump. Even if it this is uncommonly used API It sets a precedent, but I’m not 100% sure.

[aalekseevx]

@JoeTurki, We can merge the current version and remove the deprecated API in the next major release. Does that work for you?

[mengelbart]

Note that the package is still in v0, which, according to https://go.dev/doc/modules/version-numbers, indicates that it is in development and API changes are to be expected. Bumping to v1 would mean that we fix the API from then on (until the next major version).

[JoeTurki]

@JoeTurki, We can merge the current version and remove the deprecated API in the next major release. Does that work for you?

@aalekseevx This makes sense :)

Note that the package is still in v0, which, according to https://go.dev/doc/modules/version-numbers, indicates that it is in development and API changes are to be expected. Bumping to v1 would mean that we fix the API from then on (until the next major version).

@mengelbart Maybe we keep deprecation tags, until we have a stable API then we can bump the version, and delete the deprecated APIs?

Would love to hear @Sean-Der thought on this, and maybe we can have a standard policy for all packages / repos?

[mengelbart]

Feel free to merge as is. We can remove the old API later.

[aalekseevx]

One more thought to consider: some of the interceptors are configured by default in pion/webrtc, so their behavior is part of the pion/webrtc external contract. Therefore, even while staying in v0, we can't change any behavior freely. It might be worth releasing version 1.0.0 in the near future to prevent this confusion.