Interceptors can't be shared between PeerConnections #1749
Comments
|
@at-wat @jeremija What do you think of an API like this?
Add a
|
|
I think the memory space of the interceptors are better to be separated by default to prevent accidental interference among the sessions. Instead, it would be safer to conditionally share data on creation of Interceptor instance by introducing InterceptorFactory. type InterceptorFactory interface {
NewInterceptor(sessionId string, or something) (Interceptor, error)
}
// Example of InterceptorFactory sharing something between Interceptors.
type SharedInterceptorFactory struct {
sharedBuffer map[string]*sharedData
}
type sharedData struct{}
func (f *SharedInterceptorFactory) NewInterceptor(sessionId string) (Interceptor, error) {
i := &SharedInterceptor{}
if data, ok := f.sharedBuffer[sessionId]; ok {
i.data = data
} else {
i.data = &sharedData{}
f.sharedBuffer[sessionId] = i.data
}
return i, nil
}
type SharedInterceptor struct {
data *sharedData
...
}
// Example of InterceptorFactory not sharing data between Interceptors.
type UnsharedInterceptorFactory struct {}
func (f *UnsharedInterceptorFactory) NewInterceptor(_ string) (Interceptor, error) {
return &UnsharedInterceptor{}, nil
}
type UnsharedInterceptor struct {
...
} |
|
Fantastic idea! Totally agree with that Do you have any idea how we can fix the problems people are having today with interceptor re-use? People are creating one API and sharing between multiple PeerConnections. What do you think of I would rather not start a |
|
I also had an idea to add some sort of identifier to the If we decide to move in this direction, does it make sense to unify this Factory and the current |
|
@jeremija 100% agree. I think we just need to fix the re-use problem, we have two choices.
@at-wat what do you think is better? I would prefer a copy because it means zero API breakage, it does make interceptors more complicated unfortunately. |
This commit addresses the changes discussed [here][1]. I added `SessionID` to all interceptor methods and ensured that the existing interceptors can be shared by different `PeerConnection`s. I defined an `interceptor.Factory` following AtWat's suggestion. It took a while for this to click for me: I didn't realize that the `interceptor.Registry` contained already prebuilt interceptors. I like the idea of a `Factory` better than implementing `Copy`. I added the `interceptor.Registry.AddFactory` method and modified the internals a little. The new `interceptor.NewChainFromFactories` will now be called from the `Registry`. The `interceptor.SharedFactory` allows us to easily keep the current behavior with existing interceptors. I left a number of TODOs, main bullet points being: 1. I think binding anything before calling `BindRTCPWriter` should be an error, because otherwise it becomes difficult to track state (e.g. if we've started a write loop). But I did not want to add an error return value to all methods before discussing it. If we do decide that calling `BindRTCPWriter` is the first step, then I think it would make sense to pass `RTCPWriter` as an argument to `interceptor.Registry.Build`? 2. We could modify the interceptors to only have a single `loop` goroutine which would go through all SSRCs for all bound sessions at once, but I'm not sure what's the best way to concurrently handle RTCP writes - I'm sure we wouldn't want a hanging RTCPWrite to slow down any other processing or writes. I also think that spinning up an unbounded number of goroutines just to do these writes is not a good idea, so I'm not sure how to processed. I'm open to suggestions. Maybe this shouldn't be a part of this change, but it's something to think about. [1]: pion/webrtc#1749
Closes #37. This commit addresses the changes discussed [here][1]. I added `SessionID` to all interceptor methods and ensured that the existing interceptors can be shared by different `PeerConnection`s. I defined an `interceptor.Factory` following AtWat's suggestion. It took a while for this to click for me: I didn't realize that the `interceptor.Registry` contained already prebuilt interceptors. I like the idea of a `Factory` better than implementing `Copy`. I added the `interceptor.Registry.AddFactory` method and modified the internals a little. The new `interceptor.NewChainFromFactories` will now be called from the `Registry`. The `interceptor.SharedFactory` allows us to easily keep the current behavior with existing interceptors. I left a number of TODOs, main bullet points being: 1. I think binding anything before calling `BindRTCPWriter` should be an error, because otherwise it becomes difficult to track state (e.g. if we've started a write loop). But I did not want to add an error return value to all methods before discussing it. If we do decide that calling `BindRTCPWriter` is the first step, then I think it would make sense to pass `RTCPWriter` as an argument to `interceptor.Registry.Build`? 2. We could modify the interceptors to only have a single `loop` goroutine which would go through all SSRCs for all bound sessions at once, but I'm not sure what's the best way to concurrently handle RTCP writes - I'm sure we wouldn't want a hanging RTCPWrite to slow down any other processing or writes. I also think that spinning up an unbounded number of goroutines just to do these writes is not a good idea, so I'm not sure how to processed. I'm open to suggestions. Maybe this shouldn't be a part of this change, but it's something to think about. [1]: pion/webrtc#1749
Closes #37. This commit addresses the changes discussed [here][1]. I added `SessionID` to all interceptor methods and ensured that the existing interceptors can be shared by different `PeerConnection`s. I defined an `interceptor.Factory` following AtWat's suggestion. It took a while for this to click for me: I didn't realize that the `interceptor.Registry` contained already prebuilt interceptors. I like the idea of a `Factory` better than implementing `Copy`. I added the `interceptor.Registry.AddFactory` method and modified the internals a little. The new `interceptor.NewChainFromFactories` will now be called from the `Registry`. The `interceptor.SharedFactory` allows us to easily keep the current behavior with existing interceptors. I left a number of TODOs, main bullet points being: 1. I think binding anything before calling `BindRTCPWriter` should be an error, because otherwise it becomes difficult to track state (e.g. if we've started a write loop). But I did not want to add an error return value to all methods before discussing it. If we do decide that calling `BindRTCPWriter` is the first step, then I think it would make sense to pass `RTCPWriter` as an argument to `interceptor.Registry.Build`? 2. We could modify the interceptors to only have a single `loop` goroutine which would go through all SSRCs for all bound sessions at once, but I'm not sure what's the best way to concurrently handle RTCP writes - I'm sure we wouldn't want a hanging RTCPWrite to slow down any other processing or writes. I also think that spinning up an unbounded number of goroutines just to do these writes is not a good idea, so I'm not sure how to processed. I'm open to suggestions. Maybe this shouldn't be a part of this change, but it's something to think about. [1]: pion/webrtc#1749
|
@at-wat @Sean-Der Just asked you to review my attempt in fixing this. It took a while for the I left a number of TODOs which is the only reason the build is failing (linting), but I'd be curious to hear your thoughts! More info in the PR: |
Closes #37. This commit addresses the changes discussed [here][1]. I added `SessionID` to all interceptor methods and ensured that the existing interceptors can be shared by different `PeerConnection`s. I defined an `interceptor.Factory` following AtWat's suggestion. It took a while for this to click for me: I didn't realize that the `interceptor.Registry` contained already prebuilt interceptors. I like the idea of a `Factory` better than implementing `Copy`. I added the `interceptor.Registry.AddFactory` method and modified the internals a little. The new `interceptor.NewChainFromFactories` will now be called from the `Registry`. The `interceptor.SharedFactory` allows us to easily keep the current behavior with existing interceptors. I left a number of TODOs, main bullet points being: 1. I think binding anything before calling `BindRTCPWriter` should be an error, because otherwise it becomes difficult to track state (e.g. if we've started a write loop). But I did not want to add an error return value to all methods before discussing it. If we do decide that calling `BindRTCPWriter` is the first step, then I think it would make sense to pass `RTCPWriter` as an argument to `interceptor.Registry.Build`? 2. We could modify the interceptors to only have a single `loop` goroutine which would go through all SSRCs for all bound sessions at once, but I'm not sure what's the best way to concurrently handle RTCP writes - I'm sure we wouldn't want a hanging RTCPWrite to slow down any other processing or writes. I also think that spinning up an unbounded number of goroutines just to do these writes is not a good idea, so I'm not sure how to processed. I'm open to suggestions. Maybe this shouldn't be a part of this change, but it's something to think about. [1]: pion/webrtc#1749
Closes #37. This commit addresses the changes discussed [here][1]. I added `SessionID` to all interceptor methods and ensured that the existing interceptors can be shared by different `PeerConnection`s. I defined an `interceptor.Factory` following AtWat's suggestion. It took a while for this to click for me: I didn't realize that the `interceptor.Registry` contained already prebuilt interceptors. I like the idea of a `Factory` better than implementing `Copy`. I added the `interceptor.Registry.AddFactory` method and modified the internals a little. The new `interceptor.NewChainFromFactories` will now be called from the `Registry`. The `interceptor.SharedFactory` allows us to easily keep the current behavior with existing interceptors. I left a number of TODOs, main bullet points being: 1. I think binding anything before calling `BindRTCPWriter` should be an error, because otherwise it becomes difficult to track state (e.g. if we've started a write loop). But I did not want to add an error return value to all methods before discussing it. If we do decide that calling `BindRTCPWriter` is the first step, then I think it would make sense to pass `RTCPWriter` as an argument to `interceptor.Registry.Build`? 2. We could modify the interceptors to only have a single `loop` goroutine which would go through all SSRCs for all bound sessions at once, but I'm not sure what's the best way to concurrently handle RTCP writes - I'm sure we wouldn't want a hanging RTCPWrite to slow down any other processing or writes. I also think that spinning up an unbounded number of goroutines just to do these writes is not a good idea, so I'm not sure how to processed. I'm open to suggestions. Maybe this shouldn't be a part of this change, but it's something to think about. [1]: pion/webrtc#1749
|
@jeremija sorry for the late response. My idea on #1749 (comment) was intended not to pass I'm not very sure it actually works, but it should be safer since each Interceptor instance doesn't need to care the |
|
Hi @at-wat, no worries and thanks for taking a look! I see your point, but after talking to Sean it seemed like he wanted to provide the ability to share the same interceptor resources among multiple peer connections to conserve resources where that made sense, for example in the same conference room (@Sean-Der please correct me if I misunderstood). So that's the main reason why I added the Now, there's high probability I over-engineered the whole thing and that the correct thing to do is just to remove the |
|
Sorry to taking too long. I'll take a look again. |
|
@jeremija sorry for the delayed response.
I still don't understand why
To support both globally shared resource and session designated shared resource, and hold // Example of InterceptorFactory sharing something between Interceptors.
type SharedInterceptorFactory struct {
sessionBuffer map[string]*sharedData // shared data in the session
globalBuffer *sharedData // globally shared data among all Interceptors
}
type sharedData struct{}
func (f *SharedInterceptorFactory) NewInterceptor(sessionId string) (Interceptor, error) {
i := &SharedInterceptor{
globalData: f.globalBuffer,
sessionID: sessionId,
}
if data, ok := f.sessionBuffer[sessionId]; ok {
i.sessionData = data
} else {
i.sessionData = &sharedData{}
f.sessionBuffer[sessionId] = i.data
}
return i, nil
}
type SharedInterceptor struct {
sessionData *sharedData
globalData *sharedData
sessionID string // Optinal if sessionID is required in Interceptor's methods
...
}Or, is there any case that |
This is where I was hoping to get Sean's input - the current implementation of At some point Sean said that he'd want to do processing for all sessions in a single goroutine, and your idea of using a We could also defer this decision to the users who need total control, and they could build their own interceptors and factories that are more tightly coupled. That said, I think your comment makes total sense, and it would simplify implementations quite a bit. |
|
@at-wat @jeremija The new design needs to accomodate the way API/Interceptor is used in So my goal/question is 'How can we keep this code compiling and make it work'. Today when a, b or c is closed it closes the shared interceptor and the user loses everything. |
|
Closing for #1956 |
Right now an interceptor can't be shared among interceptors. This is a problem because we support sharing an API between PeerConnections. This was an API we provided and can't break.
This means when we close the first PeerConnection here it stops everything. We also are sharing the Interceptors between PeerConnections which also seems really bad? I am not even sure of the behavior.
I think we have two choices here. We can add a
copy()to interceptors, or we can support sharing them and do refcounting.Sharing+refcounting could be good for high performance cases? It could be powerful to have one NACK loop for all your PeerConnections.
The text was updated successfully, but these errors were encountered: