Expose extension fields

[jerry-tao]

Description

The Extension in the RTP header is exposed, but the id and payload are not. I think it would make sense for these two fields to also be exposed.

[codecov]

Codecov Report

Patch coverage is 96.00% of modified lines.

Files Changed	Coverage
packet.go	96.00%

📢 Thoughts on this report? Let us know!.

[aler9]

Hello, extensions are currently managed through SetExtension(), GetExtensionIDs(), GetExtension(), DelExtension(), that also perform validation and can return errors in case of invalid values. These methods already allow to fully manage extensions.

Do i like this implementation? no. Personally i think that those functions should be removed, extensions should be exposed directly and consistency tests should be performed into MarshalTo() or in a dedicated Check() method.

But anyway we're not talking about a missing feature, since the feature is already fully implemented, we're talking about a design choice, so it's not up to me to decide.

[jerry-tao]

The current API actually cannot meet the requirement of SFU updating extension ID.

payload := header.GetExtension(oldId)
header. SetExtension(newId, payload) // buggy, this may overwrite existing extensions.

The only way we can do this under the current API design is:

tmpHeader := NewHeader()
tmpHeader.SetExtension(newId, header.GetExtension(oldId))
...
header.Extensions = tmpHeader.Extensions

As you can see, this makes no sense.

Since Extensions are already exposed, I think this is an acceptable minimal change without changing the existing API design.

[aler9]

Why do you say that SetExtension() is buggy?

I think your task (changing an extension ID) can be solved by calling in sequence GetExtension(oldId) DelExtension(oldId) and SetExtension(newId, payload). If you want to check whether newId is already is use, there's GetExtension(newId). Is there any case not covered by those functions?

[jerry-tao]

Yes, I could also :

	// get all extensions
	ids := header.GetExtensionIDs()
	for _, id := range ids {
		payloads = append(payloads, header.GetExtensionID(id))
	}
	// empty the old extensions
	header.Extensions = nil
	// set the new extensions
	for i, payload := range payloads {
		header.SetExtension(newIdMap[ids[i]], payload)
	}

Still, why not just expose the ID.

And if we can't change extensions, why expose it.

[jerry-tao]

Whether they are exposed or not, I think they should be consistent - either all exposed or none exposed. Since Extensions are already exposed, then the ID and Payload should also be exposed.

[aler9]

Doing what you propose would allow to bypass consistency checks, as i already wrote, and this would mean losing a feature instead of gaining one. If you want to expose extensions, also reimplement consistency checks into MarshalTo() or into a dedicated Check() method.

[jerry-tao]

Yes, I understand your concern. But I think it's already been bypassed.

Extension        bool
ExtensionProfile uint16
Extensions       []Extension

The exposed fields in the header have already caused inconsistencies.

[jerry-tao]

Here are my thoughts:

• If the fields in the header are private, then it is definitely a bad idea.
• Considering it's already exposed, it won't make it worse.

I will leave it up to you or others, thanks for your replies.

[enobufs]

@aler9 @jerry-tao I filed a similar issue might be related to this one at #249