meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.25k stars 2.48k forks source link

[1.1.0] Janus sip plugin recorded peer video is black, because of unimplemented keyframe request #3092

Closed tvildo closed 2 years ago

tvildo commented 2 years ago

What version of Janus is this happening on? 1.1.0

Have you tested a more recent version of Janus too? Yes, Master

Was this working before? No

Is there a gdb or libasan trace of the issue? No

Additional context I see that in the source code of sip plugin, when recording is enabled, we don't request PLI. So when I try to play, recorded video is black. Blame: See source code here

I just want to ask if there is an easy way to get peer session reference and send PLI as we do it here:

lminiero commented 2 years ago

Yes, that's currently missing, IIRC because we thought that since there's no guarantee the other peer is a browser, they may not understand a PLI or FIR, and we don't know which one they support either. We can't use the same code we use for the WebRTC user (2nd link, the one that asks the core to send a PLI) as the SIP side has a different media stack, where we have to manually craft/send packets ourselves.

tvildo commented 2 years ago

I think it will be great to add optional parameter in "start recording" request and based on that we can decide to send PLI or not. I can implement that, but It is just not clear to me how can I get peer session reference at that point in recording request handler in sip plugin.

lminiero commented 2 years ago

As I said, there's no peer session reference, as the peer is a SIP user, who may or may not be a browser user. The RTCP packet must be manually crafted and manually sent on the related socket.

lminiero commented 2 years ago

@tvildo please test the PR above.

tvildo commented 2 years ago

Thank you @lminiero for quick fix. In my case I found another problem between Janus <--> Asterisk and solved it and created pull request with description what and why I did

lminiero commented 2 years ago

Closing the issue then, as your PR aims to address a different requirement.

michelepra commented 2 years ago

can also be done in nosip plugin?