Header.SetExtension does not allow adding a two-byte extension if one-byte extension already exists

[enobufs]

Your environment.

• Version: <=v1.8.13

What did you do?

In scenarios where RTP packet is forwarded across SFU cluster, there are cases where adding a RTP header extension (to the existing list) makes sense. Current implementation of SetExtension method, however, does not upgrade the extension profile from one-byte to two-byte, makes it a bit difficult for users to get around it.

Here is a couple of test cases that represent two kinds of the scenarios:

1. Adding extension with size greater than 16 bytes (needs to be two-byte extension)
2. Adding extension with ID greater than 14 (needs to be two-byte extension)

func TestSetExtension(t *testing.T) {
    t.Run("add two-byte extension due to the size > 16", func(t *testing.T) {
        h := Header{}
        if err := h.SetExtension(1, make([]byte, 2)); err != nil {
            t.Fatal(err)
        }
        if err := h.SetExtension(2, make([]byte, 3)); err != nil {
            t.Fatal(err)
        }

        // Adding another extension that requires two-byte header extension
        if err := h.SetExtension(3, make([]byte, 20)); err != nil {
            t.Error(err)
        }
    })

    t.Run("add two-byte extension due to id > 14", func(t *testing.T) {
        h := Header{}
        if err := h.SetExtension(1, make([]byte, 2)); err != nil {
            t.Fatal(err)
        }
        if err := h.SetExtension(2, make([]byte, 3)); err != nil {
            t.Fatal(err)
        }

        // Adding another extension that requires two-byte header extension
        // because the extmap ID is greater than 14.
        if err := h.SetExtension(16, make([]byte, 4)); err != nil {
            t.Error(err)
        }
    })
}

What did you expect?

Above test case to pass. (under the hood, it should upgrade the profile to the two-byte. (two profiles can not be mixed in the same header if I understand it correctly)

My thoughts

In my opinion, whether the marshler uses one-byte or two-byte could potentially been determined at the time of marshaling. But, I'd also agree that performing some level of validation at the time of SetExtension() call. In this case though, the method needs to take callers intension. For example:

func (h *Header) SetExtensionWithProfile(id uint8, payload []byte, intendedProfile ExtensionProfile) error

[at-wat]

BTW, https://datatracker.ietf.org/doc/html/rfc8285#section-1 says

This document obsoletes [RFC5285] and removes a limitation from RFC 5285 that did not allow sending both one-byte and two-byte header extensions in the same RTP stream.

[enobufs]

@at-wat What you pointed out is about allowing two extension profiles in the same stream (we have been using a=extmap-allow-mixed to allow two header modes in the stream already with no problem). I don't think mixing two extension profiles in the same header is possible because of the extension header format defined in RFC 3550 sec 5.3.1 which allows only one extension profile added to the header. Current implementation has only one ExtensionProfile field in the header, which is correct according to RFC 3550. RFC 8285 inherits this limitation.

[jerry-tao]

It makes sense to me, but expose extensionProfileOneByte and extensionProfileTwoByte and let the developer decide it could do the job. It's just a matter of choosing, does Packet represent a RTP Packet or a validated RTP Packet.