Bug 235879

Summary: [GStreamer][WebRTC] Allow backend to keep track of buffered-amount
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: WebRTCAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, philn, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 235885    
Attachments:
Description Flags
Patch none

Description Philippe Normand 2022-01-31 01:29:47 PST
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.
Comment 1 Philippe Normand 2022-01-31 01:59:02 PST
Created attachment 450388 [details]
Patch
Comment 2 youenn fablet 2022-01-31 02:21:33 PST
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?
Comment 3 Philippe Normand 2022-01-31 02:48:51 PST
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.
Comment 4 youenn fablet 2022-01-31 03:03:32 PST
(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.
Comment 5 Philippe Normand 2022-01-31 03:39:02 PST
Right, this approach is a bit flawed indeed, as the gstwebrtc implementation also has an internal queue (in the appsrc element)...
Comment 6 Radar WebKit Bug Importer 2022-02-07 01:30:19 PST
<rdar://problem/88561869>
Comment 7 Philippe Normand 2022-05-30 04:12:56 PDT
Pull request: https://github.com/WebKit/WebKit/pull/1157
Comment 8 EWS 2022-06-30 05:31:22 PDT
Committed 251990@main (af95424cb233): <https://commits.webkit.org/251990@main>

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