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

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

Closed
winlinvip opened this issue Aug 20, 2023 · 10 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@winlinvip
Copy link

winlinvip commented Aug 20, 2023

Package version

3.0.0-pre.6

Environment

This PR #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.
@winlinvip winlinvip added the bug Something isn't working label Aug 20, 2023
@karasusan
Copy link
Collaborator

@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
Copy link
Author

winlinvip commented Aug 28, 2023

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.

@winlinvip winlinvip reopened this Aug 28, 2023
@karasusan
Copy link
Collaborator

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

@winlinvip
Copy link
Author

winlinvip commented Sep 13, 2023

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

@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
Copy link
Author

winlinvip commented Sep 16, 2023

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

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

winlinvip added a commit to ossrs/srs-unity that referenced this issue Oct 9, 2023
1. Define WEBRTC_3_0_0_PRE_5_OR_BEFORE in OSSRS.Samples.asmdef
2. Fix build fail for Unity-Technologies/com.unity.webrtc#963
@winlinvip
Copy link
Author

winlinvip commented Oct 9, 2023

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.

winlinvip added a commit to ossrs/srs-unity that referenced this issue Oct 9, 2023
    1. Define WEBRTC_3_0_0_PRE_5_OR_BEFORE in OSSRS.Samples.asmdef
    2. Fix build fail for Unity-Technologies/com.unity.webrtc#963
@karasusan
Copy link
Collaborator

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

@winlinvip
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants