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

feat: add new removeLineByKey method as a PoC #20

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

Conversation

antsukanova
Copy link

@antsukanova antsukanova commented Apr 25, 2024

Hello, team. I would like to share my Proof of Concept (PoC) regarding the use of the MediaDescription method. Currently, the method only allows adding lines to the Session Description Protocol (SDP). However, I encountered a situation where I needed to delete certain lines from the SDP. To address this, I propose a new method allowing deleting lines from the SDP. My idea is to use specific key strings to avoid any typos or errors. This method will ideally be used in TypeScript projects, but it will also be able to handle strange key values. I would appreciate your thoughts and feedback on this. Let's discuss if this can potentially be useful for our project.

Here's an example of how we are currently working in WCME with deleting properties:

mediaDescription.bandwidth = undefined;

with this PoC:

mediaDescription.removeLineByKey("bandwidth");

Some additional thoughts on why it may potentially be a better option:
Using mediaDescription.bandwidth = undefined does not seem the best option because we have a library that manipulates with SDP, and here, when using the previous option, we disregard the abstraction and directly manipulate the object. If, in the future, the MediaDescription class is no longer an object, then we will have to find all the places that work directly with the object and update them. However, using mediaDescription.removeLineByKey("bandwidth") will only change the implementation of a ts-sdp method.

I also noticed that we have an AvMediaDescription class with some extra properties that have been deleted in the same way. Additionally, we can a similar handling method which could also be placed inside the AvMediaDescription class. This would allow for the handling of deletion keys, such as mediaDescription.simulcast = undefined, from within the AvMediaDescription class.

@antsukanova antsukanova self-assigned this Apr 25, 2024
}

// The 'otherLines' key would represent any other line in otherLines.
if (key === 'otherLines') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this doesn't work very well for the 'other lines' case--do you have any ideas on how that situation would be supported in this scheme?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it's important to be able to handle otherLines here. If we're going to have this API, I'm wondering if the param for this function should just be a Line, similar to the addLine API? That would allow this API to work with any line (including otherLines), not just lines that are associated with a property of MediaDescription.

Calling this API then, would look something like mediaDescription.removeLine(mediaDescription.bandwidth).

This would be tricky for some of the properties though. mid and setup, for example, are not actual lines, so we'd have to handle those a bit differently.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbaldino I've discussed some current code logic with @brycetham, and this example implementation will not be a good idea due to issues with "other lines" as you mentioned. But I want to take time and think about the idea mediaDescription.removeLine(mediaDescription.bandwidth). I also had such an idea, so I just need to see how tricky it is to change some properties' implementations. I will prepare a new example and update this PR. Still, I think aligning functionality to removing likes could be a good idea, and later, it can save us time and keep in our base better abstraction.

@brycetham @bbaldino thanks you for your thoughts and discussion

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.

None yet

3 participants