search

EricssonResearch / openwebrtc-android-sdk

SDK for adding OpenWebRTC to your Android app
BSD 2-Clause "Simplified" License
45 stars 29 forks source link

Create JSON messages instead of SDP strings and also handle parsing them #19

Closed sdroege closed 7 years ago

sdroege commented 7 years ago

All applications are producing these now and nothing else works anymore.

superdump commented 7 years ago

@Rugvip : can you review this please?

Rugvip commented 7 years ago

The SDK is meant to be used with traditional SDP signalling, so removing all of that code doesn't make sense. We could add the JSON generating/parsing code as separate methods though. The current to/fromJson and to/fromSdpAttribute should stay, but we can add the new code in something like to/fromOwrJson.

sdroege commented 7 years ago

So basically keeping the old code as is, and calling new functions that implement what I did here now from the demos?

Is anybody else using the JSON message format of the demos? Should it be called from/toOwrJson() or from/toWhatEverNameThisThingHasJson()?

Rugvip commented 7 years ago

Yes.

Only the demos use it, and afaik it's not standardised in any way, I think @pererikb and @adam-be came up with the format. Then again, we could probably find a better name than OwrJson.

sdroege commented 7 years ago

Ok, I'll name them "Owr" then and move to their own functions. Why is any of this in the SDK at all if it's a custom format though?

sdroege commented 7 years ago

Also this basically means duplicating all of the fromJsep/toJsep functions as the outer part of the JSON message is still the same and only the inner part changes.

What the iOS SDK is currently doing is to always add/parse all variants and prefer the OWR specific format. As such, creating huge messages as both variants are going to be in there.

This all seems like it needs a major rework to become consistent, especially between different platforms.

sdroege commented 7 years ago

How should we proceed here? For the upcoming release just the same as on iOS? Fixing it properly on iOS and Android by either 1) only have the standardized stuff in the SDK and everything else in the application (which parts are exactly standardized), or 2) have different functions for both variants in the SDK?

Rugvip commented 7 years ago

Imo we should just stick with JSEP, not sure what the benefit of our own format is. Should probably not take that discussion here though.

sdroege commented 7 years ago

I discussed with @superdump and we decided to just do the same as iOS does.

sdroege commented 7 years ago

This should do it now, please review again. Just one comment about NativeCall in the other pull request that should be considered first (writing that one next)

Rugvip commented 7 years ago

:+1:

sdroege commented 7 years ago

You have to merge it then, I don't have access to this specific repository :)