Bug 235879 – [GStreamer][WebRTC] Allow backend to keep track of buffered-amount

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Philippe Normand]

In GstWebRTC the buffered-amount and corresponding low-threshold are managed at public properties, so in WebKit we shouldn't need to manually update those in RTCDataChannel.

[Philippe Normand]

Created attachment 450388 [details]
Patch

[youenn fablet]

Comment on attachment 450388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450388&action=review

> Source/WebCore/ChangeLog:10
> +        corresponding property instead.

m_bufferedAmount is mapping precisely to https://w3c.github.io/webrtc-pc/#dfn-bufferedamount.
By adding the logic in RTCDataChannel, we can follow the spec to the letter, for instance:
- https://w3c.github.io/webrtc-pc/#datachannel-send step 6 (Increase [[BufferedAmount]] for each send call synchronously).
- https://w3c.github.io/webrtc-pc/#dom-datachannel-bufferedamount (backend to queue a task to reduce [[BufferedAmount]] asynchronously).

What is the goal of this change? Won't it affect interop for GstWebRTC?

[Philippe Normand]

GstWebRTC follows the spec and defines a public interface in:
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-bad/gst-libs/gst/webrtc/datachannel.c

So with the current code we end-up managing buffered-amount and low-threshold twice, which is prone to inconsistencies, and actually introduces test failures in webrtc/ data-channel tests with the GstWebRTC backend enabled. That's why I propose this patch :)

Fortunately I didn't have to change much of the platform-agnostic WebRTC code WebKit, most of the GStreamer-specific code will be contained to dedicated platform modules.

[youenn fablet]

(In reply to Philippe Normand from comment #3)
> GstWebRTC follows the spec and defines a public interface in:
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/
> gst-plugins-bad/gst-libs/gst/webrtc/datachannel.c
> 
> So with the current code we end-up managing buffered-amount and
> low-threshold twice, which is prone to inconsistencies, and actually
> introduces test failures in webrtc/ data-channel tests with the GstWebRTC
> backend enabled. That's why I propose this patch :)
> 
> Fortunately I didn't have to change much of the platform-agnostic WebRTC
> code WebKit, most of the GStreamer-specific code will be contained to
> dedicated platform modules.

I am not sure this will work properly, due to blob data being sent asynchronously to the backend, but the blob size impacting bufferedAmount synchronously (see NetworkSendQueue).

Here is an example:
// amount is initially zero for channel and backend.
channel.send(string1);
// RTCDataChannel amount is string1.size. backend amount is also string1.size.
channel.send(blob1);
// RTCDataChannel amount is string1.size+blob1.size, backend amount is string1.size since blob1 data will be received asynchronously.
channel.send(string2);
// RTCDataChannel amount is string1.size+blob1.size+string2.size, backend amount is still string1.size.

[Philippe Normand]

Right, this approach is a bit flawed indeed, as the gstwebrtc implementation also has an internal queue (in the appsrc element)...

[Radar WebKit Bug Importer]

<rdar://problem/88561869>

[Philippe Normand]

Pull request: https://github.com/WebKit/WebKit/pull/1157

[EWS]

Committed 251990@main (af95424cb233): <https://commits.webkit.org/251990@main>

Reviewed commits have been landed. Closing PR #1157 and removing active labels.