Unity-Technologies / com.unity.webrtc

WebRTC package for Unity
Other
738 stars 185 forks source link

[BUG]: WebRTC.Initialize is removed and breaks the build of Example. #963

Closed winlinvip closed 8 months ago

winlinvip commented 10 months ago

Package version

3.0.0-pre.6

Environment

This PR https://github.com/Unity-Technologies/com.unity.webrtc/pull/850 causes the build of the example to fail:

Assets/ossrs.io/Video Streaming and WebRTC Samples/Streamer/SrsStreamer.cs(34,16): 
error CS0117: 'WebRTC' does not contain a definition for 'Initialize'
private void Awake() 
{
    WebRTC.Initialize();
}

How can I write an example that is compatible with both pre5 and pre6? Pre5 requires WebRTC.Initialize, while pre6 has removed it. Is there any way to check the API version like OpenSSL:

// Something like this or similar to do this?
#if version < 3.0.0-pre.6
WebRTC.Initialize();
#endif

// Like OpenSSL version check?
#if OPENSSL_VERSION_NUMBER < 0x10002000L // v1.0.2
dtls_ctx = SSL_CTX_new(DTLSv1_method());
#else

Alternatively, instead of removing the API, why not just retain it and make it empty function? This would be a straightforward approach to maintain API compatibility and prevent any disruptions.

If only support pre6, someone use old versions of SDK will file issues and get stuck.

Steps To Reproduce

  1. Use examples https://github.com/ossrs/srs-unity
  2. Install pre6.
  3. Build it, failed.
karasusan commented 10 months ago

@winlinvip Hi, you can use the assembly definition to determine the package version like below.

image

The definition should be used in your script. image

Currently APIs are not stable yet, but I will try not to change APIs as possible.

winlinvip commented 10 months ago

I changed the code to:

    private void Awake()
    {
#if WEBRTC_3_0_0_PRE_6_OR_LATER
        Debug.Log("WebRTC: WEBRTC_3_0_0_PRE_6_OR_LATER is enabled");
#else
        WebRTC.Initialize();
#endif
        Debug.Log("WebRTC: Initialize ok");
    }

Still failed:

Assets/ossrs.io/Video Streaming and WebRTC Samples/Publisher/SrsPublisher.cs(31,16): 
error CS0117: 'WebRTC' does not contain a definition for 'Initialize'

I searched the macro WEBRTC_3_0_0_PRE_6_OR_LATER in this project, didn't find it.

karasusan commented 10 months ago

@winlinvip You need to make an assembly definitions and add version defines yourself. https://docs.unity3d.com/Manual/ScriptCompilationAssemblyDefinitionFiles.html#create-asmdef

winlinvip commented 9 months ago

Thank you, but I don't think that's the right approach. For instance, if I create a WEBRTC_3_0_0_PRE_6_OR_LATER, users would need to modify it based on the WebRTC SDK they're using. This could confuse people and make it difficult to use.

Instead, I'd prefer to duplicate examples for each WebRTC SDK version, like:

v3.0.0-pre5
    Players.cs Publisher.cs
v3.0.0-pre6
    Players.cs Publisher.cs

Maintaining this would be a terrible experience for me. Please don't change the API. Why not keep the empty WebRTC.Initialize() and just remove it from the documentation? This way, it won't break the build.

karasusan commented 9 months ago

@winlinvip It is easy to add the empty method for comatibility, but I want to ask a question before adding them.

What I can't understand about your issue, I'm not sure why you need to add code for two versions. I think you can avoid duplication with define directives. Is there any reason?

winlinvip commented 9 months ago

@karasusan I want to make one example that works for all webrtc sdk versions, like SrsPlayer.cs for pre4, pre5, and pre6.

If the API isn't compatible, I'll need to create a separate repository or branch for each version, which will be confusing and complicated:

  1. Users won't know which example version matches their webrtc sdk version since SrsPlayer.cs isn't included in the webrtc sdk example.

  2. I'll need to add a new branch for each sdk version, even if they're compatible. I'll also have to test and check each webrtc sdk version.

  3. If webrtc sdk users want to upgrade for bug fixes or new features, they'll face issues and have to read many documents. For instance, if someone uses pre3 and there's a bug fix in pre6, if want to upgrade from pre3 to pre6, they'll need to check the changes in pre4, pre5, and pre6.

If the SDK API isn't compatible, it can cause problems for me and everyone else using the SDK. This can happen when we update it to fix issues or when a new user tries to use an API that doesn't match the examples.

karasusan commented 9 months ago

@winlinvip Why don't you add an assembly definitions file to your SDK? Is there any reason you can't use an asmdef?

winlinvip commented 8 months ago

Got it! Thank you! 👍

I add a file OSSRS.Samples.asmdef which defines:

{
    "references": [
        "Unity.WebRTC"
    ],
    "versionDefines": [
        {
            "name": "com.unity.webrtc",
            "expression": "[2.4.0-exp.11,3.0.0-pre.5]",
            "define": "WEBRTC_3_0_0_PRE_5_OR_BEFORE"
        }
    ],
}

In code such as SrsPublisher.cs to use the macro:

    private void Awake()
    {
#if WEBRTC_3_0_0_PRE_5_OR_BEFORE
        WebRTC.Initialize();
#endif
        Debug.Log("WebRTC: Initialize ok");
    }

It works!

BTW: I notice the pre7 is released at roadmap at 2023.9, but not in releases, how can I get the pre7 version and test it? Thank you.

karasusan commented 8 months ago

@winlinvip Oh, I'm sorry to delay the release pre7. I am working on it.

winlinvip commented 7 months ago

Has version pre7 not been released yet? Are there any changes to the SDK? Will this SDK continue to be maintained in the future? 😅