Codec negotiation #606

ipavlidakis · 2024-11-24T11:02:19Z

🔗 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.

Before	After
`img`	`img`

🧪 Manual Testing Notes

Explain how this change can be tested manually, if applicable.

☑️ Contributor Checklist

I have signed the Stream CLA (required)
This change follows zero ⚠️ policy (required)
This change should receive manual QA
Changelog is updated with client-facing changes
New code is covered by unit tests
Comparison screenshots added for visual changes
Affected documentation updated (tutorial, CMS)

🎁 Meme

Provide a funny gif or image that relates to your work on this pull request. (Optional)

github-actions · 2024-11-24T11:03:20Z

	1 Error
🚫	Please use more than one word. `2176173`

	2 Warnings
⚠️	Please be sure to complete the `Contributor Checklist` in the Pull Request description
⚠️	Big PR

Generated by 🚫 Danger

Stream-SDK-Bot · 2024-11-24T11:12:43Z

SDK Size

`title`	`develop`	`branch`	`diff`	`status`
StreamVideo	7.05 MB	7.27 MB	+226 KB	🟢
StreamVideoSwiftUI	2.07 MB	2.08 MB	+3 KB	🟢
StreamVideoUIKit	2.21 MB	2.21 MB	+4 KB	🟢
StreamWebRTC	9.85 MB	9.85 MB	0 KB	🟢

martinmitrevski

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.

martinmitrevski · 2024-12-10T10:28:54Z

Sources/StreamVideo/Models/PublishOptions.swift

+        }
+
+        init(
+            id: Int = -1,


what does this mean, why -1?

martinmitrevski · 2024-12-10T10:30:39Z

Sources/StreamVideo/Models/PublishOptions.swift

+        for trackType: TrackType,
+        codec: VideoCodec
+    ) -> [VideoLayer] {
+        []


why do we need this?

martinmitrevski · 2024-12-10T10:32:35Z

Sources/StreamVideo/Models/VideoCodec.swift

-public enum VideoCodec: String, Sendable {
-    case h264, vp8, vp9, av1
+public enum VideoCodec: String, Sendable, Hashable, CustomStringConvertible {
+    case none, h264, vp8, vp9, av1


isn't unknown more appropriate than none?

martinmitrevski · 2024-12-10T10:34:34Z

Sources/StreamVideo/WebRTC/Screensharing/BroadcastScreenCapturer.swift

-                )
-            )
-
+//            bufferDimensions = BroadcastUtils.adjust(


this is going to be deleted, right?

martinmitrevski · 2024-12-10T10:35:26Z

Sources/StreamVideo/WebRTC/Screensharing/ScreenshareCapturer.swift

-                )
-            }
-        }
+//        let outputFormat = VideoCapturingUtils.outputFormat(


I think we still gonna need this

martinmitrevski · 2024-12-10T12:57:37Z

Sources/StreamVideo/WebRTC/v2/SDP Parsing/SDPParser.swift

+import Foundation
+
+/// The main SDP parser that uses visitors to process lines.
+final class SDPParser {


Didn't expect this 😄 The WebRTC SDK doesn't have an SDP parser already?

martinmitrevski · 2024-12-10T12:58:40Z

Sources/StreamVideo/WebRTC/v2/SDP Parsing/SupportedPrefix.swift

+
+enum SupportedPrefix: String, Hashable, CaseIterable {
+    case unsupported
+    case rtmap = "a=rtpmap:"


Maybe good to check if this is the only prefix we have/need

martinmitrevski · 2024-12-10T13:00:42Z

Sources/StreamVideo/WebRTC/v2/StateMachine/Stages/WebRTCCoordinator+Joining.swift

@@ -158,20 +159,22 @@ extension WebRTCCoordinator.StateMachine.Stage {

                    try Task.checkCancellation()

-                    try await coordinator.stateAdapter.configurePeerConnections()


can you also explain this part, why are we removing the setup of PCs?

martinmitrevski · 2024-12-10T13:09:44Z

Sources/StreamVideo/WebRTC/v2/StateMachine/Stages/WebRTCCoordinator+Joining.swift

+
+            try Task.checkCancellation()
+
+            if !isFastReconnecting {


ok, I see them moved here, is that because of the publish options?

martinmitrevski · 2024-12-10T13:13:46Z

...ces/StreamVideo/WebRTC/v2/Extensions/Stream_Video_Sfu_Models_PublishOption+Convenience.swift

+import Foundation
+import StreamWebRTC
+
+extension Stream_Video_Sfu_Models_PublishOption {


There's also a Stream_Video_Sfu_Models_SubscribeOption now, available in develop

martinmitrevski

Things are looking great! Added few small things, we should also clean up the commented out code and few todos.
Also, a lot of testing is needed, cc @testableapple for visibility.

martinmitrevski · 2024-12-13T15:43:32Z

Sources/StreamVideo/WebRTC/v2/PeerConnection/RTCPeerConnectionCoordinator.swift

+        guard tracks != transceivers else {
+            return
+        }
+        log.error(


do we do this just for logging purposes? Shouldn't we also try to match them in those cases?

martinmitrevski · 2024-12-13T15:51:36Z

Sources/StreamVideo/WebRTC/v2/StateMachine/Stages/WebRTCCoordinator+Joining.swift

-                    try await coordinator.stateAdapter.configurePeerConnections()
-
-                    try Task.checkCancellation()
-
                    await sfuAdapter.sendJoinRequest(
                        WebRTCJoinRequestFactory()


shouldn't we have one shared instance (or static methods) for this? I see it being created in few places.

...es/StreamVideo/WebRTC/v2/VideoCapturing/ActionHandlers/Camera/CameraStopCaptureHandler.swift

martinmitrevski · 2024-12-13T16:39:02Z

Sources/StreamVideo/WebRTC/v2/VideoCapturing/StreamCaptureDeviceProvider.swift

+
+final class StreamCaptureDeviceProvider {
+
+    private let firstResultIfMiss: Bool


maybe useFallback is better?

martinmitrevski · 2024-12-13T16:39:41Z

Sources/StreamVideo/WebRTC/v2/VideoCapturing/StreamVideoCapturer.swift

+actor StreamVideoCapturer {
+
+    enum Action: @unchecked Sendable, CustomStringConvertible {
+    case checkBackgroundCameraAccess(_ videoCaptureSession: AVCaptureSession)


small formatting issue here

martinmitrevski · 2024-12-13T16:43:26Z

Sources/StreamVideo/WebRTC/v2/VideoCapturing/StreamVideoCapturer.swift

+
+    // MARK: - Private
+
+    private func enqueueOperation(


For a second I thought we got back to NSOperation 😅

Improve protobuf models description WIP Fix publishOptions missing when start screenSharing Change trackLookUp parsing Background camera access check Update track handling Transceiver and track validation on SetPublisher Fix trackInfo mismatch Updates - Always send preferredPublishOptions - Update participant handling on track publish/unpublish - Initial approach for showing all publishing codecs in stats

…publish

ipavlidakis · 2024-12-23T18:00:23Z

Sources/StreamVideo/Models/PublishOptions.swift

+        /// Returns the codec for the specified track type.
+        /// - Parameter trackType: The type of track for which to get the codec.
+        /// - Returns: The codec for the specified track type.
+        func codec(for trackType: TrackType) -> Stream_Video_Sfu_Models_Codec {


Why not converting the existing VideoCodec to its SFU model directly?

I didn't find one, we only have in the opposite direction

martinmitrevski

Things are looking good client side! Let's retest after the backend bugs are fixed and then merge this one.

sonarqubecloud · 2024-12-25T09:15:13Z

Quality Gate passed

Issues
141 New issues
0 Accepted issues

Measures
0 Security Hotspots
68.1% Coverage on New Code
0.7% Duplication on New Code

See analysis details on SonarQube Cloud

ipavlidakis added the enhancement New feature or request label Nov 24, 2024

ipavlidakis self-assigned this Nov 24, 2024

ipavlidakis force-pushed the feature/codec-negotiation branch from 06adf99 to 0a4645c Compare December 6, 2024 16:16

martinmitrevski reviewed Dec 10, 2024

View reviewed changes

ipavlidakis force-pushed the feature/codec-negotiation branch 2 times, most recently from 80bd59a to d721cb6 Compare December 13, 2024 10:45

martinmitrevski reviewed Dec 13, 2024

View reviewed changes

ipavlidakis force-pushed the feature/codec-negotiation branch from d721cb6 to f2291a4 Compare December 13, 2024 16:58

ipavlidakis added 5 commits December 17, 2024 17:42

Add comments

ed29bef

Update ActionHandlers

eabc398

Remove unnecessary files

3c8d6e5

Update logic for layers calculation

07664aa

ipavlidakis force-pushed the feature/codec-negotiation branch from 7c6d91a to 07664aa Compare December 17, 2024 15:42

ipavlidakis added 5 commits December 17, 2024 18:10

Fix av1 layers rewrite

20f1c0f

Fix screensharing missing due to empty rid

0cb8f58

Fix low quality issue

0c2498c

Fix trackInfo layer

2217b21

Fix unnecessarily updating transceivers

510beb7

ipavlidakis force-pushed the feature/codec-negotiation branch from b3c4345 to 510beb7 Compare December 18, 2024 16:08

ipavlidakis added 8 commits December 18, 2024 18:41

Update pip and fix screensharing

8429948

Update pip tracks when app becomes active again

12087c5

Maintain tracks in the SetPublisherRequest after codec disabled or un…

0634082

…publish

Initial tests and changes

e476f55

Updated mocks

cdd234e

Add tests I

7cf8914

WIP

2176173

Make tests compile again

4992051

ipavlidakis and others added 12 commits December 20, 2024 14:09

Add/update tests

5595f7f

Add/Update tests II

2f9de82

Add/Update tests III

4b1959b

Update publishOptions event handling to align with SFU

ce358bf

Add/update tests IV

9ded17e

Announce expected tracks on migration/rejoin

d84e738

Fixed the tests

076c74a

Tests cleanup

5a0033f

Fixed wrong target membership

c2c084c

Fixed tests

6374c5a

Misuse of compact map, fix build on Xcode 15

93ee255

Sending codec in setPublisher

01ae4ad

martinmitrevski marked this pull request as ready for review December 23, 2024 16:18

martinmitrevski requested a review from a team as a code owner December 23, 2024 16:18

ipavlidakis commented Dec 23, 2024

View reviewed changes

Small fixes

7c9c603

martinmitrevski approved these changes Dec 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Codec negotiation #606

Codec negotiation #606

ipavlidakis commented Nov 24, 2024

github-actions bot commented Nov 24, 2024 •

edited

Loading

Stream-SDK-Bot commented Nov 24, 2024 •

edited

Loading

martinmitrevski left a comment

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski Dec 10, 2024

martinmitrevski left a comment

martinmitrevski Dec 13, 2024

martinmitrevski Dec 13, 2024

martinmitrevski Dec 13, 2024

martinmitrevski Dec 13, 2024

martinmitrevski Dec 13, 2024

ipavlidakis Dec 23, 2024

martinmitrevski Dec 23, 2024

martinmitrevski left a comment

sonarqubecloud bot commented Dec 25, 2024

		@@ -158,20 +159,22 @@ extension WebRTCCoordinator.StateMachine.Stage {

		try Task.checkCancellation()

		try await coordinator.stateAdapter.configurePeerConnections()


		final class StreamCaptureDeviceProvider {

		private let firstResultIfMiss: Bool

Codec negotiation #606

Are you sure you want to change the base?

Codec negotiation #606

Conversation

ipavlidakis commented Nov 24, 2024

🔗 Issue Links

🎯 Goal

📝 Summary

🛠 Implementation

🎨 Showcase

🧪 Manual Testing Notes

☑️ Contributor Checklist

🎁 Meme

github-actions bot commented Nov 24, 2024 • edited Loading

Stream-SDK-Bot commented Nov 24, 2024 • edited Loading

SDK Size

martinmitrevski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinmitrevski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinmitrevski left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 25, 2024

Quality Gate passed

github-actions bot commented Nov 24, 2024 •

edited

Loading

Stream-SDK-Bot commented Nov 24, 2024 •

edited

Loading