RTC: fix play rtc judge for config rtc2rtmp on.(#2863) #2872

chundonglinlin · 2022-01-11T12:02:01Z

Configuration1

vhost __defaultVhost__ {
    rtc {
        enabled     on;
        rtmp_to_rtc off;
        rtc_to_rtmp off;
    }
}

Step1: Rtmp Publish: ffmpeg -re -i 264_aac.flv -c copy -f flv rtmp://127.0.0.1/live/livestream
Step2: Rtmp stream can play: ffplay rtmp://localhost/live/livestream
Step3: Rtc stream can't play(reject tips): ./objs/srs_bench -sr webrtc://localhost/live/livestream

Step1: RTC Publish: ./objs/srs_bench -pr webrtc://localhost/live/livestream -sa avatar.ogg -sv avatar.h264 -fps 25
Step2: Rtmp stream can't play: ffplay rtmp://localhost/live/livestream
Step3: Rtc stream can play: ./objs/srs_bench -sr webrtc://localhost/live/livestream

Configuration2

vhost __defaultVhost__ {
    rtc {
        enabled     on;
        rtmp_to_rtc off;
        rtc_to_rtmp on;
    }
}

Step1: Rtmp Publish: ffmpeg -re -i 264_aac.flv -c copy -f flv rtmp://127.0.0.1/live/livestream
Step2: Rtmp stream can play: ffplay rtmp://localhost/live/livestream
Step3: Rtc stream can't play(reject tips): ./objs/srs_bench -sr webrtc://localhost/live/livestream

Step1: RTC Publish: ./objs/srs_bench -pr webrtc://localhost/live/livestream -sa avatar.ogg -sv avatar.h264 -fps 25
Step2: Rtmp stream can play: ffplay rtmp://localhost/live/livestream
Step3: Rtc stream can play: ./objs/srs_bench -sr webrtc://localhost/live/livestream

Configuration3

vhost __defaultVhost__ {
    rtc {
        enabled     on;
        rtmp_to_rtc on;
        rtc_to_rtmp off;
    }
}

Step1: Rtmp Publish: ffmpeg -re -i 264_aac.flv -c copy -f flv rtmp://127.0.0.1/live/livestream
Step2: Rtmp stream can play: ffplay rtmp://localhost/live/livestream
Step3: Rtc stream can play: ./objs/srs_bench -sr webrtc://localhost/live/livestream

Step1: RTC Publish: ./objs/srs_bench -pr webrtc://localhost/live/livestream -sa avatar.ogg -sv avatar.h264 -fps 25
Step2: Rtmp stream can't play: ffplay rtmp://localhost/live/livestream
Step3: Rtc stream can play: ./objs/srs_bench -sr webrtc://localhost/live/livestream

Configuration4

vhost __defaultVhost__ {
    rtc {
        enabled     on;
        rtmp_to_rtc on;
        rtc_to_rtmp on;
    }
}

Step1: Rtmp Publish: ffmpeg -re -i 264_aac.flv -c copy -f flv rtmp://127.0.0.1/live/livestream
Step2: Rtmp stream can play: ffplay rtmp://localhost/live/livestream
Step3: Rtc stream can play: ./objs/srs_bench -sr webrtc://localhost/live/livestream

Step1: RTC Publish: ./objs/srs_bench -pr webrtc://localhost/live/livestream -sa avatar.ogg -sv avatar.h264 -fps 25
Step2: Rtmp stream can play: ffplay rtmp://localhost/live/livestream
Step3: Rtc stream can play: ./objs/srs_bench -sr webrtc://localhost/live/livestream

winlinvip · 2022-01-12T00:57:54Z

trunk/src/app/srs_app_rtc_api.cpp

-        if (rtmp && !rtmp->inactive()) {
-            return srs_error_new(ERROR_RTC_DISABLED, "Disabled rtmp_to_rtc of %s, see #2728", ruc.req_->vhost.c_str());
+        // FIX ERR: when config is { rtmp_to_rtc off; rtc_to_rtmp on; }, playing rtc will be rejected if publishing rtc stream.
+        if (!_srs_config->get_rtc_to_rtmp(ruc.req_->vhost)) {


This place is not just about checking the configuration, this logic is incorrect. For example, there will still be issues in the following situation:

Configure rtmp2rtc off; rtc2rtmp on;, which means not enabling RTMP to RTC conversion, but enabling RTC to RTMP conversion.

Push RTMP stream, but do not push RTC stream.

Play RTC stream, here it will display a black screen instead of prompting a configuration error.

The core issue here is how to detect this type of error.

Configure as rtmp2rtc off, which means not enabling stream conversion.

Push RTMP stream and play RTC stream. At this point, there won't be any RTC stream (no RTC stream pushed, no RTMP to RTC stream conversion). It must fail at this point, otherwise, further investigation is required to determine why there is a black screen.

The issue with the original logic is not caused by the configuration, but rather due to overlooking a special case: "If there is an RTC stream, there is no need to check the rtmp2rtc switch." Because if there is an RTC stream, it means there is already an RTC stream available, so there is no need to worry about any switches. The RTC stream should be playable in this case.

If there is an RTC stream, it doesn't mean that enabling rtc2rtmp implies the presence of an RTC stream. Instead, it should directly check whether there is an RTC stream being pushed and obtain the stream status from the RTC source.

It should be changed to this:

'It should be changed to this:

bool is_rtc_stream_active = xxx; // Check if RTC has a stream if (!is_rtc_stream_active && !_srs_config->get_rtc_from_rtmp(ruc.req_->vhost)) { // Check if there is an RTMP stream and rtmp2rtc is disabled, in this case an error should be reported, the current logic is correct.

TRANS_BY_GPT3

Added RtcStream acquisition, only if the stream does not exist will it check the rtmp2rtc switch and print tips.

TRANS_BY_GPT3

winlinvip · 2022-01-13T04:25:14Z

trunk/src/app/srs_app_rtc_api.cpp

@@ -187,7 +187,9 @@ srs_error_t SrsGoApiRtcPlay::do_serve_http(ISrsHttpResponseWriter* w, ISrsHttpMe
    }

    // For RTMP to RTC, fail if disabled and RTMP is active, see https://github.com/ossrs/srs/issues/2728
-    if (!_srs_config->get_rtc_from_rtmp(ruc.req_->vhost)) {
+    SrsRtcSource* source = _srs_rtc_sources->fetch(ruc.req_);
+    bool is_rtc_stream_active = (source && !source->can_publish());


This place exposes a source object, but in reality, it is only used to generate the value of is_rtc_stream_active. Therefore, the source variable should be hidden to prevent others from misusing it.

// Whether RTC stream is active. bool is_rtc_stream_active = false; if (true) { SrsRtcSource* source = _srs_rtc_sources->fetch(ruc.req_); is_rtc_stream_active = (source && !source->can_publish()); } // For RTMP to RTC, fail if disabled and RTMP is active, see https://github.com/ossrs/srs/issues/2728 if (!is_rtc_stream_active && !_srs_config->get_rtc_from_rtmp(ruc.req_->vhost)) {

Although it increases the number of lines of code, it hides unused variables, so others don't need to understand the code inside because it simply generates this flag. It is very worthwhile.

PS: Pay attention to comments and paragraphs.

TRANS_BY_GPT3

Okay, it has been modified. Finally understood the if (true) {} syntax of srs.

TRANS_BY_GPT3

codecov-commenter · 2022-01-13T05:14:58Z

Codecov Report

Merging #2872 (5fe69d0) into 4.0release (c6c2e97) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@              Coverage Diff               @@
##           4.0release    #2872      +/-   ##
==============================================
- Coverage       60.22%   60.21%   -0.01%     
==============================================
  Files             121      121              
  Lines           51070    51073       +3     
==============================================
  Hits            30756    30756              
- Misses          20314    20317       +3

| Impacted Files | Coverage Δ | |'

Translated to English while maintaining the markdown structure:

'| Impacted Files | Coverage Δ | |
|---|---|---|
| trunk/src/app/srs_app_rtc_api.cpp | 0.00% <0.00%> (ø) | |
| trunk/src/app/srs_app_rtc_source.hpp | 9.09% <ø> (ø) | |

'

Translated to English while maintaining the markdown structure:

'| trunk/src/app/srs_app_rtc_api.cpp | 0.00% <0.00%> (ø) | |
| trunk/src/app/srs_app_rtc_source.hpp | 9.09% <ø> (ø) | |

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data'

Translated to English while maintaining the markdown structure:

'> Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update c6c2e97...5fe69d0. Read the comment docs.

TRANS_BY_GPT3

chundonglinlin force-pushed the bugfix/play_rtc_judge_error branch from 6fabe91 to 5c2bed1 Compare January 11, 2022 12:16

winlinvip reviewed Jan 12, 2022

View reviewed changes

winlinvip self-assigned this Jan 12, 2022

chundonglinlin force-pushed the bugfix/play_rtc_judge_error branch 2 times, most recently from 67050f3 to 990a1a9 Compare January 12, 2022 09:52

winlinvip reviewed Jan 13, 2022

View reviewed changes

chundonglinlin force-pushed the bugfix/play_rtc_judge_error branch from 990a1a9 to a7da5a3 Compare January 13, 2022 05:10

RTC: fix play rtc judge for config rtc2rtmp on.(ossrs#2863)

5fe69d0

chundonglinlin force-pushed the bugfix/play_rtc_judge_error branch from a7da5a3 to 5fe69d0 Compare January 13, 2022 05:12

winlinvip approved these changes Jan 13, 2022

View reviewed changes

winlinvip merged commit 5848897 into ossrs:4.0release Jan 13, 2022

winlinvip added a commit that referenced this pull request Jan 13, 2022

RTC: fix play rtc judge for config rtc2rtmp on. (#2872). v4.0.229

7796083

winlinvip added the TransByAI Translated by AI/GPT. label Jul 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTC: fix play rtc judge for config rtc2rtmp on.(#2863) #2872

RTC: fix play rtc judge for config rtc2rtmp on.(#2863) #2872

chundonglinlin commented Jan 11, 2022 •

edited

Loading

winlinvip Jan 12, 2022 •

edited

Loading

chundonglinlin Jan 13, 2022 •

edited by winlinvip

Loading

winlinvip Jan 13, 2022 •

edited

Loading

chundonglinlin Jan 13, 2022 •

edited by winlinvip

Loading

codecov-commenter commented Jan 13, 2022 •

edited by winlinvip

Loading

RTC: fix play rtc judge for config rtc2rtmp on.(#2863) #2872

RTC: fix play rtc judge for config rtc2rtmp on.(#2863) #2872

Conversation

chundonglinlin commented Jan 11, 2022 • edited Loading

Configuration1

Configuration2

Configuration3

Configuration4

winlinvip Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

chundonglinlin Jan 13, 2022 • edited by winlinvip Loading

Choose a reason for hiding this comment

winlinvip Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

chundonglinlin Jan 13, 2022 • edited by winlinvip Loading

Choose a reason for hiding this comment

codecov-commenter commented Jan 13, 2022 • edited by winlinvip Loading

Codecov Report

chundonglinlin commented Jan 11, 2022 •

edited

Loading

winlinvip Jan 12, 2022 •

edited

Loading

chundonglinlin Jan 13, 2022 •

edited by winlinvip

Loading

winlinvip Jan 13, 2022 •

edited

Loading

chundonglinlin Jan 13, 2022 •

edited by winlinvip

Loading

codecov-commenter commented Jan 13, 2022 •

edited by winlinvip

Loading