CR #1: Rountrip0: Comments from Steven Liu

[winlinvip]

Review for #1

[winlinvip]

uint32_t audio_ssrc;
uint32_t video_ssrc;
/* The PT(Payload Type) of stream, generated by the muxer. */
uint8_t audio_payload_type;
uint8_t video_payload_type;

还有这四个，移动到generate_sdp_offer里面

[winlinvip]

Winlin: 这个是SSRC和PT，RTP发包时会作为包的字段。

[winlinvip]

RTCContext *rtc = s->priv_data;

pb = avio_alloc_context(
    (unsigned char *)rtc->sdp_answer, (int)strlen(rtc->sdp_answer),

没必要强行转换，是不是在声明变量的时候就可以unsigned char *, 后边的strlen也一样，不需要要强行int

[winlinvip]

Winlin: 收到，我改下。

[winlinvip]

av_freep(&rtc->ice_ufrag_remote);
rtc->ice_ufrag_remote = av_strdup(ptr);
} else if (av_strstart(line, "a=ice-pwd:", &ptr)) {
av_freep(&rtc->ice_pwd_remote);

这里的几个有必要av_freep先释放不？

[winlinvip]

Winlin: 我判断下不为空再赋值吧，ice的用户名和密码可能会有多个，比如音频和视频各一个，但他们应该相同：

 *       m=video 9 UDP/TLS/RTP/SAVPF 106
 *       a=ice-ufrag:a174B
 *       a=ice-pwd:wY8rJ3gNLxL3eWZs6UPOxy
 * 
 *       m=audio 9 UDP/TLS/RTP/SAVPF 111
 *       c=IN IP4 0.0.0.0
 *       a=ice-ufrag:a174B
 *       a=ice-pwd:wY8rJ3gNLxL3eWZs6UPOxy

[winlinvip]

av_freep(&rtc->ice_protocol);
rtc->ice_protocol = av_strdup(protocol);
av_freep(&rtc->ice_host);

这些av_freep是不是可以提成一个函数，然后在进入这个函数后第一步先清理一下。

[winlinvip]

Winlin: 这个和前面一样，可能会有多个candidate，比如：

 *       m=audio 9 UDP/TLS/RTP/SAVPF 111
 *       a=candidate:0 1 udp 2130706431 172.20.10.7 8000 typ host generation 0
 * 
 *       m=video 9 UDP/TLS/RTP/SAVPF 106
 *       a=candidate:0 1 udp 2130706431 172.20.10.7 8000 typ host generation 0

可以只解析第一个，这样就不用释放。不过这块未来可能需要支持多个candidate，这样可以同时探测，类似多路径。

[winlinvip]

char ice_ufrag_local[9];
char ice_pwd_local[33];

这两个变量放到generate_sdp_offer函数里面作为局部变量是不是也可以？

[winlinvip]

Winlin: 这个是客户端本地的用户名和密码，在ice握手时，会和服务器的数据签名包，所以后面还会用到。

[winlinvip]

av_freep(&rtc->ice_protocol);
rtc->ice_protocol = av_strdup(protocol);
av_freep(&rtc->ice_host);
rtc->ice_host = av_strdup(host);

添加一个if (!rtc->ice_host) 的mem alloc判断，如果异常的话需要释放掉前面申请的所有资源，并且返回ENOMEM

[winlinvip]

Winlin: 这个和前面一样，可能会有多个candidate，我改成只取第一个，这样就不必释放了。

[winlinvip]

static int rtc_write_trailer(AVFormatContext *s)
{
return 0;
}

Will this function be used?

[winlinvip]

Winlin: Got it.

In rtc_write_trailer, I request server to close the stream, by sending a HTTP DELETE request. This is also defined in WHIP protocol:

   To explicitly terminate a session, the WHIP client MUST perform an
   HTTP DELETE request to the resource URL returned in the Location
   header field of the initial HTTP POST.  Upon receiving the HTTP
   DELETE request, the WHIP resource will be removed and the resources
   freed on the Media Server, terminating the ICE and DTLS sessions.

This is because UDP is connectionless, so it use timeout to detect client unpublish event. WHIP defines this DELETE message to let server to know the disconnect fastly, which allows client to republish stream fastly.

See 4d564c8

[winlinvip]

static const AVOption options[] = {
    { NULL },
};

#define MAX_SDP_SIZE 8192
#define MAX_UDP_SIZE 1500
#define MAX_UDP_PAYLOAD_SIZE (MAX_UDP_SIZE - 300)
#define UDP_FAST_RETRIES 6
#define UDP_START_TIMEOUT 21
#define DTLS_FRAGMENT_MAX_SIZE 1200
#define DTLS_CIPHER_SUTES "ECDHE-ECDSA-AES128-GCM-SHA256" 
#define DTLS_SRTP_MASTER_KEY_LEN 30
#define NALU_TYPE_STAP_A 24

can these controlled by options?

[winlinvip]

Winlin: If the value may change in different use scenario, it should be an option. For a constant value that never changes, it should be defined by macro, rather than an option.

The bellow values literally never change or user doesn't know how to set it:

1. #define MAX_SDP_SIZE 8192: It never changes and 8k is big enough. Rather than an option, it should be a larger value if not enough.
2. #define DTLS_CIPHER_SUTES ECDHE-ECDSA-AES128-GCM-SHA256: Depends on the ability of ssl library, user doesn't know how to set it.
3. #define DTLS_SRTP_MASTER_KEY_LEN 30: Never changes.
4. #define NALU_TYPE_STAP_A 24: Never changes.

The MTU and packet size should be set by option pkt_size like RTP muxer:

1. #define MAX_UDP_SIZE 1500: Generally, the network MTU is 1500, but it may be smaller for some special and legacy network, so it should be an option, rather than a macro.
2. #define UDP_FAST_RETRIES 6: Should be an option because user may change it to retry more times.

The bellow values should be FFmpeg options because it may change:

1. #define UDP_START_TIMEOUT 21: Should be an option because user may change it to use larger timeout.
2. #define DTLS_FRAGMENT_MAX_SIZE 1200: Should be an option for it's similar to MAX_UDP_SIZE.

Fixed by 58ac81e

[winlinvip]

 }
    }

    rtc->sdp_answer = av_strdup(tmp);

add memory alloc failed check and give failed message when it failed.
other code block for memory alloc should have same operation.

[winlinvip]

Roger that. All similar issues are fixed by e5bbc91

[winlinvip]

    av_log(s, AV_LOG_INFO, "WHIP: SDP offer=%luB, answer=%luB, ufrag=%s, pwd=%luB, transport=%s://%s:%d\n",
        strlen(rtc->sdp_offer), strlen(rtc->sdp_answer), rtc->ice_ufrag_remote, strlen(rtc->ice_pwd_remote),
        rtc->ice_protocol, rtc->ice_host, rtc->ice_port);

[rtc @ 0x6170000cda80] WHIP: Init ice_arq_max=5, ice_arq_timeout=30, dtls_arq_max=5, dtls_arq_timeout=50 pkt_size=1500
AddressSanitizer:DEADLYSIGNAL
=================================================================
==29537==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7ff819e16572 bp 0x7ff7b65e7b20 sp 0x7ff7b65e7b20 T0)
==29537==The signal is caused by a READ memory access.
==29537==Hint: address points to the zero page.
    #0 0x7ff819e16572 in _platform_strlen+0x12 (libsystem_platform.dylib:x86_64+0x1572) (BuildId: f314b62b98f43a7c82968739f8b6855a240000001000000000010d0000010d00)
    #1 0x10f69e4c7 in wrap_strlen+0x37 (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x194c7) (BuildId: 756bb7515781379f84412f22c4274ffd2400000010000000000a0a0000030d00)
    #2 0x109fb95bc in parse_answer rtcenc.c:677
    #3 0x10b8a0193 in rtc_init rtcenc.c:1559
    #4 0x109f64522 in init_muxer mux.c:350
    #5 0x109f63ac3 in avformat_init_output mux.c:438
    #6 0x109f64857 in avformat_write_header mux.c:462
    #7 0x10992e30f in mux_check_init ffmpeg_mux.c:590
    #8 0x10992ea61 in of_stream_init ffmpeg_mux.c:670
    #9 0x10992390e in enc_open ffmpeg_enc.c:421
    #10 0x109924876 in enc_frame ffmpeg_enc.c:1109
    #11 0x109928bb1 in reap_filters ffmpeg_filter.c:1449
    #12 0x1099526de in transcode_step ffmpeg.c:2109
    #13 0x109950f74 in transcode ffmpeg.c:2145
    #14 0x109950ab2 in main ffmpeg.c:2260
    #15 0x7ff819abb30f in start+0x97f (dyld:x86_64+0xfffffffffff7230f) (BuildId: bb7a09708c623dcea7a25cec9c501f1132000000200000000100000000010d00)

==29537==Register values:
rax = 0x000000011010827c  rbx = 0x000000010cb2d500  rcx = 0x0000000000000000  rdx = 0x0000000000000000
rdi = 0x0000000000000000  rsi = 0x00001c1c00009bf0  rbp = 0x00007ff7b65e7b20  rsp = 0x00007ff7b65e7b20
 r8 = 0x00001c1c00009bf7   r9 = 0x0000000000000004  r10 = 0x00000c1c00009be4  r11 = 0x00001c1c00009bf7
r12 = 0x000000010cb2eda0  r13 = 0x0000000000000000  r14 = 0x000000010cb2ed80  r15 = 0x0000000109950940
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (libsystem_platform.dylib:x86_64+0x1572) (BuildId: f314b62b98f43a7c82968739f8b6855a240000001000000000010d0000010d00) in _platform_strlen+0x12
==29537==ABORTING
Abort trap: 6

[winlinvip]

Winlin: I didn't manage to fix the issue by recompiling FFmpeg with AddressSanitizer, but I have modified the code to include a check for null strings and return an error if one is detected, which should prevent this problem from occurring.

See 067c883

[winlinvip]

@@ -493,6 +493,7 @@ OBJS-$(CONFIG_RSD_DEMUXER)               += rsd.o
OBJS-$(CONFIG_RPL_DEMUXER)               += rpl.o
OBJS-$(CONFIG_RSO_DEMUXER)               += rsodec.o rso.o pcm.o
OBJS-$(CONFIG_RSO_MUXER)                 += rsoenc.o rso.o rawenc.o
OBJS-$(CONFIG_RTC_MUXER)                 += rtcenc.o avc.o

should add http.o if rtcenc.o will use http.o functions.

[winlinvip]

Winlin: Add http.o to the list of dependencies, and also add srtp.o.

[winlinvip]

    if (!extradata || !extradata_size)
        return 0;

    /* Not H.264 IOSM format, may be annexb etc. */

typo?

[winlinvip]

Winlin: Fixed

[winlinvip]

        goto end;
    }

    rtc->avc_sps = av_malloc(rtc->avc_sps_size);

memory alloc check

[winlinvip]

Winlin: Fixed, and similar issues were also fixed.

[winlinvip]

        goto end;
    }

end:

Should av_freep(&rtc->avc_sps) here?

[winlinvip]

Winlin: No, it will not be freed in the rtc_deinit. It is not a function-level variable but rather belongs to the entire muxer.

[winlinvip]

    int ret, profile, level, profile_iop;
    RTCContext *rtc = s->priv_data;

    char *tmp = av_mallocz(MAX_SDP_SIZE);

Can this replaced by av_bprint ?

[winlinvip]

Winlin: Fixed by bbdf7eb

[winlinvip]

/src
/mapfile
/tools/python/__pycache__/
.idea
patcheck.*

two nouse line

[winlinvip]

Winlin: Got it. Fixed by 10de0fc

[winlinvip]

} DTLSContext;

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"

I think you can remove these two line by some ways.

[winlinvip]

Winlin: If the two lines of code are removed, there will be warnings about the SSL deprecated API. Is it acceptable to have these warnings?

Fixed by 10de0fc

Within the next few days, I will fix the warnings introduced by this commit.

[winlinvip]

#if OPENSSL_VERSION_NUMBER < 0x1010102fL
#error "OpenSSL version 1.1.1b or newer is required"
#endif
#endif

是不是跟上面的ifndef可以搞成else的方式

[winlinvip]

Winlin: Fixed.

[winlinvip]

#include "srtp.h"
#include "avc.h"
#include "http.h"
#include "libavutil/bprint.h"

头文件引用的是不是带目录的是一坨，不带目录的是一坨，排序一坨

[winlinvip]

Winlin: Fixed.

[winlinvip]

    ctx->dtls_cert = dtls_cert = X509_new();
    ctx->dtls_pkey = dtls_pkey = EVP_PKEY_new();
    dtls_eckey = EC_KEY_new();

这几个new需要check失败不

[winlinvip]

Winlin: Fixed.

[winlinvip]

        } else {
            av_log(s1, AV_LOG_ERROR, "DTLS: SSL3 alert method=%s type=%s, desc=%s(%s), where=%d, ret=%d, r1=%d\n",
                method, alert_type, alert_desc, SSL_alert_desc_string_long(ret), where, ret, r1);
        }

如果只是一条语句，是不是可以没有{}

[winlinvip]

Winlin: Fixed.

[winlinvip]

        method = "SSL_accept";
    } else {
        method = "undefined";
    }

是不是可以先undefined，然后这里再if else，可以节省一次else

[winlinvip]

Winlin: Fixed.

[winlinvip]

            } else {
                av_log(s1, AV_LOG_VERBOSE, "DTLS: method=%s state=%s(%s), where=%d, ret=%d, r1=%d\n",
                    method, SSL_state_string(dtls), SSL_state_string_long(dtls), where, ret, r1);
            }

ditoo

[winlinvip]

Winlin: Fixed

[winlinvip]

     * when got ServerHello, we should reset the timer. */
    if (!previous_us || ctx->dtls_should_reset_timer) {
        timeout_us =  ctx->dtls_arq_timeout * 1000; /* in us */
    }

ditoo

[winlinvip]

Winlin: Fixed.

[winlinvip]

    if (length >= 14) {
        handshake_type = (uint8_t)data[13];
    }

不必要的大括号

[winlinvip]

Winlin: Fixed

[winlinvip]

{
    /* Always OK, we don't check the certificate of client, because we allow client self-sign certificate. */
    return 1;
}

没啥用的可以删掉

[winlinvip]

Winlin: Removed.

[winlinvip]

    STUN_ATTR_FINGERPRINT               = 0x8028, /// rfc5389
};

#if CONFIG_OPENSSL

This should be removed, because this file should be enabled if CONFIG_OPENSSL is true when configure

[winlinvip]

Winlin: Fixed.

[winlinvip]

    { "dtls_arq_max",       "Maximum number of retransmissions for the DTLS ARQ mechanism",     OFFSET(dtls_arq_max),       AV_OPT_TYPE_INT,    { .i64 = 5 },       -1, INT_MAX, DEC },
    { "dtls_arq_timeout",   "Start timeout in milliseconds for the DTLS ARQ mechanism",         OFFSET(dtls_arq_timeout),   AV_OPT_TYPE_INT,    { .i64 = 50 },     -1, INT_MAX, DEC },
    { "pkt_size",           "The maximum size, in bytes, of RTP packets that send out",         OFFSET(pkt_size),           AV_OPT_TYPE_INT,    { .i64 = 1500 },    -1, INT_MAX, DEC },
    { NULL },

add descriptions of the options into doc/muxer.texi

[winlinvip]

Winlin: Add section:

@anchor{rtc}
@section rtc

[winlinvip]

 * @param extradata_size   Size of the extradata
 * @returns                Returns 0 if successful or AVERROR_xxx in case of an error.
 */
static int isom_read_avcc(AVFormatContext *s, uint8_t *extradata, int  extradata_size)

Can libavformat/avc.c replace this function?

[winlinvip]

Winlin: In avc.c, there is only a function ff_isom_write_avcc to encode avcc in ISOM format, but we need to decode it.

I didn't find a similar API to decode SPS/PPS from extradata, so we must parse it ourselves.