CR #1: Roundtrip2: Comments from Kieran Kunhya

[winlinvip]

Review for #1

[winlinvip]

 * Maximum size limit of a Session Description Protocol (SDP),
 * be it an offer or answer.
 */
#define MAX_SDP_SIZE 8192

?

[winlinvip]

Winlin: When generating an SDP offer or parsing an SDP answer, a maximum length for the SDP is established.

I don't really understand what the question is about.

这里就是限制了SDP最大长度，因为生成和解析时，字符串操作都必须有个最大长度。我其实没理解这里具体是什么疑问。

Won't fix.

[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)

Kieran Kunhya:

This code is duplicated from other parts of FFmpeg

[winlinvip]

Winlin: 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

Fixed.

[winlinvip]

Kieran Kunhya:

 * The below is an example of SDP offer:
 *
 *       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

This is not a legal SDP, each line must end only with one space (yes, this is regularly wrong).

[winlinvip]

Winlin: I'm sorry, this SDP example is not specific and correct SDP data from the real world. I added an empty line between m lines to make it easier for other users to read. I have also removed unncessary SDP examples in the comments.

应该是指的m之前的空行？这些空行并不是真的在SDP中有，也就是WHIP中交换的SDP确实是没有这个空行的，这些空行只是我刻意加的，为了让注释看起来容易理解。再次强调，SDP中并不能加空行，只是为了好理解的注释而已。我已经把这个SDP例子从注释中去掉了，为了避免混淆。

Winlin: Fixed.

[winlinvip]

    /* 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;

Kieran Kunhya:

Correct how?

[winlinvip]

Winlin: Occasionally, the timestamp of OPUS audio may be inaccurate, particularly when the input is an MP4 file.

Summary of the issues:

1. When playing an MP4 file (264+Opus) directly in Chrome, there are no audio issues.
2. When copying the MP4 file using -c copy and pushing it to WHIP, playing it in Chrome results in audio noise issues.
3. When copying the MP4 file using -re -c copy and pushing it to WHIP, there are no audio issues.
4. After re-encoding the MP4 file with -acodec libopus, pushing it to WHIP, and playing it in Chrome, there are no audio issues.
5. When copying an MKV file using -c copy and pushing it to WHIP, playing it in Chrome results in no audio issues.

For opus 48khz, each frame is 20ms which is 48000*20/1000 = 960. It appears that there is a bug introduced by libopus regarding the timestamp. Instead of being exactly 960, there is a slight deviation, such as 956 or 970. This deviation can cause Chrome to play the audio stream with noise.

To replicate the issue, start by transcoding the file into an MP4 format then publish by WHIP muxer:

ffmpeg -i ~/git/srs/trunk/doc/source.200kbps.768x320.flv \
  -vcodec libx264 -profile:v baseline -r 25 -g 50 \
  -acodec libopus -ar 48000 -ac 2 \
  -y t.mp4

ffmpeg -re -i t.mp4 -c copy \
  -f rtc "http://localhost:1985/rtc/v1/whip/?app=live&stream=livestream"

When adding -re, the timestamp is accurate, and there is no such issue:

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

ffmpeg -re -i t.mp4 -c copy \
  -f rtc "http://localhost:1985/rtc/v1/whip/?app=live&stream=livestream"

The process also functions smoothly without any issues when converting to MKV format and then publishing as a WHIP stream:

ffmpeg -i ~/git/srs/trunk/doc/source.200kbps.768x320.flv \
  -vcodec libx264 -profile:v baseline -r 25 -g 50 \
  -acodec libopus -ar 48000 -ac 2
  -y t.mkv

ffmpeg -re -i t.mkv -c copy \
  -f rtc "http://localhost:1985/rtc/v1/whip/?app=live&stream=livestream"

Nonetheless, when directly transcoding and publishing through the WHIP muxer, this problem is absent, and the difference in audio pts consistently remains at 960:

~/git/FFmpeg/ffmpeg_g -re -i ~/git/srs/trunk/doc/source.200kbps.768x320.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"

Although we are unsure of the root cause, I have included a "TODO" comment regarding this issue. We need to conduct further research and remove this line.

/**
 * TODO: FIXME: There is an issue with the timestamp of OPUS audio, especially when
 *  the input is an MP4 file. The timestamp deviates from the expected value of 960,
 *  causing Chrome to play the audio stream with noise. This problem can be replicated
 *  by transcoding a specific file into MP4 format and publishing it using the WHIP
 *  muxer. However, when directly transcoding and publishing through the WHIP muxer,
 *  the issue is not present, and the audio timestamp remains consistent. The root
 *  cause is still unknown, and this comment has been added to address this issue
 *  in the future. Further research is needed to resolve the problem.
 */
static int parse_codec(AVFormatContext *s)

The log indicates that the pts does not equal 960, but rather has a deviation:

>>> audio diff=960
>>> audio diff=954
>>> audio diff=973
>>> audio diff=960
>>> audio diff=960
>>> audio diff=960
>>> audio diff=952

See winlinvip@961c95cc4f

[winlinvip]

Winlin: 偶尔，OPUS音频的时间戳可能不准确，特别是当输入是MP4文件时。

总结下问题：

1. Chrome直接播放 MP4 文件(264+Opus)，声音没有问题。
2. 将 MP4 文件-c copy推 WHIP，使用 Chrome 播放，声音有杂音问题。
3. 将 MP4 文件-re -c copy推 WHIP，声音没有问题。
4. 将 MP4 文件-acodec libopus重新转码后，推 WHIP，使用 Chrome 播放，声音没问题。
5. MKV文件-c copy推 WHIP，使用 Chrome 播放，声音没问题。

对于48kHz的opus，每帧为20ms，即48000*20/1000=960。看起来libopus引入了关于时间戳的错误。而不是恰好为960，而是有轻微的偏差，例如956或970。这种偏差可能导致Chrome播放带有噪音的音频流。

要重现此问题，请首先将file转换为MP4格式，然后使用 WHIP 推流：

ffmpeg -i ~/git/srs/trunk/doc/source.200kbps.768x320.flv \
  -vcodec libx264 -profile:v baseline -r 25 -g 50 \
  -acodec libopus -ar 48000 -ac 2 \
  -y t.mp4

ffmpeg -re -i t.mp4 -c copy \
  -f rtc "http://localhost:1985/rtc/v1/whip/?app=live&stream=livestream"

如果加上-re，时间戳就没有问题，声音正常：

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

ffmpeg -re -i t.mp4 -c copy \
  -f rtc "http://localhost:1985/rtc/v1/whip/?app=live&stream=livestream"

然而，将其转换为MKV格式然后作为WHIP流发布时，没有任何问题：

ffmpeg -i ~/git/srs/trunk/doc/source.200kbps.768x320.flv \
  -vcodec libx264 -profile:v baseline -r 25 -g 50 \
  -acodec libopus -ar 48000 -ac 2
  -y t.mkv

ffmpeg -re -i t.mkv -c copy \
  -f rtc "http://localhost:1985/rtc/v1/whip/?app=live&stream=livestream"

而且，如果再次使用 libopus 编码时没有这个问题，音频pts的差异始终保持在960：

~/git/FFmpeg/ffmpeg_g -re -i ~/git/srs/trunk/doc/source.200kbps.768x320.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"

尽管我们不确定根本原因，但我已在此问题上添加了一个“TODO”注释。我们需要进行进一步的研究并删除此行。

/**
 * TODO: FIXME: There is an issue with the timestamp of OPUS audio, especially when
 *  the input is an MP4 file. The timestamp deviates from the expected value of 960,
 *  causing Chrome to play the audio stream with noise. This problem can be replicated
 *  by transcoding a specific file into MP4 format and publishing it using the WHIP
 *  muxer. However, when directly transcoding and publishing through the WHIP muxer,
 *  the issue is not present, and the audio timestamp remains consistent. The root
 *  cause is still unknown, and this comment has been added to address this issue
 *  in the future. Further research is needed to resolve the problem.
 */
static int parse_codec(AVFormatContext *s)

出现声音问题时，日志显示pts不等于960，而是有偏差：

>>> audio diff=960
>>> audio diff=954
>>> audio diff=973
>>> audio diff=960
>>> audio diff=960
>>> audio diff=960
>>> audio diff=952

参见 winlinvip@961c95cc4f

[winlinvip]

Since this problem is unrelated to WHIP and appears to be a bug when converting FLV to MP4 without using the -re option, resulting in a mismatch between the timestamp and OPUS frame size that causes audio distortion, we will not fix this issue.

[winlinvip]

Kieran Kunhya:

This patchset is massive and unmaintainable. Not to mention only implements a small portion of webrtc, that will inevitably get patched to add more features and become a giant sprawl.

[winlinvip]

Winlin: If we aim to support all the full features of WebRTC, the patch will become a sprawling giant and difficult to maintain. However, I believe that WHIP is only a small subset of WebRTC, as there are some usefull features that we do not need to support. For example:

• P2P and ICE will require the inclusion of numerous transport algorithms and libraries. However, we only need to support ICE Lite, which functions as a UDP client and server, similar to other protocols like SRT, RTMP, or RTSP that are supported by FFmpeg.
• Simulcast and SVC are also significant gitant features, and I do not recommend supporting them immediately. These advanced features are not necessary, and they are not commonly used in WebRTC scenarios.
• Congestion control and algorithms, such as ARQ and FEC, can be straightforward if we only support NACK for ARQ and simple FEC like RED. However, supporting GCC or RSFEC, etc., can be complex. I also suggest not supporting them immediately because, in some use scenarios, the network conditions are good.
• Signaling and room management are not necessary to touch as WHIP is a straightforward signaling method for WebRTC. FFmpeg only needs to publish a WebRTC stream for use cases such as robot control or similar scenarios. Therefore, there is no need to introduce rooms or multiple streams in a WebRTC session.
• Audio processing, such as 3A, is not required for FFmpeg as it is a feature for players like Chrome.

This patch is simple not only because WHIP is a subset of WebRTC, but also because it reuses many of FFmpeg's powerful capabilities, such as codec and format, SRTP, HTTP, HTTPS, OpenSSL, and more.

如果我们要支持完善的WebRTC的功能，那么这个会变得很大而且难以维护。但我理解WHIP只是WebRTC的一个子集，如果我们只是实现有限但有用的功能，那么最终是可维护的，比如：

• P2P和ICE Full是复杂的功能，但是WHIP只需要实现ICE lite。这样在网络层只需要实现类似CS结构，和SRT、RTMP、RTSP等目前已经实现的协议是类似的。
• Simulcast和SVC这是WebRTC中比较复杂的高阶能力，一般涉及多流和网络自适应调整，但它并不是必须的功能，非常非常多的应用场景并不需要这些高阶能力。
• Congestion control拥塞控制算法，比如ARQ和FEC。如果只是实现基本的NACK和FEC复杂度是可控的。如果要实现高阶的GCC和RSFEC等会很复杂。我建议先不要着急实现复杂的算法，一般网络条件很好，也并不需要完全实现这些复杂的算法。
• Signaling和Room管理，WHIP是非常简单的信令，我们并不需要实现WebRTC业务中常见的复杂信令和房间管理。FFmpeg是基于流级别的处理，房间中的多个流本来也可以解耦成独立的流。
• Audio处理，比如3A，这些是播放器需要处理的功能，FFmpeg本身也不需要实现。

这个Patch看起来简单，不仅仅是因为WHIP是WebRTC的一个子集，更加重要的是复用了FFmpeg强大的现有的能力，比如编解码和封装、SRTP、HTTP、HTTPS和OpenSSL等。