structure: Change NULL and empty string handling

Don't forbid the empty string "" in generic structures, only in taglists.
Properly allow the NULL string by adding special cases for serialising
and deserialising it. prop1=(string)NULL is the NULL string,
prop1=(string)"NULL" is the actual string with the value "NULL"

gst/gststructure.c

@@ -657,18 +657,19 @@ gst_structure_set_field (GstStructure * structure, GstStructureField * field)
     s = g_value_get_string (&field->value);
     /* only check for NULL strings in taglists, as they are allowed in message
      * structs, e.g. error message debug strings */
-    if (G_UNLIKELY (s == NULL && IS_TAGLIST (structure))) {
+    if (G_UNLIKELY (IS_TAGLIST (structure) && (s == NULL || *s == '\0'))) {
-      g_warning ("Trying to set NULL string on field '%s' on taglist. "
+      if (s == NULL) {
-          "Please file a bug.", g_quark_to_string (field->name));
+        g_warning ("Trying to set NULL string on field '%s' on taglist. "
-      g_value_unset (&field->value);
+            "Please file a bug.", g_quark_to_string (field->name));
-      return;
+        g_value_unset (&field->value);
-    } else if (G_UNLIKELY (s != NULL && *s == '\0')) {
+        return;
-      /* empty strings never make sense */
+      } else {
-      g_warning ("Trying to set empty string on %s field '%s'. Please file a "
+        /* empty strings never make sense */
-          "bug.", IS_TAGLIST (structure) ? "taglist" : "structure",
+        g_warning ("Trying to set empty string on taglist field '%s'. "
-          g_quark_to_string (field->name));
+            "Please file a bug.", g_quark_to_string (field->name));
-      g_value_unset (&field->value);
+        g_value_unset (&field->value);
-      return;
+        return;
+      }
     } else if (G_UNLIKELY (s != NULL && !g_utf8_validate (s, -1, NULL))) {
       g_warning ("Trying to set string on %s field '%s', but string is not "
           "valid UTF-8. Please file a bug.",
@@ -1602,7 +1603,7 @@ priv_gst_structure_append_to_gstring (const GstStructure * structure,
     g_string_append_len (s, "=(", 2);
     g_string_append (s, gst_structure_to_abbr (type));
     g_string_append_c (s, ')');
-    g_string_append (s, GST_STR_NULL (t));
+    g_string_append (s, t == NULL ? "NULL" : t);
     g_free (t);
   }
 
@@ -1653,7 +1654,8 @@ gst_structure_to_string (const GstStructure * structure)
  * THIS FUNCTION MODIFIES THE STRING AND DETECTS INSIDE A NONTERMINATED STRING
  */
 static gboolean
-gst_structure_parse_string (gchar * s, gchar ** end, gchar ** next)
+gst_structure_parse_string (gchar * s, gchar ** end, gchar ** next,
+    gboolean unescape)
 {
   gchar *w;
 
@@ -1669,21 +1671,32 @@ gst_structure_parse_string (gchar * s, gchar ** end, gchar ** next)
     return ret;
   }
 
-  w = s;
+  if (unescape) {
-  s++;
+    w = s;
-  while (*s != '"') {
+    s++;
-    if (*s == 0)
+    while (*s != '"') {
-      return FALSE;
+      if (G_UNLIKELY (*s == 0))
-
+        return FALSE;
-    if (*s == '\\') {
+      if (G_UNLIKELY (*s == '\\'))
+        s++;
+      *w = *s;
+      w++;
       s++;
     }
-
-    *w = *s;
-    w++;
     s++;
+  } else {
+    /* Find the closing quotes */
+    s++;
+    while (*s != '"') {
+      if (G_UNLIKELY (*s == 0))
+        return FALSE;
+      if (G_UNLIKELY (*s == '\\'))
+        s++;
+      s++;
+    }
+    s++;
+    w = s;
   }
-  s++;
 
   *end = w;
   *next = s;
@@ -1931,11 +1944,7 @@ gst_structure_parse_value (gchar * str,
     ret = gst_structure_parse_array (s, &s, value, type);
   } else {
     value_s = s;
-    if (G_UNLIKELY (!gst_structure_parse_string (s, &value_end, &s)))
-      return FALSE;
 
-    c = *value_end;
-    *value_end = '\0';
     if (G_UNLIKELY (type == G_TYPE_INVALID)) {
       GType try_types[] =
           { G_TYPE_INT, G_TYPE_DOUBLE, GST_TYPE_FRACTION, G_TYPE_BOOLEAN,
@@ -1943,6 +1952,12 @@ gst_structure_parse_value (gchar * str,
       };
       int i;
 
+      if (G_UNLIKELY (!gst_structure_parse_string (s, &value_end, &s, TRUE)))
+        return FALSE;
+      /* Set NULL terminator for deserialization */
+      c = *value_end;
+      *value_end = '\0';
+
       for (i = 0; i < G_N_ELEMENTS (try_types); i++) {
         g_value_init (value, try_types[i]);
         ret = gst_value_deserialize (value, value_s);
@@ -1953,6 +1968,13 @@ gst_structure_parse_value (gchar * str,
     } else {
       g_value_init (value, type);
 
+      if (G_UNLIKELY (!gst_structure_parse_string (s, &value_end, &s,
+                  (type != G_TYPE_STRING))))
+        return FALSE;
+      /* Set NULL terminator for deserialization */
+      c = *value_end;
+      *value_end = '\0';
+
       ret = gst_value_deserialize (value, value_s);
       if (G_UNLIKELY (!ret))
         g_value_unset (value);
@@ -1999,7 +2021,7 @@ gst_structure_from_string (const gchar * string, gchar ** end)
     r++;
 
   name = r;
-  if (G_UNLIKELY (!gst_structure_parse_string (r, &w, &r))) {
+  if (G_UNLIKELY (!gst_structure_parse_string (r, &w, &r, TRUE))) {
     GST_WARNING ("Failed to parse structure string");
     goto error;
   }

gst/gstvalue.c

@@ -1803,8 +1803,10 @@ gst_value_deserialize_float (GValue * dest, const gchar * s)
 static gint
 gst_value_compare_string (const GValue * value1, const GValue * value2)
 {
-  if (!value1->data[0].v_pointer || !value2->data[0].v_pointer) {
+  if (G_UNLIKELY (!value1->data[0].v_pointer || !value2->data[0].v_pointer)) {
-    return GST_VALUE_UNORDERED;
+    /* if only one is NULL, no match - otherwise both NULL == EQUAL */
+    if (value1->data[0].v_pointer != value2->data[0].v_pointer)
+      return GST_VALUE_UNORDERED;
   } else {
     int x = strcmp (value1->data[0].v_pointer, value2->data[0].v_pointer);
 
@@ -1812,8 +1814,9 @@ gst_value_compare_string (const GValue * value1, const GValue * value2)
       return GST_VALUE_LESS_THAN;
     if (x > 0)
       return GST_VALUE_GREATER_THAN;
-    return GST_VALUE_EQUAL;
   }
+
+  return GST_VALUE_EQUAL;
 }
 
 static int
@@ -1822,9 +1825,13 @@ gst_string_measure_wrapping (const gchar * s)
   int len;
   gboolean wrap = FALSE;
 
-  if (s == NULL)
+  if (G_UNLIKELY (s == NULL))
     return -1;
 
+  /* Special case: the actual string NULL needs wrapping */
+  if (G_UNLIKELY (strcmp (s, "NULL") == 0))
+    return 4;
+
   len = 0;
   while (*s) {
     if (GST_ASCII_IS_STRING (*s)) {
@@ -1839,7 +1846,9 @@ gst_string_measure_wrapping (const gchar * s)
     s++;
   }
 
-  return wrap ? len : -1;
+  /* Wrap the string if we found something that needs
+   * wrapping, or the empty string (len == 0) */
+  return (wrap || len == 0) ? len : -1;
 }
 
 static gchar *
@@ -1866,6 +1875,7 @@ gst_string_wrap_inner (const gchar * s, int len)
   *e++ = '\"';
   *e = 0;
 
+  g_assert (e - d <= len + 3);
   return d;
 }
 
@@ -1875,7 +1885,7 @@ gst_string_wrap (const gchar * s)
 {
   int len = gst_string_measure_wrapping (s);
 
-  if (len < 0)
+  if (G_LIKELY (len < 0))
     return g_strdup (s);
 
   return gst_string_wrap_inner (s, len);
@@ -1888,7 +1898,7 @@ gst_string_take_and_wrap (gchar * s)
   gchar *out;
   int len = gst_string_measure_wrapping (s);
 
-  if (len < 0)
+  if (G_LIKELY (len < 0))
     return s;
 
   out = gst_string_wrap_inner (s, len);
@@ -1991,14 +2001,16 @@ gst_value_serialize_string (const GValue * value)
 static gboolean
 gst_value_deserialize_string (GValue * dest, const gchar * s)
 {
-  if (*s != '"') {
+  if (G_UNLIKELY (strcmp (s, "NULL") == 0)) {
+    g_value_set_string (dest, NULL);
+    return TRUE;
+  } else if (G_LIKELY (*s != '"')) {
     if (!g_utf8_validate (s, -1, NULL))
       return FALSE;
     g_value_set_string (dest, s);
     return TRUE;
   } else {
     gchar *str = gst_string_unwrap (s);
-
     if (G_UNLIKELY (!str))
       return FALSE;
     g_value_take_string (dest, str);

tests/check/gst/gststructure.c

@@ -213,6 +213,40 @@ GST_START_TEST (test_complete_structure)
 
 GST_END_TEST;
 
+GST_START_TEST (test_string_properties)
+{
+  GstCaps *caps1, *caps2;
+  GstStructure *st1, *st2;
+  gchar *str, *res1, *res2;
+
+  /* test escaping/unescaping */
+  st1 = gst_structure_new ("RandomStructure", "prop1", G_TYPE_STRING, "foo",
+      "prop2", G_TYPE_STRING, "", "prop3", G_TYPE_STRING, NULL,
+      "prop4", G_TYPE_STRING, "NULL", NULL);
+  str = gst_structure_to_string (st1);
+  st2 = gst_structure_from_string (str, NULL);
+  g_free (str);
+
+  fail_unless (st2 != NULL);
+
+  /* need to put stuctures into caps to compare */
+  caps1 = gst_caps_new_empty ();
+  gst_caps_append_structure (caps1, st1);
+  caps2 = gst_caps_new_empty ();
+  gst_caps_append_structure (caps2, st2);
+  res1 = gst_caps_to_string (caps1);
+  res2 = gst_caps_to_string (caps2);
+  fail_unless (gst_caps_is_equal (caps1, caps2),
+      "Structures did not match:\n\tStructure 1: %s\n\tStructure 2: %s\n",
+      res1, res2);
+  gst_caps_unref (caps1);
+  gst_caps_unref (caps2);
+  g_free (res1);
+  g_free (res2);
+}
+
+GST_END_TEST;
+
 GST_START_TEST (test_structure_new)
 {
   GstStructure *s;
@@ -421,18 +455,6 @@ GST_START_TEST (test_structure_nested_from_and_to_string)
 
 GST_END_TEST;
 
-GST_START_TEST (test_empty_string_fields)
-{
-  GstStructure *s;
-
-  s = gst_structure_empty_new ("con/struct");
-  ASSERT_WARNING (gst_structure_set (s, "layout", G_TYPE_STRING, "", NULL));
-  gst_structure_set (s, "debug-string", G_TYPE_STRING, NULL, NULL);
-  gst_structure_free (s);
-}
-
-GST_END_TEST;
-
 GST_START_TEST (test_vararg_getters)
 {
   GstStructure *s;
@@ -546,13 +568,13 @@ gst_structure_suite (void)
   tcase_add_test (tc_chain, test_from_string);
   tcase_add_test (tc_chain, test_to_string);
   tcase_add_test (tc_chain, test_to_from_string);
+  tcase_add_test (tc_chain, test_string_properties);
   tcase_add_test (tc_chain, test_complete_structure);
   tcase_add_test (tc_chain, test_structure_new);
   tcase_add_test (tc_chain, test_fixate);
   tcase_add_test (tc_chain, test_fixate_frac_list);
   tcase_add_test (tc_chain, test_structure_nested);
   tcase_add_test (tc_chain, test_structure_nested_from_and_to_string);
-  tcase_add_test (tc_chain, test_empty_string_fields);
   tcase_add_test (tc_chain, test_vararg_getters);
   return s;
 }

tests/check/gst/gstvalue.c

@@ -428,7 +428,8 @@ GST_START_TEST (test_string)
   gchar *try[] = {
     "Dude",
     "Hi, I'm a string",
-    "tüüüt!"
+    "tüüüt!",
+    "\"\""                      /* Empty string */
   };
   gchar *tmp;
   GValue v = { 0, };
@@ -465,7 +466,8 @@ GST_START_TEST (test_deserialize_string)
     {
     "", ""},                    /* empty strings */
     {
-    "\"\"", ""},                /* FAILURES */
+    "\"\"", ""},                /* quoted empty string -> empty string */
+        /* Expected FAILURES: */
     {
     "\"", NULL},                /* missing second quote */
     {
@@ -538,6 +540,12 @@ GST_START_TEST (test_value_compare)
   fail_unless (gst_value_compare (&value1, &value2) == GST_VALUE_LESS_THAN);
   fail_unless (gst_value_compare (&value2, &value1) == GST_VALUE_GREATER_THAN);
   fail_unless (gst_value_compare (&value1, &value1) == GST_VALUE_EQUAL);
+  /* Test some NULL string comparisons */
+  g_value_set_string (&value2, NULL);
+  fail_unless (gst_value_compare (&value1, &value2) == GST_VALUE_UNORDERED);
+  fail_unless (gst_value_compare (&value2, &value1) == GST_VALUE_UNORDERED);
+  fail_unless (gst_value_compare (&value2, &value2) == GST_VALUE_EQUAL);
+
   g_value_unset (&value1);
   g_value_unset (&value2);