CR #1: Roundtrip3: Comments from Derek Buitenhuis

[winlinvip]

Review for #1

[winlinvip]

Derek Buitenhuis:

> +rtc_muxer_deps_any="openssl"

Given we support a bunch of SSL libraries, how exactly does this work with that? Can
it be abstracted out? OpenSSL poses licensing issues for many.

[winlinvip]

Winlin: Currently, we only support OpenSSL, but after merging this patch, we intend to add support for other SSL libraries. This is because including support for all SSL libraries in a single patch would make it overly complex and difficult for others to follow. Is this plan acceptable, or do we need to support all SSL libraries within this patch?

目前，我们只支持 OpenSSL，但在合并此补丁后，我们打算添加对其他 SSL 库的支持。这是因为在一个补丁中包含对所有 SSL 库的支持会使其过于复杂且难以让其他人理解。不知道这可以接受吗？还是说，我们需要一次性支持所有 SSL 库？

[T-bagwell]

这个可以作为 TODO 做

[winlinvip]

Add a TODO to configure.

+# TODO: Support libtls, mbedtls, and gnutls.
 rtc_muxer_deps_any="openssl"

Fixed.

[winlinvip]

Derek Buitenhuis:

> +rtc_muxer_deps_any="openssl"

Further, I think this is missing a bunch of dependencies like HTTP and AVC?

[winlinvip]

Winlin: In the Makefile, we add dependencies at this line:

OBJS-$(CONFIG_RTC_MUXER) += rtcenc.o avc.o http.o srtp.o

I'm not certain if we should also include avc and http as dependency libraries like this:

rtc_muxer_deps_any="openssl avc http"

Need your help. @T-bagwell

[winlinvip]

Derek Buitenhuis:

> +#ifndef CONFIG_OPENSSL
> +#error "DTLS is not supported, please enable openssl"
> +#else
> +#include <openssl/ssl.h>
> +#include <openssl/err.h>
> +#if OPENSSL_VERSION_NUMBER < 0x1010102fL
> +#error "OpenSSL version 1.1.1b or newer is required"
> +#endif
> +#endif

This should be in configure.

[winlinvip]

Winlin: I elliminated the lines of code that used macros for checking, and instead, I only perform the check in the configure script like this:

enabled openssl            && {
    enabled rtc_muxer && {
        $pkg_config --exists --print-errors "openssl >= 1.0.1k" ||
        require_pkg_config openssl "openssl >= 1.0.1k" openssl/ssl.h SSL_library_init ||
        require_pkg_config openssl "openssl >= 1.0.1k" openssl/ssl.h OPENSSL_init_ssl
    }
}

Fixed.

[winlinvip]

Derek Buitenhuis:

> +#define DTLS_CIPHER_SUTES "ECDHE-ECDSA-AES128-GCM-SHA256"\
> +    ":ECDHE-RSA-AES128-GCM-SHA256"\
> +    ":ECDHE-ECDSA-AES128-SHA"\
> +    ":ECDHE-RSA-AES128-SHA"\
> +    ":ECDHE-ECDSA-AES256-SHA"\
> +    ":ECDHE-RSA-AES256-SHA"

The reason for this specific subset should be documented.

[winlinvip]

Winlin: Resolved by employing "ALL" cipher suite to attain the maximum compatibility.

    /**
     * We activate "ALL" cipher suites to align with the peer's capabilities,
     * ensuring maximum compatibility.
     */
    if (SSL_CTX_set_cipher_list(dtls_ctx, "ALL") != 1) {

Fixed.

[winlinvip]

Derek Buitenhuis:

> +#define NALU_TYPE_STAP_A 24

Why is a NALU type being defined in a protocol implementation file?

[winlinvip]

Winlin: There might be an issue in rtpenc.c, where the RTP marker for STAP-A should be set to 0, but it is currently set to 1. This problem leads to Chrome's inability to decode the video stream. It is important to note that STAP-A includes SPS/PPS. Therefore, we need to define this macro to parse the RTP packet and fix the issue.

    /**
     * For video, the STAP-A with SPS/PPS should:
     * 1. The marker bit should be 0, never be 1.
     * 2. The NRI should equal to the first NALU's.
     * TODO: FIXME: Should fix it in rtpenc.c
     */
    if (is_video && buf_size > 12) {
        nalu_header = buf[12] & 0x1f;
        if (nalu_header == NALU_TYPE_STAP_A) {
            /* Reset the marker bit to 0. */
            if (buf[1] & 0x80)
                buf[1] &= 0x7f;

            /* Reset the NRI to the first NALU's NRI. */
            if (buf_size > 15 && (buf[15]&0x60) != (buf[12]&0x60))
                buf[12] = (buf[12]&0x80) | (buf[15]&0x60) | (buf[12]&0x1f);
        }
    }

After utilizing BSF, the marker is correctly set to false, so there is no need to reset it. Regarding the NRI, it appears to be functioning properly and does not require resetting at this time. Thus, I eliminated this code and macro.

在rtpenc.c中可能有个bug，其中STAP-A的RTP marker应设置为0，但当前设置为1。这个问题导致Chrome无法解码视频流。值得注意的是，STAP-A包括SPS/PPS。因此，我们需要定义这个宏来解析RTP数据包并解决这个问题。在使用BSF后，这个Marker是正确的，不需要重置了；同时，最新测试发现，NRI也不需要重置。

Fixed.

[winlinvip]

Derek Buitenhuis:

> +#define ICE_PROCESSING_TIMEOUT 10

Are we really hardcoding sleeps? Surely a better way exists. What do other implementations
do here?

[winlinvip]

Winlin: We have improved the ICE and DTLS code, and removed this macro as it is no longer necessary. Currently, only a handshake_timeout option is utilized, with a default value of 5 seconds, to restrict the ICE and DTLS timeout duration.

我们已经优化了ICE和DTLS代码，并且移除了这个不再需要的宏。目前，只使用一个名为handshake_timeout的选项（默认值为5秒）来限制ICE和DTLS的超时时长。

Fixed.

[winlinvip]

Derek Buitenhuis:

> +#define DTLS_EAGAIN_RETRIES_MAX 5

Should this be an option?

[winlinvip]

Winlin: We have improved the ICE and DTLS code, and removed this macro as it is no longer necessary. Currently, only a handshake_timeout option is utilized, with a default value of 5 seconds, to restrict the ICE and DTLS timeout duration.

我们已经优化了ICE和DTLS代码，并且移除了这个不再需要的宏。目前，只使用一个名为handshake_timeout的选项（默认值为5秒）来限制ICE和DTLS的超时时长。

Fixed.

[winlinvip]

Derek Buitenhuis:

> +#define DTLS_SSL_TIMER_BASE 400 * 1000

Is this from a spec or arbitrary?

[winlinvip]

Winlin: We have improved the ICE and DTLS code, and removed this macro as it is no longer necessary. Currently, only a handshake_timeout option is utilized, with a default value of 5 seconds, to restrict the ICE and DTLS timeout duration.

我们已经优化了ICE和DTLS代码，并且移除了这个不再需要的宏。目前，只使用一个名为handshake_timeout的选项（默认值为5秒）来限制ICE和DTLS的超时时长。

Fixed.

[winlinvip]

Derek Buitenhuis:

> +typedef struct DTLSContext {
> +    /* For av_log to write log to this category. */
> +    void *log_avcl;

Why is this void *?

[winlinvip]

Winlin: The log_avcl is utilized for av_log(void* avcl), where the first argument is of void* type. We have change the void* to AVClass*.

    RTCContext *rtc = s->priv_data;
    rtc->dtls_ctx.av_class = rtc->av_class;

    DTLSContext *ctx;
    av_log(ctx, AV_LOG_ERROR, "DTLS: Unable to generate private key, EC_KEY_set_group failure\n");

Fixed.

[JackLau1222]

Jack Lau: In latest code, the log_avcl is removed, because DTLS has its own class.

[winlinvip]

Derek Buitenhuis:

> +    uint8_t dtls_srtp_material[DTLS_SRTP_MASTER_KEY_LEN * 2];

*2?

[winlinvip]

Winlin: The dtls_srtp_materials consist of pairs of keys and salts, with each pair containing a key and a salt, such as the client key and salt. The layout is as follows:

     16B         16B         14B             14B
client_key | server_key | client_salt | server_salt

That's the reason for multiplying it by 2. However, we have refined it to make it clear.

// This material consists of a key of 16 bytes and a salt of 14 bytes.
#define DTLS_SRTP_MASTER_KEY_LEN 30
uint8_t dtls_srtp_materials[DTLS_SRTP_MASTER_KEY_LEN];

Refined to:

#define DTLS_SRTP_KEY_LEN 16
#define DTLS_SRTP_SALT_LEN 14
/**
 * This represents the material used to build the SRTP master key. It is
 * generated by DTLS and has the following layout:
 *          16B         16B         14B             14B
 *      client_key | server_key | client_salt | server_salt
 */
uint8_t dtls_srtp_materials[(DTLS_SRTP_KEY_LEN + DTLS_SRTP_SALT_LEN) * 2];

Fixed.

[winlinvip]

Derek Buitenhuis:

> +/**
> + * Generate a self-signed certificate and private key for DTLS.
> + */

Does WHIP allow use of arbitrary certs (i.e. non-self-signed)?

[winlinvip]

Winlin: Supported both self-signed and non-self-signed certificates.

[T-bagwell]

证书可以通过增加一个 option 来指定不

[winlinvip]

可以加一个option，是字符串，指定的是用户的证书和私钥文件的路径。参考：

We can include an option as a string that denotes the path to the certificate and key files, similar to:

    {"hls_key_info_file",    "file with key URI and key file path", OFFSET(key_info_file),      AV_OPT_TYPE_STRING, {.str = NULL},            0,       0,         E},

[winlinvip]

Fixed by 55228b3

[winlinvip]

Derek Buitenhuis:

> +    unsigned char md[EVP_MAX_MD_SIZE];
> +    const char *aor = "[ffmpeg.org](http://ffmpeg.org/)", *curve = NULL;

I am not sure the the FFmpeg community wants to advertise itself as a CN.

Mayb lavf?

[winlinvip]

Which CN(Common Name) should we use?

[T-bagwell]

应该是可以加一个 option 来让用户自己指定，然后默认是一个dummy.localhost?

[winlinvip]

这个只有生成的自签名证书才需要指定这个CommonName，如果是通过文件导入的CA签发的证书是不需要指定的，所以感觉这个弄个option真的没啥用，它也不会显示在什么窗口上，只是DTLS中交换密钥使用到了。

This CommonName only needs to be specified when generating a self-signed certificate. If the certificate is issued by a CA through file import, there is no need to specify it. So, it seems that having this option is not really useful. It won't be displayed in any window, and is only used in the DTLS key exchange.

[winlinvip]

Change the CommonName from ffmpeg.org to lavf.

Fixed

[winlinvip]

Derek Buitenhuis:

const char *aor = "ffmpeg.org", *curve = NULL;
curve = "prime256v1";

For this and all other occurrences: They should be intialized at declaration
time, like the rest of the codebase.

[winlinvip]

Fixed.

[winlinvip]

Derek Buitenhuis:

> +    ctx->dtls_pkey = dtls_pkey = EVP_EC_gen(curve);
> +    if (!dtls_pkey) {
> +        av_log(s1, AV_LOG_ERROR, "DTLS: EVP_EC_gen curve=%s failed\n", curve);

For this and all ther logs: Something more descriptive than "function_name failed:"
should be used.

[winlinvip]

Winlin: I have included additional information in the log message, beyond just the failed function name.

Fixed.

[winlinvip]

Derek Buitenhuis:

> +    serial = (int)av_get_random_seed();

For all uses of av_get_random_seed: Why are we using a seed and not a PRNG?

[winlinvip]

Winlin: I have looked through the code related to PRNG, but I still don't understand it. Can you please explain which API I should use?

[T-bagwell]

看上去他的意思是可以尝试使用libavutil/lfg.h里面的接口

[winlinvip]

Got it! Replace av_get_random_seed by AVLFG.

Fixed.

[winlinvip]

Derek Buitenhuis:

> +    rtc->audio_payload_type = 111;
> +    rtc->video_payload_type = 106;

Magic numbers. I assume this is H.264 and Opus? Do not harcode.

[winlinvip]

Winlin: Replace by macros.

/* Referring to Chrome's definition of RTP payload types. */
#define RTC_RTP_PAYLOAD_TYPE_H264 106
#define RTC_RTP_PAYLOAD_TYPE_OPUS 111

Fixed.

[winlinvip]

Derek Buitenhuis:

> +
> +    av_bprintf(&bp, ""
> +        "v=0\r\n"
> +        "o=FFmpeg 4489045141692799359 2 IN IP4 127.0.0.1\r\n"

Hardcoding 127.0.0.1?

4489045141692799359?

[winlinvip]

Winlin: In the case of ICE-LITE, these fields are not used; instead, they are defined as constant values.

/**
 * In the case of ICE-LITE, these fields are not used; instead, they are defined
 * as constant values.
 */
#define RTC_SDP_SESSION_ID "4489045141692799359"
#define RTC_SDP_CREATOR_IP "127.0.0.1"

Fixed.

[winlinvip]

Derek Buitenhuis:

> +    if (rtc->video_par) {
> +        profile = rtc->video_par->profile < 0 ? 0x42 : rtc->video_par->profile;
> +        level = rtc->video_par->level < 0 ? 30 : rtc->video_par->level;
> +        profile_iop = profile & FF_PROFILE_H264_CONSTRAINED;

Do not hardcode H264.

[winlinvip]

Winlin: Fixed.

[winlinvip]

Derek Buitenhuis:

> +            "a=rtpmap:%u H264/90000\r\n"

Do not hardcode H264.

[winlinvip]

Winlin: Fixed.

[winlinvip]

Derek Buitenhuis:

We have SDP facilities already.

[winlinvip]

Winlin: I have experimented with the SDP functionality in FFmpeg by utilizing the av_sdp_create function. I discovered that the SDP generated by av_sdp_create is tailored for RTSP, which significantly differs from WebRTC. Below is a comparison of the test code and the SDP.

    int i;
    char url[MAX_URL_SIZE];
    char sdp[MAX_SDP_SIZE];
    AVFormatContext sdp_ctx, *ctx_array[1];
    sdp_ctx = *s;
    sdp_ctx.url = url;
    for (i = 0; i < sdp_ctx.nb_streams; i++) {
        /* Exclude the stream's bitrate information to prevent writing a line in the SDP like b=AS:96. */
        sdp_ctx.streams[i]->codecpar->bit_rate = 0;
        /* Do not write a sprop-stereo=1 line. */
        if (sdp_ctx.streams[i]->codecpar->codec_id == AV_CODEC_ID_OPUS)
            sdp_ctx.streams[i]->codecpar->ch_layout.nb_channels = 1;
    }
    ff_url_join(url, sizeof(url), "rtp", NULL, "0.0.0.0", 9, NULL);
    ctx_array[0] = &sdp_ctx;
    if (av_sdp_create(ctx_array, 1, sdp, MAX_SDP_SIZE)) {
        av_free(sdp);
        return AVERROR_INVALIDDATA;
    }
    av_log(s, AV_LOG_INFO, "SDP:\n%s\n", sdp);

Below is the SDP produced by av_sdp_create for RTSP.

v=0
o=- 0 0 IN IP4 127.0.0.1
s=No Name
c=IN IP4 0.0.0.0
t=0 0
a=tool:libavformat 60.5.100
m=video 9 RTP/AVP 96
a=rtpmap:96 H264/90000
a=fmtp:96 packetization-mode=1; sprop-parameter-sets=Z0LAHtkAwCmwEQAAAwABAAADADIPFi5I,aMuDyyA=; profile-level-id=42C01E
m=audio 11 RTP/AVP 97
a=rtpmap:97 opus/48000/2

Below is the SDP produced by WHIP muxer for WebRTC.

v=0
o=FFmpeg 4489045141692799359 2 IN IP4 127.0.0.1
s=FFmpegPublishSession
t=0 0
a=group:BUNDLE 0 1
a=extmap-allow-mixed
a=msid-semantic: WMS
m=audio 9 UDP/TLS/RTP/SAVPF 111
c=IN IP4 0.0.0.0
a=ice-ufrag:f07a1195
a=ice-pwd:5c4b3fe3a7ffed9262c712fdc77fe232
a=fingerprint:sha-256 CD:EE:CE:45:4C:D7:3C:57:7B:79:38:7F:83:3E:3D:2F:3E:38:92:87:EC:57:4C:6D:21:26:97:B8:FA:58:EC:E1
a=setup:passive
a=mid:0
a=sendonly
a=msid:FFmpeg audio
a=rtcp-mux
a=rtpmap:111 opus/48000/1
a=ssrc:3591919697 cname:FFmpeg
a=ssrc:3591919697 msid:FFmpeg audio
m=video 9 UDP/TLS/RTP/SAVPF 106
c=IN IP4 0.0.0.0
a=ice-ufrag:f07a1195
a=ice-pwd:5c4b3fe3a7ffed9262c712fdc77fe232
a=fingerprint:sha-256 CD:EE:CE:45:4C:D7:3C:57:7B:79:38:7F:83:3E:3D:2F:3E:38:92:87:EC:57:4C:6D:21:26:97:B8:FA:58:EC:E1
a=setup:passive
a=mid:1
a=sendonly
a=msid:FFmpeg video
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:106 H264/90000
a=fmtp:106 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001e
a=ssrc:1300609085 cname:FFmpeg
a=ssrc:1300609085 msid:FFmpeg video

The primary distinction is outlined below:

-	RTSP	WHIP Muxer	Description
connection	c=IN IP4 0.0.0.0	c=IN IP4 0.0.0.0	RTSP use global connection. WHIP use it for each m line.
m line	m=video 9 RTP/AVP 96	m=video 9 UDP/TLS/RTP/SAVPF 106	The protocols are difference.
m line	m=audio 11 RTP/AVP 97	m=audio 9 UDP/TLS/RTP/SAVPF 111	RTSP has a port 11, while WHIP ignores port.
ssrc	-	a=ssrc:3591919697 cname:FFmpeg	No SSRC in RTSP.
dtls	-	a=setup:passive	No DTLS role in RTSP.
stream id	-	a=mid:1	No stream id in RTSP.
direction	-	a=sendonly	No stream direction in RTSP.
rtcp	-	a=rtcp-mux	No rtcp pamameters in RTSP.
ice	-	a=ice-ufrag:f07a1195	No ICE config in RTSP.

The difference is significant, and it is highly recommended to improve the SDP functionality in the future once this patch has been merged.

Need your help to evaluate. @T-bagwell

[T-bagwell]

看了一下，应该是可以尝试给av_sdp_create增加参数来达到一样的效果。

[winlinvip]

Winlin: I attempted to utilize av_sdp_create for SDP creation, but it required extensive modification of the existing code, making it difficult to maintain due to the significant differences. Therefore, it is recommended to improve this approach in the future. See WHIP: Use av_sdp_create to create SDP. for detail.

我尝试使用av_sdp_create来创建SDP，但这需要对现有代码进行大量修改，由于差异很大，使得难以维护。因此，建议在未来改进SDP部分代码。需要做的修改请参考：WHIP: Use av_sdp_create to create SDP.

[winlinvip]

Derek Buitenhuis:

> +    av_opt_set(rtc->udp_uc->priv_data, "connect", "1", 0);
> +    av_opt_set(rtc->udp_uc->priv_data, "fifo_size", "0", 0);
> +    /* Set the max packet size to the buffer size. */
> +    snprintf(tmp, sizeof(tmp), "%d", rtc->pkt_size);
> +    av_opt_set(rtc->udp_uc->priv_data, "pkt_size", tmp, 0);

This is as good of a place as any to ask: Do we plan to handle passing
AVOptions down to subcontexts?

[winlinvip]

Winlin: Fixed by WHIP: Use options for passing parameters to the HTTP or UDP context.

[T-bagwell]

这个是不是采用 Derek 的意见会比较靠谱一些

[winlinvip]

Fixed.

[winlinvip]

Derek Buitenhuis:

> +    avio_wb32(pb, av_get_random_seed()); /* transaction ID */

Use the PRNG.

[winlinvip]

Winlin: Dup to #5 (comment)

[winlinvip]

Derek Buitenhuis:

> +    const char* suite = "AES_CM_128_HMAC_SHA1_80";

Source?

Same as before, re: subcontext AVOptions?

[winlinvip]

Winlin: Dup to #5 (comment)

Fixed.

[winlinvip]

Derek Buitenhuis:

> +    /* Ignore if not RTP or RTCP packet. */
> +    if (buf_size < 12 || (buf[0] & 0xC0) != 0x80)
> +        return 0;
> +
> +    /* Only support audio, video and rtcp. */
> +    is_rtcp = buf[1] >= 192 && buf[1] <= 223;

Magic numbers.

[winlinvip]

Winlin: Replace the magic number with a macro for both RTP and RTCP payload types.

/**
 * For RTCP, PT is [128, 223] (or without marker [0, 95]). Literally, RTCP starts
 * from 64 not 0, so PT is [192, 223] (or without marker [64, 95]), see "RTCP Control
 * Packet Types (PT)" at
 * https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml#rtp-parameters-4
 *
 * For RTP, the PT is [96, 127], or [224, 255] with marker. See "RTP Payload Types (PT)
 * for standard audio and video encodings" at
 * https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml#rtp-parameters-1
 */
#define RTC_RTCP_PT_START 192
#define RTC_RTCP_PT_END   223

Fixed.

[winlinvip]

Derek Buitenhuis:

> +    /**
> +     * For video, the STAP-A with SPS/PPS should:
> +     * 1. The marker bit should be 0, never be 1.
> +     * 2. The NRI should equal to the first NALU's.
> +     */
> +    if (is_video && buf_size > 12) {
> +        nalu_header = buf[12] & 0x1f;
> +        if (nalu_header == NALU_TYPE_STAP_A) {
> +            /* Reset the marker bit to 0. */
> +            if (buf[1] & 0x80)
> +                buf[1] &= 0x7f;
> +
> +            /* Reset the NRI to the first NALU's NRI. */
> +            if (buf_size > 15 && (buf[15]&0x60) != (buf[12]&0x60))
> +                buf[12] = (buf[12]&0x80) | (buf[15]&0x60) | (buf[12]&0x1f);
> +        }
> +    }

This seems hardcoded to H.264.

[winlinvip]

Winlin: Dup to #5 (comment)

Fixed.

[winlinvip]

Derek Buitenhuis:

> +/**
> + * Inserts the SPS/PPS data before each IDR (Instantaneous Decoder Refresh) frame.
> + *
> + * The SPS/PPS is parsed from the extradata. If it's in ISOM format, the SPS/PPS is
> + * multiplexed to the data field of the packet. If it's in annexb format, then the entire
> + * extradata is set to the data field of the packet.

Why do we do it different ways instea of always munging it into the same way?

Also, do we have a BSF for this?

[winlinvip]

Winlin: It varies depending on the situation. When copying, the packet is in the MP4/ISOM format. In this case, use the h264_mp4toannexb BSF and eliminate this function. However, when encoding, the packet is in the ANNEXB format, and it becomes essential to manually insert encoder metadata before each IDR while handling annexb format packets.

ISOM

When transcoding then copying, the packet is in the MP4/ISOM format.

~/git/FFmpeg/ffmpeg_g -re -i ~/git/srs/trunk/doc/source.flv \
    -vcodec libx264 -profile:v baseline -r 25 -g 50 -acodec libopus -ar 48000 -ac 2 \
    -y t.mp4

~/git/FFmpeg/ffmpeg_g -re -i t.mp4 \
    -f rtc "http://localhost:1985/rtc/v1/whip/?app=live&stream=livestream"

Utilize the h264_mp4toannexb BSF and remove this function.

const FFOutputFormat ff_rtc_muxer = {
    .p.name             = "rtc",
    /**
     * Avoid using AVFMT_GLOBALHEADER, for annexb format, it's necessary for the
     * encoder to insert metadata headers (e.g., SPS/PPS for H.264) before each
     * IDR frame.
     */
    .p.flags            = AVFMT_NOFILE,
    .check_bitstream    = rtc_check_bitstream,

static int rtc_check_bitstream(AVFormatContext *s, AVStream *st, const AVPacket *pkt) {
    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
                             (AV_RB24(pkt->data) != 0x000001 ||
                              (st->codecpar->extradata_size > 0 &&
                               st->codecpar->extradata[0] == 1)))
            ret = ff_stream_add_bitstream_filter(st, "h264_mp4toannexb", NULL);

Fixed by winlinvip@6598ffc

Keep in mind that if the packet is already in annexb format, this BSF will bypass the packet:

static int h264_mp4toannexb_init(AVBSFContext *ctx)
    if (!extra_size                                               ||
        (extra_size >= 3 && AV_RB24(ctx->par_in->extradata) == 1) ||
        (extra_size >= 4 && AV_RB32(ctx->par_in->extradata) == 1)) {
        av_log(ctx, AV_LOG_VERBOSE,
               "The input looks like it is Annex B already\n");

// s->extradata_parsed is 0.
static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
    /* nothing to filter */
    if (!s->extradata_parsed) {
        av_packet_move_ref(opkt, in);
        av_packet_free(&in);
        return 0;
    }

In this scenario, we must manually insert the SPS/PPS.

ANNEXB

When encoding, the packet is in the ANNEXB format.

~/git/FFmpeg/ffmpeg_g -re -i ~/git/srs/trunk/doc/source.flv \
    -vcodec libx264 -profile:v baseline -r 25 -g 50 -acodec libopus -ar 48000 -ac 2 \
    -f rtc "http://localhost:1985/rtc/v1/whip/?app=live&stream=livestream"

Since the h264_mp4toannexb filter only processes the MP4 ISOM format and bypasses the annexb format, it is necessary to manually insert encoder metadata before each IDR when dealing with annexb format packets. For instance, in the case of H.264, we must insert SPS and PPS before the IDR frame.

static int rtc_check_bitstream(AVFormatContext *s, AVStream *st, const AVPacket *pkt) {
    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
        extradata_isom = st->codecpar->extradata_size > 0 && st->codecpar->extradata[0] == 1;
        if (pkt->size >= 5 && AV_RB32(b) != 0x0000001 && (AV_RB24(b) != 0x000001 || extradata_isom))
            ret = ff_stream_add_bitstream_filter(st, "h264_mp4toannexb", NULL);
        else
            rtc->h264_annexb_insert_sps_pps = 1;
    }

static int rtc_write_packet(AVFormatContext *s, AVPacket *pkt) {
    if (rtc->h264_annexb_insert_sps_pps && st->codecpar->codec_id == AV_CODEC_ID_H264) {
        if ((ret = h264_annexb_insert_sps_pps(s, pkt)) < 0) {

/**
 * Since the h264_mp4toannexb filter only processes the MP4 ISOM format and bypasses
 * the annexb format, it is necessary to manually insert encoder metadata before each
 * IDR when dealing with annexb format packets. For instance, in the case of H.264,
 * we must insert SPS and PPS before the IDR frame.
 */
static int h264_annexb_insert_sps_pps(AVFormatContext *s, AVPacket *pkt)

See 981a098

Fixed.

[winlinvip]

Derek Buitenhuis:

> +    /* For audio OPUS stream, correct the timestamp. */
> +    if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
> +        pkt->dts = pkt->pts = rtc->audio_jitter_base;
> +        rtc->audio_jitter_base += 960;
> +    }

1. Do not hardcode opus.
2. 960 is not always true for opus, it just happens to be the libopus default.

[winlinvip]

Winlin: Dup to #4 (comment)

Fixed.

[winlinvip]

Derek Buitenhuis:

> +        av_log(s, AV_LOG_ERROR, "Failed to insert SPS/PPS packet\n");
> +        return ret;
> +    }

Do not hardcode H264.

[winlinvip]

Winlin: Refine the comments to:

        av_log(rtc, AV_LOG_ERROR, "WHIP: Failed to insert codec metadata packet\n");

Fixed.

[winlinvip]

Derek Buitenhuis:

> +static const AVClass rtc_muxer_class = {
> +    .class_name = "WebRTC muxer",

WHIP or WebRTC?

[winlinvip]

Winlin: Rename to WHIP muxer

static const AVClass rtc_muxer_class = {
    .class_name = "WHIP muxer",

const FFOutputFormat ff_rtc_muxer = {
    .p.long_name        = NULL_IF_CONFIG_SMALL("WHIP(WebRTC-HTTP ingestion protocol) muxer"),

Fixed.

[winlinvip]

Derek Buitenhuis:

This patch, to me, seems almost like a proof of concept rather than a fully fleshed out
(read: pushable) patch. I have few concerns:

1. If it is pushed in such a state, lacking so many things, my hunch, based on experience,
   is that it will stay that way: incomplete, just being maintained, rather than finished.
   Users will rightfully ask why it is like this, and will ask for it to be completed, but
   I expect nobody will. But there will be a blog post about how "FFmpeg has WebRTC now",
   like we saw with IPFS.
2. It is monolithic - WHIP, DTLS, ICE, STUN, SRTP, RTP, and SPD should really all be separate
   layers. Typing that out really does make one realize WebRTC suffers from deep IETF
   committee-ness...
3. The codec parts should probably be separated out like they are for RTP.
4. If our SDP facilities are not good enough for WHIP, they should be improved so that they
   are.

[winlinvip]

Winlin: This patch goes beyond being a mere prototype for proof of concept; it is a fully functional patch. Since WHIP is significantly simpler and only a subset of WebRTC, there is no need to implement the entire WebRTC stack, making this patch truly workable.

At present, we only have support for OpenSSL, but adding support for other TLS libraries like GnuTLS is not difficult. To maintain the simplicity of this patch, we suggest incorporating this additional support in the future.

I believe that extracting WHIP, DTLS, ICE, and STUN into separate layers may not be necessary, as it represents a complete WebRTC stack. WHIP involves a single HTTP request and response, so creating an additional API layer is not required. Similarly, for ICE and STUN, we utilize ICE-LITE mode instead of ICE-FULL mode, which consists of a straightforward UDP binding request and response, making an extra API layer redundant. However, for DTLS, it might be beneficial to introduce an API layer. Regarding SRTP and RTP, FFmpeg already provides an API layer, which only needs some refinement.

The issue with the SDP is somewhat complicated, as the WHIP's SDP is quite straightforward, requiring only the generation of a single SDP. However, if we want to utilize ffmpeg's SDP API, it becomes more complex and necessitates code modifications to function properly. I suggest addressing this in a future update rather than in the current patch.

Currently, we only support H.264 and OPUS codecs, but we may need to add AV1 support in the future. Incorporating a new codec is straightforward; it simply requires updating the SDP and codec metadata, such as the H.264's SPS/PPS.

这个patch不仅仅是一个用于概念验证的原型；它是一个功能完善的patch。由于 WHIP 是比较简化和简单的，且仅为 WebRTC 的子集，因此无需实现整个 WebRTC 技术栈，这使得这个patch是真正可用于线上环境。

目前，我们只支持 OpenSSL，但添加对其他 TLS 库（如 GnuTLS）的支持并不困难。为保持这个patch的简单容易维护，我们建议在未来加入这些额外支持。

我认为将 WHIP、DTLS、ICE 和 STUN 分离成单独的层可能没有必要，因为这些分层往往是实现了一个完整的 WebRTC 堆栈。WHIP 只涉及单个 HTTP 请求和响应，因此无需抽象成单独的一层API。类似地，对于 ICE 和 STUN，我们使用 ICE-LITE 模式而不是 ICE-FULL 模式，它也只需要简单的 UDP binding request和response，所以要定义为单独的一层就有点多余了。然而，对于 DTLS，引入独立的一层 API 可能比较合理。关于 SRTP 和 RTP，FFmpeg 已经抽象了一层并有现有的API，只需要进行一些改进。

SDP 的问题有点复杂，因为 WHIP 的 SDP 相当简单，只需要生成一个 SDP。然而，如果我们想使用 ffmpeg 的 SDP API，它变得更加复杂并需要代码修改才能正常工作。我建议在未来的更新中解决这个问题，而不是在当前的patch中解决。

目前，我们只支持 H.264 和 OPUS 编解码器，但未来可能需要添加 AV1 支持。添加新编解码器非常简单；只需更新 SDP 和编解码器元数据，例如 H.264 的 SPS/PPS。

[JackLau1222]

Jack Lau: Since I refactored the DTLS and merged it into tls, we can easily add support for other tls libs.