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

Change default return value of "getH264ProfileLevelId" #147

Open
wants to merge 1 commit into
base: v3
Choose a base branch
from

Conversation

salmoncode
Copy link

hello!

I suggest getH264ProfileLevelId behavior when profile-level-id is not found in H264 codec parameters.

Now, it returns an empty string, so it will always not match when compared to codecs with a profile-level-id.

On the other hand, the H264 checker used in the JavaScript SDK returns an id of ConstrainedBaseline (Level3.1), which works differently from libmediasoupclient.
https://github.com/ibc/h264-profile-level-id/blob/8f2eee9ed846113a58b9f562947f45cd9d8f7cb9/index.js#L78

I think the JavaScript SDK is more available and should be used that way.

With this fix, I have confirmed that H264 communication with mobile devices is possible even if profile-level-id is not included in the codec parameters on the mediasoup server.

@salmoncode
Copy link
Author

Could someone please review it?
@jmillan ?

@salmoncode salmoncode closed this Oct 27, 2022
@ibc
Copy link
Member

ibc commented Oct 27, 2022

I understand your concern but I'm not sure this is the right approach. If you check ortc.ts in mediasoup-client you'll see that it doesn't return a default value anywhere but it relies on the h264 profile level library, which BTW mimics the built-in h264 profile level utility in libwebrtc C++ code, so the inconsistency should be somewhere else.

So the thing is: is there some difference between ortc.ts in mediasoup-client and ortc.cpp in libmediasoupclient that is causing this problem in the latter? That's the thing to address AFAIS.

@ibc ibc reopened this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants