v2 of this package

[Sean-Der]

Because of #126 we are going to need to do a /v2 break. Anyone have features/ideas/changes they would like to see?

@jech @adamroach @kevmo314 @boks1971 @mengelbart @cnderrauber @OrlandoCo @aler9 @spe-dev You all are active in this repo or use it extensively.

[aler9]

Hello, IMHO a good improvement would be adding MarshalSize() and MarshalTo() to all packets, exactly like pion/rtp, in order to improve performance.

Furthermore, this can be the occasion to fully adopt pion/rtp/v2, that actually is in a sort of limbo - it has been ready for some time but its adoption has stalled.

[cnderrauber]

Application-Defined RTCP Packet would be useful for some use case

[kevmo314]

Application-Defined RTCP Packet would be useful for some use case

👍 application defined RTCP, it's a bit of a pain to incorporate custom RTCP packets right now because raw packets get filtered out by SSRC.

[jech]

If you bump the version of RTCP, then the type of the parameter to WriteRTCP and ReadRTCP will be rtcp/v2.Packet instead of rtcp.Packet. This will break all applications that use WriteRTCP.

This would seem to imply that it is impossible to bump the version of rtcp without simultaneously bumping the version of webrtc, which you've stated you're not ready to do now. (It would of course be possible to define v2.Packet as a type alias of Packet, but that in turn would prevent us from changing the Packet structure, which might or might not be okay.)

[mengelbart]

I think there's a little inconsistency between the TransportLayerCC (and in master also the CCFeedbackReport from RFC8888) and the other packets. While most of the packets have a Header() method that generates a header and is used in Marshal, the two mentioned packets only have a Header field. I think the reason for this might be that calculating the size each time we call Marshal() could be less efficient for these packets than letting a user set the length (because he probably already knows anyway). But as a user I find that kind of unintuitive or surprising.

In case we want to change this: I just created #129 for the RFC8888 packet, where we might be able to fix this, because I think it has not been released in any tagged version. Changing it for the TransportLayerCC would probably be a breaking change.

[shlompy]

I wasn't really sure why supporting application-defined packets would be a pain...
Perhaps there is something I didn't understand, but I've allowed myself to raise a PR to support this:
#181

[emiago]

+1 for MarshalTo. There could be at least implementation of this before v2. At least it can allow calling this directly on type and avoid allocs, unless I miss something.
@Sean-Der something acceptable to be merged before v2 ?

[jech]

+1 for MarshalTo.

I'd prefer Append. MarshalTo can be implemented as:

func MarshalTo(p *P, buf []byte) error {
    return p.Append(buf[:0:length(buf)])
}