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

Codec negotiation #606

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from
Open

Codec negotiation #606

wants to merge 30 commits into from

Conversation

ipavlidakis
Copy link
Collaborator

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

@ipavlidakis ipavlidakis added the enhancement New feature or request label Nov 24, 2024
@ipavlidakis ipavlidakis self-assigned this Nov 24, 2024
Copy link

1 Message
📖 Skipping Danger since the Pull Request is classed as Draft/Work In Progress

Generated by 🚫 Danger

@Stream-SDK-Bot
Copy link
Collaborator

Stream-SDK-Bot commented Nov 24, 2024

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 🟢

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

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

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

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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this mean, why -1?

for trackType: TrackType,
codec: VideoCodec
) -> [VideoLayer] {
[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

isn't unknown more appropriate than none?

)
)

// bufferDimensions = BroadcastUtils.adjust(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is going to be deleted, right?

)
}
}
// let outputFormat = VideoCapturingUtils.outputFormat(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we still gonna need this

import Foundation

/// The main SDP parser that uses visitors to process lines.
final class SDPParser {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


try Task.checkCancellation()

if !isFastReconnecting {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

import Foundation
import StreamWebRTC

extension Stream_Video_Sfu_Models_PublishOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's also a Stream_Video_Sfu_Models_SubscribeOption now, available in develop

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

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

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

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.

guard tracks != transceivers else {
return
}
log.error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

try await coordinator.stateAdapter.configurePeerConnections()

try Task.checkCancellation()

await sfuAdapter.sendJoinRequest(
WebRTCJoinRequestFactory()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


final class StreamCaptureDeviceProvider {

private let firstResultIfMiss: Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe useFallback is better?

actor StreamVideoCapturer {

enum Action: @unchecked Sendable, CustomStringConvertible {
case checkBackgroundCameraAccess(_ videoCaptureSession: AVCaptureSession)
Copy link
Collaborator

Choose a reason for hiding this comment

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

small formatting issue here


// MARK: - Private

private func enqueueOperation(
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a second I thought we got back to NSOperation 😅

@ipavlidakis ipavlidakis force-pushed the feature/codec-negotiation branch from d721cb6 to f2291a4 Compare December 13, 2024 16:58
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
@ipavlidakis ipavlidakis force-pushed the feature/codec-negotiation branch from 7c6d91a to 07664aa Compare December 17, 2024 15:42
@ipavlidakis ipavlidakis force-pushed the feature/codec-negotiation branch from b3c4345 to 510beb7 Compare December 18, 2024 16:08
@martinmitrevski martinmitrevski marked this pull request as ready for review December 23, 2024 16:18
@martinmitrevski martinmitrevski requested a review from a team as a code owner December 23, 2024 16:18
/// 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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Successfully merging this pull request may close these issues.

3 participants