From 8ad882bed5ffbe92592ebe7ccbdaa74a1601adef Mon Sep 17 00:00:00 2001
From: Mathieu Duponchelle <mathieu@centricular.com>
Date: Fri, 16 Aug 2024 17:37:51 +0200
Subject: [PATCH] gstwebrtc-api: address issues raised by mix matrix support

1c48d7065d59d7da00a155c68f32fcff1936763c was mistakenly merged too
early, and there were concerns about the implementation and API design:

The fact that the frontend had to expose a text area specifically for
sending over a mix matrix, and had to manually edit in floats into the
stringified JSON was suboptimal.

Said text area was always present even when remote control was not
enabled.

The sendControlRequest API was made more complex than needed by
accepting an optional stringifier callback.

This patch addresses all those concerns:

The deserialization code in webrtcsink is now made more clever and
robust by first having it pick a numerical type to coerce to when
deserializing arrays with numbers, then making sure it doesn't allow
mixed types in arrays (or arrays of arrays as those too must share
the same inner value type).

The frontend side simply sends over strings wrapped with a request
message envelope to the backend.

The request text area is only shown when remote control is enabled.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/merge_requests/1725>
---
 net/webrtc/gstwebrtc-api/index.html           |  89 ++-----
 net/webrtc/gstwebrtc-api/src/index.js         |   4 -
 .../gstwebrtc-api/src/remote-controller.js    |  17 +-
 net/webrtc/src/utils.rs                       | 235 +++++++++++++++++-
 net/webrtc/src/webrtcsink/imp.rs              |  59 +----
 net/webrtc/src/webrtcsrc/imp.rs               |   2 +-
 6 files changed, 267 insertions(+), 139 deletions(-)

diff --git a/net/webrtc/gstwebrtc-api/index.html b/net/webrtc/gstwebrtc-api/index.html
index d087e508..e76eaebc 100644
--- a/net/webrtc/gstwebrtc-api/index.html
+++ b/net/webrtc/gstwebrtc-api/index.html
@@ -82,11 +82,19 @@
       outline: none;
     }
 
-    div.video, div.offer-options, div.mix-matrix {
+    div.video, div.offer-options, div.request-box {
       position: relative;
       margin: 1em;
     }
 
+    div.request-box {
+      display: none;
+    }
+
+    #remote-streams>li.streaming.has-remote-control .request-box {
+      display: block;
+    }
+
     div.video>div.fullscreen {
       position: absolute;
       top: 0;
@@ -299,52 +307,6 @@
       });
     }
 
-    const beginFloat = "~begin~float~";
-    const endFloat = "~end~float~";
-
-    // Count the number of nested elements in array
-    function deepCount(arr = []) {
-       return arr
-       .reduce((acc, val) => {
-          return acc + (Array.isArray(val) ? deepCount(val) : 0);
-       }, arr.length);
-    };
-
-    // Adapted from https://www.npmjs.com/package/stringify-with-floats in order to
-    // target the mix matrix and make sure the JSON output contains floating point
-    // numbers no matter what
-    function stringifyMatrix(inputValue) {
-      let elementsInMatrix = 0;
-      const jsonReplacer = (key, val) => {
-        let value;
-        let inMatrix = false;
-
-        value = val;
-        if (key == "matrix") {
-          elementsInMatrix = deepCount(val);
-        } else if (elementsInMatrix > 0) {
-          elementsInMatrix--;
-          inMatrix = true;
-        }
-        const forceFloat =
-          inMatrix == true &&
-          (value || value === 0) &&
-          typeof value === "number" &&
-          !value.toString().toLowerCase().includes("e");
-        return forceFloat ? `${beginFloat}${value}${endFloat}` : value;
-      };
-      const json = JSON.stringify(inputValue, jsonReplacer, 2);
-      const regexReplacer = (match, num) => {
-        return num.includes(".") || Number.isNaN(num)
-          ? Number.isNaN(num)
-            ? num
-            : Number(num).toFixed(1)
-          : `${num}.${"0".repeat(1)}`;
-      };
-      const re = new RegExp(`"${beginFloat}(.+?)${endFloat}"`, "g");
-      return json.replace(re, regexReplacer);
-    };
-
     function initRemoteStreams(api) {
       const remoteStreamsElement = document.getElementById("remote-streams");
 
@@ -359,9 +321,9 @@
                                 <div class="offer-options">
                                   <textarea rows="5" cols="50" placeholder="offer options, empty to answer. For example:\n{\n  &quot;offerToReceiveAudio&quot;: 1\n  &quot;offerToReceiveVideo&quot;: 1\n}\n"></textarea>
                                 </div>
-                                <div class="mix-matrix">
-                                  <textarea rows="4" cols="50" placeholder="Mix matrix, if any, for example [[1.0, 0.0], [0.0, 1.0]]"></textarea>
-                                  <button disabled="disabled">Submit mix matrix</button>
+                                <div class="request-box">
+                                  <textarea rows="4" cols="50" placeholder="JSON request to send over"></textarea>
+                                  <button disabled="disabled">Submit request</button>
                                 </div>
                                 <div class="video">
                                     <div class="spinner">
@@ -379,19 +341,13 @@
             const entryElement = document.getElementById(producerId);
             const videoElement = entryElement.getElementsByTagName("video")[0];
             const offerTextareaElement = entryElement.getElementsByTagName("textarea")[0];
-            const mixmatrixTextAreaElement = entryElement.getElementsByTagName("textarea")[1];
-            const submitMixmatrixButtonElement = entryElement.getElementsByTagName("button")[0];
+            const requestTextAreaElement = entryElement.getElementsByTagName("textarea")[1];
+            const submitRequestButtonElement = entryElement.getElementsByTagName("button")[0];
 
-            submitMixmatrixButtonElement.addEventListener("click", (event) => {
+            submitRequestButtonElement.addEventListener("click", (event) => {
               try {
-                let matrix = JSON.parse(mixmatrixTextAreaElement.value);
-                let id = entryElement._consumerSession.remoteController.sendControlRequest({
-                  type: "customUpstreamEvent",
-                  structureName: "GstRequestMixMatrix",
-                  structure: {
-                    matrix: matrix
-                  }
-                }, stringifyMatrix);
+                let request = requestTextAreaElement.value;
+                let id = entryElement._consumerSession.remoteController.sendControlRequest(request);
               } catch (ex) {
                 console.error("Failed to parse mix matrix:", ex);
                 return;
@@ -458,18 +414,11 @@
                       const remoteController = session.remoteController;
                       if (remoteController) {
                         entryElement.classList.add("has-remote-control");
-                        submitMixmatrixButtonElement.disabled = false;
+                        submitRequestButtonElement.disabled = false;
                         remoteController.attachVideoElement(videoElement);
-                        let id = remoteController.sendControlRequest({
-                          type: "customUpstreamEvent",
-                          structureName: "GstRequestMixMatrix",
-                          structure: {
-                            matrix: [[1.0, 0.0], [0.0, 1.0]]
-                          }
-                        });
                       } else {
                         entryElement.classList.remove("has-remote-control");
-                        submitMixmatrixButtonElement.disabled = true;
+                        submitRequestButtonElement.disabled = true;
                       }
                     }
                   });
diff --git a/net/webrtc/gstwebrtc-api/src/index.js b/net/webrtc/gstwebrtc-api/src/index.js
index d08646e7..29119a57 100644
--- a/net/webrtc/gstwebrtc-api/src/index.js
+++ b/net/webrtc/gstwebrtc-api/src/index.js
@@ -48,10 +48,6 @@ import GstWebRTCAPI from "./gstwebrtc-api.js";
  * @external HTMLVideoElement
  * @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement
  */
-/**
- * @external JSON.stringify
- * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify
- */
 
 if (!window.GstWebRTCAPI) {
   window.GstWebRTCAPI = GstWebRTCAPI;
diff --git a/net/webrtc/gstwebrtc-api/src/remote-controller.js b/net/webrtc/gstwebrtc-api/src/remote-controller.js
index a076894b..a32fde80 100644
--- a/net/webrtc/gstwebrtc-api/src/remote-controller.js
+++ b/net/webrtc/gstwebrtc-api/src/remote-controller.js
@@ -201,21 +201,12 @@ export default class RemoteController extends EventTarget {
      *
      * @method GstWebRTCAPI.RemoteController#sendControlRequest
      * @fires {@link GstWebRTCAPI#event:ErrorEvent}
-     * @param {object} request - The request to stringify and send over the channel
-     * @param {string} request.type - The type of the request
-     * @param {function} stringifier - An optional callback for stringifying,
-     * {@link external:JSON.stringify} will be used otherwise.
+     * @param {object|string} request - The request to send over the channel
      * @returns {number} The identifier attributed to the request, or -1 if an exception occurred
      */
-  sendControlRequest(request, stringifier) {
+  sendControlRequest(request) {
     try {
-      if (stringifier && (typeof(stringifier) !== "function")) {
-        throw new Error("invalid stringifier");
-      } else if (!stringifier) {
-        stringifier = JSON.stringify;
-      }
-
-      if (!request || (typeof (request) !== "object")) {
+      if (!request || ((typeof (request) !== "object") && (typeof (request) !== "string"))) {
         throw new Error("invalid request");
       }
 
@@ -228,7 +219,7 @@ export default class RemoteController extends EventTarget {
         request: request
       };
 
-      this._rtcDataChannel.send(stringifier(message));
+      this._rtcDataChannel.send(JSON.stringify(message));
 
       return message.id;
     } catch (ex) {
diff --git a/net/webrtc/src/utils.rs b/net/webrtc/src/utils.rs
index b0005682..a04350d6 100644
--- a/net/webrtc/src/utils.rs
+++ b/net/webrtc/src/utils.rs
@@ -4,7 +4,7 @@ use std::{
     sync::atomic::{AtomicBool, Ordering},
 };
 
-use anyhow::{Context, Error};
+use anyhow::{anyhow, Context, Error};
 use gst::{glib, prelude::*};
 use once_cell::sync::Lazy;
 
@@ -1022,12 +1022,19 @@ pub enum ControlRequest {
     },
 }
 
+#[derive(Debug, Serialize, Deserialize, PartialEq)]
+#[serde(untagged)]
+pub enum StringOrRequest {
+    String(String),
+    Request(ControlRequest),
+}
+
 #[derive(Serialize, Deserialize, Debug, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct ControlRequestMessage {
     pub id: u64,
     pub mid: Option<String>,
-    pub request: ControlRequest,
+    pub request: StringOrRequest,
 }
 
 #[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
@@ -1043,10 +1050,234 @@ pub fn find_smallest_available_ext_id(ids: impl IntoIterator<Item = u32>) -> u32
     (1..).find(|&num| !used_numbers.contains(&num)).unwrap()
 }
 
+#[derive(Clone, Debug)]
+enum CoerceTarget {
+    Undefined,
+    U64,
+    I64,
+    F64,
+    Other(serde_json::Value),
+}
+
+fn pick_coerce_target(
+    arr: &Vec<serde_json::Value>,
+    mut target: CoerceTarget,
+) -> Result<CoerceTarget, Error> {
+    for val in arr {
+        match val {
+            serde_json::Value::Null => {
+                return Err(anyhow!("Untyped null values are not handled"));
+            }
+            serde_json::Value::Bool(_) => match &target {
+                CoerceTarget::Undefined => {
+                    target = CoerceTarget::Other(val.clone());
+                }
+                CoerceTarget::Other(other) => {
+                    if !other.is_boolean() {
+                        return Err(anyhow!("Mixed types in arrays are not supported"));
+                    }
+                }
+                _ => {
+                    return Err(anyhow!("Mixed types in arrays are not supported"));
+                }
+            },
+            serde_json::Value::Number(v) => {
+                let v_target = if v.as_u64().is_some() {
+                    CoerceTarget::U64
+                } else if v.as_i64().is_some() {
+                    CoerceTarget::I64
+                } else {
+                    CoerceTarget::F64
+                };
+                match &target {
+                    CoerceTarget::Undefined => {
+                        target = v_target;
+                    }
+                    CoerceTarget::Other(_) => {
+                        return Err(anyhow!("Mixed types in arrays are not supported"));
+                    }
+                    CoerceTarget::U64 => {
+                        target = v_target;
+                    }
+                    CoerceTarget::I64 => {
+                        if matches!(v_target, CoerceTarget::F64) {
+                            target = CoerceTarget::F64;
+                        }
+                    }
+                    _ => (),
+                }
+            }
+            serde_json::Value::Array(a) => {
+                target = pick_coerce_target(a, target)?;
+            }
+            serde_json::Value::Object(_) => match &target {
+                CoerceTarget::Undefined => {
+                    target = CoerceTarget::Other(val.clone());
+                }
+                CoerceTarget::Other(other) => {
+                    if !other.is_object() {
+                        return Err(anyhow!("Mixed types in arrays are not supported"));
+                    }
+                }
+                _ => {
+                    return Err(anyhow!("Mixed types in arrays are not supported"));
+                }
+            },
+            serde_json::Value::String(_) => match &target {
+                CoerceTarget::Undefined => {
+                    target = CoerceTarget::Other(val.clone());
+                }
+                CoerceTarget::Other(other) => {
+                    if !other.is_object() {
+                        return Err(anyhow!("Mixed types in arrays are not supported"));
+                    }
+                }
+                _ => {
+                    return Err(anyhow!("Mixed types in arrays are not supported"));
+                }
+            },
+        }
+    }
+
+    Ok(target)
+}
+
+fn deserialize_serde_value(
+    val: &serde_json::Value,
+    mut target: CoerceTarget,
+) -> Result<gst::glib::SendValue, Error> {
+    match val {
+        serde_json::Value::Null => Err(anyhow!("Untyped null values are not handled")),
+        serde_json::Value::Bool(v) => Ok(v.to_send_value()),
+        serde_json::Value::Number(v) => match target {
+            CoerceTarget::U64 => Ok(v
+                .as_u64()
+                .ok_or(anyhow!("Mixed types in arrays are not supported"))?
+                .to_send_value()),
+            CoerceTarget::I64 => Ok(v
+                .as_i64()
+                .ok_or(anyhow!("Mixed types in arrays are not supported"))?
+                .to_send_value()),
+            CoerceTarget::F64 => Ok(v
+                .as_f64()
+                .expect("all numbers coerce to f64")
+                .to_send_value()),
+            CoerceTarget::Undefined => {
+                if let Some(u) = v.as_u64() {
+                    Ok(u.to_send_value())
+                } else if let Some(i) = v.as_i64() {
+                    Ok(i.to_send_value())
+                } else if let Some(f) = v.as_f64() {
+                    Ok(f.to_send_value())
+                } else {
+                    unreachable!()
+                }
+            }
+            _ => unreachable!(),
+        },
+        serde_json::Value::String(v) => Ok(v.to_send_value()),
+        serde_json::Value::Array(a) => {
+            let mut gst_array = gst::Array::default();
+
+            target = pick_coerce_target(a, target)?;
+
+            for val in a {
+                gst_array.append_value(deserialize_serde_value(val, target.to_owned())?);
+            }
+
+            Ok(gst_array.to_send_value())
+        }
+        serde_json::Value::Object(_) => {
+            Ok(deserialize_serde_object(val, "webrtcsink-deserialized")?.to_send_value())
+        }
+    }
+}
+
+pub fn deserialize_serde_object(
+    obj: &serde_json::Value,
+    name: &str,
+) -> Result<gst::Structure, Error> {
+    let serde_json::Value::Object(map) = obj else {
+        return Err(anyhow!("not a serde object"));
+    };
+
+    let mut ret = gst::Structure::builder(name);
+
+    for (key, value) in map {
+        ret = ret.field(
+            key,
+            deserialize_serde_value(value, CoerceTarget::Undefined)?,
+        );
+    }
+
+    Ok(ret.build())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
 
+    #[test]
+    fn test_deserialize_array() -> Result<(), String> {
+        let arr = serde_json::from_str::<serde_json::Value>("[1, -1, 1.0]").unwrap();
+        let gst_arr = deserialize_serde_value(&arr, CoerceTarget::Undefined)
+            .unwrap()
+            .get::<gst::Array>()
+            .unwrap();
+        let gst_type = gst_arr.first().unwrap().value_type();
+        assert_eq!(gst_type, f64::static_type());
+
+        let arr = serde_json::from_str::<serde_json::Value>("[1, -1]").unwrap();
+        let gst_arr = deserialize_serde_value(&arr, CoerceTarget::Undefined)
+            .unwrap()
+            .get::<gst::Array>()
+            .unwrap();
+        let gst_type = gst_arr.first().unwrap().value_type();
+        assert_eq!(gst_type, i64::static_type());
+
+        let arr = serde_json::from_str::<serde_json::Value>("[1]").unwrap();
+        let gst_arr = deserialize_serde_value(&arr, CoerceTarget::Undefined)
+            .unwrap()
+            .get::<gst::Array>()
+            .unwrap();
+        let gst_type = gst_arr.first().unwrap().value_type();
+        assert_eq!(gst_type, u64::static_type());
+
+        // u64::MAX can't be represented as i64, mixed types
+        let arr = serde_json::from_str::<serde_json::Value>("[18446744073709551615, -1]").unwrap();
+        assert!(deserialize_serde_value(&arr, CoerceTarget::Undefined).is_err());
+
+        // we won't coerce bool to i64, mixed types
+        let arr = serde_json::from_str::<serde_json::Value>("[true, -1]").unwrap();
+        assert!(deserialize_serde_value(&arr, CoerceTarget::Undefined).is_err());
+
+        let arr = serde_json::from_str::<serde_json::Value>("[[0.2, 0], [0, 0]]").unwrap();
+        let gst_arr = deserialize_serde_value(&arr, CoerceTarget::Undefined)
+            .unwrap()
+            .get::<gst::Array>()
+            .unwrap();
+        let gst_type = gst_arr
+            .first()
+            .unwrap()
+            .get::<gst::Array>()
+            .unwrap()
+            .first()
+            .unwrap()
+            .value_type();
+        assert_eq!(gst_type, f64::static_type());
+        let gst_type = gst_arr
+            .last()
+            .unwrap()
+            .get::<gst::Array>()
+            .unwrap()
+            .first()
+            .unwrap()
+            .value_type();
+        assert_eq!(gst_type, f64::static_type());
+
+        Ok(())
+    }
+
     fn test_find_smallest_available_ext_id_case(
         ids: impl IntoIterator<Item = u32>,
         expected: u32,
diff --git a/net/webrtc/src/webrtcsink/imp.rs b/net/webrtc/src/webrtcsink/imp.rs
index b0cb68b2..467d826a 100644
--- a/net/webrtc/src/webrtcsink/imp.rs
+++ b/net/webrtc/src/webrtcsink/imp.rs
@@ -556,51 +556,6 @@ fn create_navigation_event(sink: &super::BaseWebRTCSink, msg: &str, session_id:
     }
 }
 
-fn deserialize_serde_value(val: &serde_json::Value) -> Result<gst::glib::SendValue, Error> {
-    match val {
-        serde_json::Value::Null => Err(anyhow!("Untyped null values are not handled")),
-        serde_json::Value::Bool(v) => Ok(v.to_send_value()),
-        serde_json::Value::Number(v) => {
-            if let Some(v) = v.as_i64() {
-                Ok(v.to_send_value())
-            } else if let Some(v) = v.as_u64() {
-                Ok(v.to_send_value())
-            } else if let Some(v) = v.as_f64() {
-                Ok(v.to_send_value())
-            } else {
-                unreachable!()
-            }
-        }
-        serde_json::Value::String(v) => Ok(v.to_send_value()),
-        serde_json::Value::Array(a) => {
-            let mut gst_array = gst::Array::default();
-
-            for val in a {
-                gst_array.append_value(deserialize_serde_value(val)?);
-            }
-
-            Ok(gst_array.to_send_value())
-        }
-        serde_json::Value::Object(_) => {
-            Ok(deserialize_serde_object(val, "webrtcsink-deserialized")?.to_send_value())
-        }
-    }
-}
-
-fn deserialize_serde_object(obj: &serde_json::Value, name: &str) -> Result<gst::Structure, Error> {
-    let serde_json::Value::Object(map) = obj else {
-        return Err(anyhow!("not a serde object"));
-    };
-
-    let mut ret = gst::Structure::builder(name);
-
-    for (key, value) in map {
-        ret = ret.field(key, deserialize_serde_value(value)?);
-    }
-
-    Ok(ret.build())
-}
-
 fn handle_control_event(
     sink: &super::BaseWebRTCSink,
     msg: &str,
@@ -608,16 +563,22 @@ fn handle_control_event(
 ) -> Result<utils::ControlResponseMessage, Error> {
     let msg: utils::ControlRequestMessage = serde_json::from_str(msg)?;
 
-    let event = match msg.request {
+    let request = match msg.request {
+        utils::StringOrRequest::String(s) => serde_json::from_str(&s)?,
+        utils::StringOrRequest::Request(r) => r,
+    };
+
+    let event = match request {
         utils::ControlRequest::NavigationEvent { event } => {
             gst::event::Navigation::new(event.structure())
         }
         utils::ControlRequest::CustomUpstreamEvent {
             structure_name,
             structure,
-        } => {
-            gst::event::CustomUpstream::new(deserialize_serde_object(&structure, &structure_name)?)
-        }
+        } => gst::event::CustomUpstream::new(utils::deserialize_serde_object(
+            &structure,
+            &structure_name,
+        )?),
     };
 
     gst::log!(CAT, obj = sink, "Processing control event: {:?}", event);
diff --git a/net/webrtc/src/webrtcsrc/imp.rs b/net/webrtc/src/webrtcsrc/imp.rs
index 9c681d53..3ae582ce 100644
--- a/net/webrtc/src/webrtcsrc/imp.rs
+++ b/net/webrtc/src/webrtcsrc/imp.rs
@@ -433,7 +433,7 @@ impl Session {
             let msg = utils::ControlRequestMessage {
                 id: self.request_counter,
                 mid: None,
-                request,
+                request: utils::StringOrRequest::Request(request),
             };
             self.request_counter += 1;
             match serde_json::to_string(&msg).ok() {