FCE-748: support for protobufs #246

MiloszFilimowski · 2024-11-19T16:46:22Z

Description

Updated mobile SDK to support protobufs for media events.
This PR does not support old version of handling media events via JSON so this is a BREAKING CHANGE.

Motivation and Context

New version of fishjam added protobufs for media events.

How has this been tested?

Run and tested bidirectional connection between Android/iOS

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to
not work as expected)

linear · 2024-11-19T16:46:25Z

FCE-837 Make sure local peer metadata is handled by mobile client

…ocal-peer-metadata

maksg · 2024-11-27T14:29:50Z

packages/ios-client/Sources/FishjamClient/utils/AnyJson.swift

@@ -230,3 +236,13 @@ public struct AnyJson: Codable {
        var stringValue: String { return key }
    }
 }
+
+extension String {


Are those really better than just using AnyJson(from: )? Seems a little bit like an overkill

Fair point, I just thought it looks better but you're right.

maksg · 2024-11-27T14:30:39Z

packages/ios-client/Sources/FishjamClient/utils/AnyJson.swift

+    }
+
+    func toAnyJson() -> AnyJson? {
+        return try? AnyJson(from: self)


And if they are better then this could be try? toAnyJson()

maksg · 2024-11-27T14:35:14Z

packages/ios-client/Sources/FishjamClient/utils/types.swift

+extension Encodable {
+
+    public func toJsonString() throws -> String {
+        if let json = String(data: try JSONEncoder().encode(self), encoding: .utf8) {


I know it's controversial but what about guard and throw in else instead?

OK MAN, NO PROBLEM

NO SHOUTING IN THE CHAT PLEASE, PEOPLE ARE TRYING TO WORK HERE

maksg · 2024-11-27T14:35:44Z

packages/ios-client/Sources/FishjamClient/utils/types.swift

+
+    var toJsonStringOrEmpty: String {
+        do {
+            return try self.toJsonString()


We can remove self here

maksg · 2024-11-27T14:36:43Z

packages/ios-client/Sources/FishjamClient/utils/types.swift

+
+extension [String: Metadata] {
+    public func toDictionaryJson() -> [String: String] {
+        return mapValues { val in


I think mapValues(\.toJsonStringOrEmpty) should work here

maksg · 2024-11-27T14:44:24Z

packages/react-native-client/src/hooks/usePeers.ts

-    })),
-  }));
+  return peers.map((peer) => {
+    return {


Remove returns

maksg · 2024-11-27T14:44:45Z

scripts/compile_protos.mk

@@ -69,7 +69,7 @@ compile_ios: $(PROTOC) $(PROTOC_GEN_SWIFT) sync
 	@echo "Compiling proto files for iOS"
 	@for proto in $(PROTO_FILES); do \
 		echo "Compiling iOS: $$proto"; \
-		$(PROTOC) -I=$(PROTOS_PATH) -I=$(PROTOC_BASE_PATH)/include --plugin=$(PROTOC_GEN_SWIFT) --swift_out=$(IOS_OUT) $$proto; \
+		$(PROTOC) -I=$(PROTOS_PATH) -I=$(PROTOC_BASE_PATH)/include --plugin=$(PROTOC_GEN_SWIFT) --swift_out=$(IOS_OUT) --swift_opt=Visibility=public $$proto; \
 	done
 	@echo "DONE for iOS"



New line please

maksg · 2024-11-27T14:46:08Z

I left some comments. But other than that great job! 🎉

…etadata' into mfilimowski/fce-837-local-peer-metadata

roznawsk · 2024-11-28T12:00:17Z

packages/ios-client/Sources/FishjamClient/webrtc/PeerConnectionManager.swift

@@ -176,11 +176,9 @@ internal class PeerConnectionManager: NSObject, RTCPeerConnectionDelegate {
        config.candidateNetworkPolicy = .all
        config.tcpCandidatePolicy = .disabled

-        // if ice servers are not empty that probably means we are using turn servers
+        // if ice servers are not empty that probably means we are using turn servers, which are not handled at the moment


suggestion:

We might actually start using turn servers, that will be passed in the connected message. They will be handled out of the box

Suggested change

// if ice servers are not empty that probably means we are using turn servers, which are not handled at the moment

roznawsk · 2024-11-28T12:01:03Z

packages/ios-client/Sources/FishjamClient/webrtc/PeerConnectionManager.swift

@@ -176,11 +176,9 @@ internal class PeerConnectionManager: NSObject, RTCPeerConnectionDelegate {
        config.candidateNetworkPolicy = .all
        config.tcpCandidatePolicy = .disabled

-        // if ice servers are not empty that probably means we are using turn servers
+        // if ice servers are not empty that probably means we are using turn servers, which are not handled at the moment
        if iceServers.count > 0 {


suggestion:
If they are empty, then we can just pass this empty list from protobuf message

Suggested change

if iceServers.count > 0 {

roznawsk · 2024-11-28T12:10:03Z

packages/ios-client/Sources/FishjamClient/webrtc/PeerConnectionManager.swift

+            guard let trackId: String = transceiver.sender.track?.trackId else {
+                return
+            }
+            mapping[trackId] = 500  // TODO: Change with simulcast


suggestion: more appropriate value would be 1500000 as this value is in bit\s

roznawsk · 2024-11-28T12:11:26Z

packages/ios-client/Sources/FishjamClient/webrtc/RTCEngineCommunication.swift

+                    var trackBitrates = Fishjam_MediaEvents_Peer_MediaEvent.TrackBitrates()
+                    trackBitrates.trackID = key
+                    var bitrate = Fishjam_MediaEvents_Peer_MediaEvent.VariantBitrate()
+                    bitrate.variant = .unspecified  // TODO: Add with simulcast


note: this will remain .unspecified for non-simulcast tracks

roznawsk · 2024-11-28T12:12:58Z

packages/ios-client/Sources/FishjamClient/webrtc/RTCEngineCommunication.swift

+        var candidate = Fishjam_MediaEvents_Candidate()
+        candidate.candidate = sdp
+        candidate.sdpMLineIndex = sdpMLineIndex
+        candidate.sdpMid = String(sdpMid)


praise: 👍🏽

roznawsk · 2024-11-28T12:17:13Z

...d-client/FishjamClient/src/main/java/com/fishjamcloud/client/webrtc/PeerConnectionManager.kt

+      if (needsRestart) {
        pc.restartIce()
      }


suggestion: In web-sdk we removed this completely. We assume ice restarts are not needed during negotiation. In fact in web-sdk the restarts introduced failures. Try removing this and checking if everything works OK

Suggested change

if (needsRestart) {

pc.restartIce()

}

…etadata' into mfilimowski/fce-837-local-peer-metadata

mironiasty

Two general comments:

can we read package version from configuration file, so releasing app will require less work with updating?
I would consider adding link to linear item for simulcast - it might help in future, to understand todo in code

mironiasty · 2024-11-29T13:28:52Z

examples/fishjam-chat/plugin/src/with-custom-config-ios.ts

-  podfileContent = podfileContent.replace(targetRegex, (match) => {
-    return match.replace(podToReplace, replacementPod);
-  });
+  podfileContent = podfileContent.replace(targetRegex, (match) =>


I think this new rule could be decoupled from this PR... Bet let it be as is.

mironiasty · 2024-11-29T13:29:49Z

.../android-client/FishjamClient/src/main/java/com/fishjamcloud/client/FishjamClientInternal.kt

@@ -162,6 +161,7 @@ internal class FishjamClientInternal(
                PeerNotifications.PeerMessage.AuthRequest
                  .newBuilder()
                  .setToken(connectConfig.token)
+                  .setSdkVersion("mobile-0.6.0")


Can we read this value from config files?

🫡 https://linear.app/swmansion/issue/FCE-952

mironiasty · 2024-11-29T13:32:04Z

.../android-client/FishjamClient/src/main/java/com/fishjamcloud/client/FishjamClientInternal.kt

            tracksTypes,
            localEndpoint.tracks.values.toList()
          )

        rtcEngineCommunication.sdpOffer(
          offer.description,
          localEndpoint.tracks.map { (_, track) -> track.webrtcId() to track.metadata }.toMap(),
-          offer.midToTrackIdMapping
+          offer.midToTrackIdMapping,
+          localEndpoint.tracks.map { (_, track) -> track.webrtcId() to 1500000 }.toMap() // TODO: Update with simulcast


WDYT about adding link to simulcast project in linear?

Suggested change

localEndpoint.tracks.map { (_, track) -> track.webrtcId() to 1500000 }.toMap() // TODO: Update with simulcast

localEndpoint.tracks.map { (_, track) -> track.webrtcId() to 1500000 }.toMap() // TODO(FCE-XXX): Update with simulcast

mironiasty · 2024-11-29T13:33:00Z

...-client/FishjamClient/src/main/java/com/fishjamcloud/client/webrtc/RTCEngineCommunication.kt

-        encoding.rid
-      )
-    )
+    // TODO: This will be useful after simulcast is enabled


Suggested change

// TODO: This will be useful after simulcast is enabled

// TODO(FCE-XXX): This will be useful after simulcast is enabled

mironiasty · 2024-11-29T13:34:51Z

packages/android-client/FishjamClient/src/main/java/fishjam/media_events/CandidateKt.kt

@@ -0,0 +1,106 @@
+// Generated by the protocol buffer compiler. DO NOT EDIT!


idea for future: would it be possible to keep this generated files in folder called generated?

Unfortunately it looks like it's not possible, as the package name comes from the protofile and protoc does not provide the ability to modify it. So although we could change the file location it won't work on Android as it expects certain folder structure.

We could modify the protos file, but this might destroy code generation for others...

mironiasty · 2024-11-29T13:35:29Z

packages/ios-client/Sources/FishjamClient/FishjamClientInternal.swift

    }

    func websocketDidConnect() {
        let authRequest = Fishjam_PeerMessage.with({
            $0.authRequest = Fishjam_PeerMessage.AuthRequest.with({
                $0.token = self.config?.token ?? ""
+                $0.sdkVersion = "mobile-0.6.0"


Could it be read from config file?

🫡 https://linear.app/swmansion/issue/FCE-952

…ocal-peer-metadata

MiloszFilimowski added 10 commits November 14, 2024 17:13

WIP

9b62636

WIP part 1

2629e8a

make protos public in swift

c389a7a

WIP 2 ios

c940e92

WIP 3

b3f097e

working iOS

8833d45

WIP2

278951a

WIP iOS

1888efc

WIP Android

617106d

WIP

0ca5899

MiloszFilimowski added 12 commits November 20, 2024 12:14

Merge branch 'main' into mfilimowski/fce-837-local-peer-metadata

b9635ac

fix connect event

d97aca7

fix android metadata

489ef51

partially fix android metadata serialization

37bc0ac

Merge remote-tracking branch 'origin/main' into mfilimowski/fce-837-l…

36e2f59

…ocal-peer-metadata

fix event parsing

5c8edbb

WIP

ce1d0fe

remove unused iOS code

030f5ce

remove Android unused events

6be5536

lint files

da97aec

parse local metadata

ace8f22

small refactor

f2abce9

MiloszFilimowski force-pushed the mfilimowski/fce-837-local-peer-metadata branch from 1f26504 to f2abce9 Compare November 22, 2024 10:21

MiloszFilimowski added 6 commits November 25, 2024 13:24

Merge remote-tracking branch 'origin/main' into mfilimowski/fce-837-l…

733d72b

…ocal-peer-metadata

bump protos

358c98a

WIP

928992b

bump proto

297f780

migrate iOS

2f29d13

migrate android

bdca9b9

maksg reviewed Nov 27, 2024

View reviewed changes

maksg requested changes Nov 27, 2024

View reviewed changes

MiloszFilimowski and others added 4 commits November 27, 2024 16:15

Merge branch 'main' into mfilimowski/fce-837-local-peer-metadata

ca85835

try tests without failing

f25e74d

code review improvements

4eb9229

lint fix

bd0c300

MiloszFilimowski force-pushed the mfilimowski/fce-837-local-peer-metadata branch from 6c233ab to bd0c300 Compare November 28, 2024 10:08

MiloszFilimowski and others added 5 commits November 28, 2024 11:31

Try different port for tests

6247845

change docker image

194b5c4

Merge branch 'main' into mfilimowski/fce-837-local-peer-metadata

b240c76

restore emulator version

8b60356

Merge remote-tracking branch 'origin/mfilimowski/fce-837-local-peer-m…

8cfa544

…etadata' into mfilimowski/fce-837-local-peer-metadata

MiloszFilimowski requested a review from maksg November 28, 2024 11:44

roznawsk requested changes Nov 28, 2024

View reviewed changes

MiloszFilimowski and others added 3 commits November 28, 2024 16:09

revert to stable image

8e4db7f

add code review improvements

5f4a834

Merge branch 'main' into mfilimowski/fce-837-local-peer-metadata

ca32a70

MiloszFilimowski requested a review from roznawsk November 28, 2024 15:33

MiloszFilimowski added 2 commits November 28, 2024 16:36

lint fix

8e1cd86

Merge remote-tracking branch 'origin/mfilimowski/fce-837-local-peer-m…

a8e5eca

…etadata' into mfilimowski/fce-837-local-peer-metadata

mironiasty reviewed Nov 29, 2024

View reviewed changes

MiloszFilimowski force-pushed the mfilimowski/fce-837-local-peer-metadata branch 4 times, most recently from a381061 to 5083e24 Compare November 29, 2024 15:46

Code review improvements

c8579ba

MiloszFilimowski force-pushed the mfilimowski/fce-837-local-peer-metadata branch from 5083e24 to c8579ba Compare November 29, 2024 15:46

Merge remote-tracking branch 'origin/main' into mfilimowski/fce-837-l…

cdda266

…ocal-peer-metadata

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FCE-748: support for protobufs #246

FCE-748: support for protobufs #246

MiloszFilimowski commented Nov 19, 2024 •

edited

Loading

linear bot commented Nov 19, 2024

maksg Nov 27, 2024

MiloszFilimowski Nov 28, 2024

maksg Nov 27, 2024

maksg Nov 27, 2024

MiloszFilimowski Nov 28, 2024

maksg Nov 28, 2024

maksg Nov 27, 2024

maksg Nov 27, 2024

maksg Nov 27, 2024

maksg Nov 27, 2024

maksg commented Nov 27, 2024

roznawsk Nov 28, 2024

roznawsk Nov 28, 2024

roznawsk Nov 28, 2024

roznawsk Nov 28, 2024

roznawsk Nov 28, 2024

roznawsk Nov 28, 2024

mironiasty left a comment

mironiasty Nov 29, 2024

mironiasty Nov 29, 2024

MiloszFilimowski Nov 29, 2024

mironiasty Nov 29, 2024

mironiasty Nov 29, 2024

mironiasty Nov 29, 2024

MiloszFilimowski Nov 29, 2024

mironiasty Nov 29, 2024

MiloszFilimowski Nov 29, 2024

	localEndpoint.tracks.map { (_, track) -> track.webrtcId() to 1500000 }.toMap() // TODO: Update with simulcast
	localEndpoint.tracks.map { (_, track) -> track.webrtcId() to 1500000 }.toMap() // TODO(FCE-XXX): Update with simulcast

	// TODO: This will be useful after simulcast is enabled
	// TODO(FCE-XXX): This will be useful after simulcast is enabled

		@@ -0,0 +1,106 @@
		// Generated by the protocol buffer compiler. DO NOT EDIT!

FCE-748: support for protobufs #246

Are you sure you want to change the base?

FCE-748: support for protobufs #246

Conversation

MiloszFilimowski commented Nov 19, 2024 • edited Loading

Description

Motivation and Context

How has this been tested?

Types of changes

linear bot commented Nov 19, 2024

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

maksg commented Nov 27, 2024

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

mironiasty 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

MiloszFilimowski commented Nov 19, 2024 •

edited

Loading