Codec negotiation #606
Codec negotiation #606
Conversation
Generated by 🚫 Danger |
SDK Size
|
ipavlidakis
force-pushed
the
feature/codec-negotiation
branch
from
06adf99 to
0a4645c
Compare
There was a problem hiding this comment.
Looks great so far 👍 Added a bunch of questions, mostly to understand the parts that are not clear from the spec. Also, would be good to cleanup the commented out code, it's not clear whether we temporary disable it or should be removed.
Also, I would do some testing.
| } | ||
|
|
||
| init( | ||
| id: Int = -1, |
There was a problem hiding this comment.
what does this mean, why -1?
| for trackType: TrackType, | ||
| codec: VideoCodec | ||
| ) -> [VideoLayer] { | ||
| [] |
There was a problem hiding this comment.
why do we need this?
| public enum VideoCodec: String, Sendable { | ||
| case h264, vp8, vp9, av1 | ||
| public enum VideoCodec: String, Sendable, Hashable, CustomStringConvertible { | ||
| case none, h264, vp8, vp9, av1 |
There was a problem hiding this comment.
isn't unknown more appropriate than none?
| ) | ||
| ) | ||
|
|
||
| // bufferDimensions = BroadcastUtils.adjust( |
There was a problem hiding this comment.
this is going to be deleted, right?
| ) | ||
| } | ||
| } | ||
| // let outputFormat = VideoCapturingUtils.outputFormat( |
There was a problem hiding this comment.
I think we still gonna need this
| import Foundation | ||
|
|
||
| /// The main SDP parser that uses visitors to process lines. | ||
| final class SDPParser { |
There was a problem hiding this comment.
Didn't expect this 😄 The WebRTC SDK doesn't have an SDP parser already?
|
|
||
| enum SupportedPrefix: String, Hashable, CaseIterable { | ||
| case unsupported | ||
| case rtmap = "a=rtpmap:" |
There was a problem hiding this comment.
Maybe good to check if this is the only prefix we have/need
| @@ -158,20 +159,22 @@ extension WebRTCCoordinator.StateMachine.Stage { | |||
|
|
|||
| try Task.checkCancellation() | |||
|
|
|||
| try await coordinator.stateAdapter.configurePeerConnections() | |||
There was a problem hiding this comment.
can you also explain this part, why are we removing the setup of PCs?
|
|
||
| try Task.checkCancellation() | ||
|
|
||
| if !isFastReconnecting { |
There was a problem hiding this comment.
ok, I see them moved here, is that because of the publish options?
| import Foundation | ||
| import StreamWebRTC | ||
|
|
||
| extension Stream_Video_Sfu_Models_PublishOption { |
There was a problem hiding this comment.
There's also a Stream_Video_Sfu_Models_SubscribeOption now, available in develop
ipavlidakis
force-pushed
the
feature/codec-negotiation
branch
from
0a4645c to
59f87b9
Compare
🔗 Issue Links
Provide all JIRA tickets and/or GitHub issues related to this PR, if applicable.
🎯 Goal
Describe why we are making this change.
📝 Summary
Provide bullet points with the most important changes in the codebase.
🛠 Implementation
Provide a detailed description of the implementation and explain your decisions if you find them relevant.
🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
imgimg🧪 Manual Testing Notes
Explain how this change can be tested manually, if applicable.
☑️ Contributor Checklist
🎁 Meme
Provide a funny gif or image that relates to your work on this pull request. (Optional)