feat: refactor, remove hardcoded youtube rtmp url

roanta2 commented 1 year ago

Refactoring on livestream part to not treat youtube as a special case. So now you need to provide the entire rtmp url ( stream url + key).

jitsi-jenkins commented 1 year ago

Hi, thanks for your contribution! If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

codecov[bot] commented 1 year ago

Codecov Report

Merging #508 (24a0dcc) into master (c23529a) will increase coverage by 0.11%. The diff coverage is 5.55%.

Additional details and impacted files

[![Impacted file tree graph](https://codecov.io/gh/jitsi/jibri/pull/508/graphs/tree.svg?width=650&height=150&src=pr&token=P6jrfpYsWM&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi)](https://codecov.io/gh/jitsi/jibri/pull/508?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi) ```diff @@ Coverage Diff @@ ## master #508 +/- ## ============================================ + Coverage 48.05% 48.17% +0.11% Complexity 174 174 ============================================ Files 73 73 Lines 2110 2105 -5 Branches 202 199 -3 ============================================ Hits 1014 1014 + Misses 1026 1021 -5 Partials 70 70 ``` | [Impacted Files](https://codecov.io/gh/jitsi/jibri/pull/508?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi) | Coverage Δ | | |---|---|---| | [...rc/main/kotlin/org/jitsi/jibri/api/xmpp/XmppApi.kt](https://codecov.io/gh/jitsi/jibri/pull/508?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi#diff-c3JjL21haW4va290bGluL29yZy9qaXRzaS9qaWJyaS9hcGkveG1wcC9YbXBwQXBpLmt0) | `51.72% <0.00%> (+3.07%)` | :arrow_up: | | [.../jitsi/jibri/service/impl/StreamingJibriService.kt](https://codecov.io/gh/jitsi/jibri/pull/508?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi#diff-c3JjL21haW4va290bGluL29yZy9qaXRzaS9qaWJyaS9zZXJ2aWNlL2ltcGwvU3RyZWFtaW5nSmlicmlTZXJ2aWNlLmt0) | `0.00% <0.00%> (ø)` | | | [...rc/main/kotlin/org/jitsi/jibri/api/http/HttpApi.kt](https://codecov.io/gh/jitsi/jibri/pull/508?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi#diff-c3JjL21haW4va290bGluL29yZy9qaXRzaS9qaWJyaS9hcGkvaHR0cC9IdHRwQXBpLmt0) | `40.00% <25.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://codecov.io/gh/jitsi/jibri/pull/508?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/jitsi/jibri/pull/508?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi). Last update [c23529a...24a0dcc](https://codecov.io/gh/jitsi/jibri/pull/508?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi).

bbaldino commented 1 year ago

Others probably have a better memory of this than I do, but there's actually existing code that I believe is trying to support exactly this. If you look at this chunk you removed in your PR:

                val rtmpUrl = if (startIq.streamId.isRtmpUrl()) {
                    startIq.streamId
                } else {
                    "$YOUTUBE_URL/${startIq.streamId}"
                }

That's allowing for the value passed as the streamId to be an entire RTMP url.

roanta2 commented 1 year ago

Others probably have a better memory of this than I do, but there's actually existing code that I believe is trying to support exactly this. If you look at this chunk you removed in your PR:
                val rtmpUrl = if (startIq.streamId.isRtmpUrl()) {
                    startIq.streamId
                } else {
                    "$YOUTUBE_URL/${startIq.streamId}"
                }
That's allowing for the value passed as the streamId to be an entire RTMP url.

Yep, so I moved this part in StreamingJibriService.kt without the else branch. My first comment is kind of misleading so I didn't add the functionality for the streamId to be the entire RTMP url, as it was already there, I just removed the custom youtube stuff.

saghul commented 1 year ago

@roanta2 We'll need to modify Jitsi Meet too to use the full rtmp URL format.

saghul commented 1 year ago

LGTM at a glance, but we need to wait to have Jitsi Meet ready before we can apply this. Can you start working on that @roanta2 ?

roanta2 commented 1 year ago

@saghul Thanks. I'm mostly backend but it seems like a minor change in Jisti Meet, so I'll give it a try.

jitsi / jibri

feat: refactor, remove hardcoded youtube rtmp url #508

Codecov Report