-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[multistream] Fix videoroom support for h264 / vp9 #2398
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch, even though it's currently incomplete. It's lacking the negotiation of the profiles in the answer too, for instance (see what master currently does for a reference).
plugins/janus_videoroom.c
Outdated
@@ -2020,6 +2020,8 @@ typedef struct janus_videoroom_subscriber_stream { | |||
janus_videoroom_media type; /* Type of this stream (audio, video or data) */ | |||
janus_audiocodec acodec; /* Audio codec this publisher is using (if audio) */ | |||
janus_videocodec vcodec; /* Video codec this publisher is using (if video) */ | |||
char *h264_profile; /* H264 profile used for this stream (if video and H264 codec) */ | |||
char *vp9_profile; /* VP9 profile this publisher is using (if video and VP9 codec) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix indentation of the comments (you're using spaces, code style requires tabs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
stream->h264_profile = g_strdup(ps->fmtp); | ||
} else if (stream->vcodec == JANUS_VIDEOCODEC_VP9) { | ||
stream->vp9_profile = g_strdup(ps->fmtp); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no check to see if ps->fmtp
exists: if it's NULL
, I think g_strdup
will segfault.
Please fix the indentation (you're using spaces, code style requires tabs). Besides, per code style there should be no space between if
and the bracket (if (
--> if(
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
plugins/janus_videoroom.c
Outdated
@@ -9231,6 +9243,7 @@ static void *janus_videoroom_handler(void *data) { | |||
h264_profile = NULL; | |||
ps->vcodec = videoroom->vcodec[i]; | |||
ps->pt = janus_videocodec_pt(ps->vcodec); | |||
ps->fmtp = g_strdup(vp9_profile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. The profile we extracted is not the content of the fmtp line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I made these changes, the fmtp
variable was not being used for anything in this branch. A question I have is how relevant it really is. Should it just be removed and replaced with a specific variable for h264_profile
and vp9_profile
?
plugins/janus_videoroom.c
Outdated
@@ -9242,6 +9255,7 @@ static void *janus_videoroom_handler(void *data) { | |||
vp9_profile = NULL; | |||
ps->vcodec = videoroom->vcodec[i]; | |||
ps->pt = janus_videocodec_pt(ps->vcodec); | |||
ps->fmtp = g_strdup(h264_profile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. The profile we extracted is not the content of the fmtp line.
I have a question around the desired direction overall. Is the goal to force every participant to use the same profile every time? Or to allow for there to be multiple profiles used simultaneously, with the problem being that some parties may not be able to handle subscribing due to client side restrictions? |
@lminiero I think I have everything resolved now. The missing component you mentioned previously was actually implemented already by a previous author (I think) in the code starting on line 9325. Let me know if there is anything else you need me to fix. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes, I added a couple of additional notes.
plugins/janus_videoroom.c
Outdated
@@ -9173,6 +9188,7 @@ static void *janus_videoroom_handler(void *data) { | |||
ps->acodec = JANUS_AUDIOCODEC_NONE; | |||
ps->vcodec = JANUS_VIDEOCODEC_NONE; | |||
ps->pt = -1; | |||
ps->fmtp = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, g_malloc0
allocates memory and sets it all to 0
to that variable is NULL
already.
plugins/janus_videoroom.c
Outdated
const char *video_fmtp = janus_sdp_get_fmtp(offer, m->index, pt); | ||
if(video_fmtp) { | ||
ps->fmtp = g_strdup(video_fmtp); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed anymore, since we're already tracking the profile. Besides, this will cause leaks if ps->fmtp
exists already, since you're setting the value to a new allocated property and the old one will be lost.
plugins/janus_videoroom.c
Outdated
const char *video_fmtp = janus_sdp_get_fmtp(offer, m->index, pt); | ||
if(video_fmtp) { | ||
ps->fmtp = g_strdup(video_fmtp); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: no need for the property, and even if it is, leaks are likely.
@tomleavy can you let me know when you'll have some time to address those notes? I'm planning some fixes on the multistream branch and a rebase to master, but I'd rather merge this first in order to avoid conflicts later on. Thanks! |
Hey @lminiero I can take care of this tomorrow |
@lminiero should be ready to go now, rebased into 1 commit to make it clean for you |
Thanks, merging! 👍 |
Multistream support currently breaks h264 and vp9 usage due to not passing around the required profile information. Currently this fix only works if a preferred h264 / vp9 profile is set at the videoroom level.
I tried to also make it work generically, but could not figure out the proper way to extract the negotiated publisher profile id to set if the room did not have a preferred profile set, which would allow for a mixed set of profiles. Any help there would be appreciated