gstwebrtc-api: address issues raised by mix matrix support

1c48d7065d 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>
2024-11-22 03:21:00 +00:00 · 2024-08-16 17:37:51 +02:00 · 2024-08-16 17:37:51 +02:00 · 8ad882bed5
commit 8ad882bed5
parent c4bcdea830
6 changed files with 267 additions and 139 deletions
--- 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;
                      }
                    }
                  });
--- 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;
--- 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) {
--- 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,
--- 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);
--- 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() {