versatica / mediasoup

Cutting Edge WebRTC Video Conferencing
https://mediasoup.org
ISC License
6.27k stars 1.13k forks source link

fix RtcpFeedback Parameter empty #1387

Closed Lynnworld closed 6 months ago

Lynnworld commented 6 months ago

Hi, RtcpFeedback Parameter may be empty . I wrote Golang code with mediasoup, flatbuffer generate code like this

       if t.Type != "" {
        type_Offset = builder.CreateString(t.Type)
    }
    parameterOffset := flatbuffers.UOffsetT(0)
    if t.Parameter != "" {
        parameterOffset = builder.CreateString(t.Parameter)
    }

so no filed in flatbuffer, C++ will crash. flatbuffer have a --force-empty options ,but it do not work for Golang generate --force-empty : When serializing from object API representation, force strings and vectors to empty rather than null.

ibc commented 6 months ago

--force-empty options ,but it do not work for Golang generate --force-empty : When serializing from object API representation, force strings and vectors to empty rather than null.

But we don't use that option in Node/Rust/C++ and AFAIK there are more fbs in which we have optional strings.

So I assume this is a real bug no matter we use Node, Rust or Golang, and worker can crash in any of them, right? And your PR fixes it.

Lynnworld commented 6 months ago

golang have no null/nil for string,so empty string will miss the field in flatbuffer ,Except you defined optional field in the flatbuffer. Actually, I think this is a flatbuffer bug in golang generate. And for mediasoup,I found some code checked the field exist, but some code not.

Lynnworld commented 6 months ago

I read ts code in RtpParameters.ts, it alway write filed to flatbuffer, so everything is ok

for (const rtcp of codec.rtcpFeedback ?? []) {
            const typeOffset = builder.createString(rtcp.type);
            const rtcpParametersOffset = builder.createString(rtcp.parameter);

            rtcpFeedback.push(
                FbsRtcpFeedback.createRtcpFeedback(
                    builder,
                    typeOffset,
                    rtcpParametersOffset
                )
            );
        }

this is the flatbuffer generate objective code, null field will be skip. And Golang code empty string will be skip too.

export class RtcpFeedbackT implements flatbuffers.IGeneratedObject {
constructor(
  public type: string|Uint8Array|null = null,
  public parameter: string|Uint8Array|null = null
){}

pack(builder:flatbuffers.Builder): flatbuffers.Offset {
  const type = (this.type !== null ? builder.createString(this.type!) : 0);
  const parameter = (this.parameter !== null ? builder.createString(this.parameter!) : 0);

  return RtcpFeedback.createRtcpFeedback(builder,
    type,
    parameter
  );
}
}

so best check every non required field in code

ibc commented 6 months ago

Merge and release if this is fixing a real crash please?