Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose extension fields #233

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Expose extension fields #233

wants to merge 1 commit into from

Conversation

jerry-tao
Copy link
Member

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.

@jerry-tao jerry-tao changed the title Expose extension filed Expose extension fields Jul 28, 2023
@codecov
Copy link

codecov bot commented Jul 28, 2023

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
Copy link
Member

aler9 commented Jul 29, 2023

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
Copy link
Member Author

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
Copy link
Member

aler9 commented Jul 29, 2023

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
Copy link
Member Author

jerry-tao commented Jul 29, 2023

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
Copy link
Member Author

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
Copy link
Member

aler9 commented Jul 29, 2023

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
Copy link
Member Author

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
Copy link
Member Author

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.

@aler9 aler9 mentioned this pull request Aug 14, 2023
@enobufs
Copy link
Member

enobufs commented Mar 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants