After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 777375 - paramspec: Add gst_param_spec_array_list for GST_TYPE_ARRAY
paramspec: Add gst_param_spec_array_list for GST_TYPE_ARRAY
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.11.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 777376
 
 
Reported: 2017-01-17 10:02 UTC by Vivia Nikolaidou
Modified: 2017-02-23 19:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch (7.76 KB, patch)
2017-01-17 10:03 UTC, Vivia Nikolaidou
none Details | Review
0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch (7.76 KB, patch)
2017-01-31 16:45 UTC, Vivia Nikolaidou
none Details | Review
0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch (35.92 KB, patch)
2017-01-31 16:46 UTC, Vivia Nikolaidou
none Details | Review
0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch (38.15 KB, patch)
2017-01-31 17:04 UTC, Vivia Nikolaidou
reviewed Details | Review
value: Add GstValueArray serialize/deserialize test (2.44 KB, patch)
2017-02-02 15:19 UTC, Vivia Nikolaidou
reviewed Details | Review

Description Vivia Nikolaidou 2017-01-17 10:02:18 UTC
See attached patch
Comment 1 Vivia Nikolaidou 2017-01-17 10:03:02 UTC
Created attachment 343629 [details] [review]
0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch
Comment 2 Tim-Philipp Müller 2017-01-17 10:16:38 UTC
I would really like to try and avoid to promote use of the Gst array type outside of struct/caps handling - isn't there some other way?

Why can't GValueArray not be used for example?
Comment 3 Sebastian Dröge (slomo) 2017-01-17 10:20:06 UTC
<vivia> __tim: the initial thought was to be able to pass the serialized array on gst-launch, but of course that doesn't work unless someone actually implements the deserialization for that - which is what i wanted to do next :) cc slomo 
<slomo> vivia: __tim: ↑ that, yes (and i was sure serialization/deserialization worked in 0.10... for the channel positions!)
Comment 4 Sebastian Dröge (slomo) 2017-01-17 10:23:57 UTC
Review of attachment 343629 [details] [review]:

Looks good to me, except for the missing deserialization (serialization is there), which is the part that makes this useful over GValueArray
Comment 5 Sebastian Dröge (slomo) 2017-01-17 10:26:00 UTC
Review of attachment 343629 [details] [review]:

Looks good to me, except for the missing deserialization (serialization is there), which is the part that makes this useful over GValueArray
Comment 6 Sebastian Dröge (slomo) 2017-01-17 10:26:09 UTC
Review of attachment 343629 [details] [review]:

Looks good to me, except for the missing deserialization (serialization is there), which is the part that makes this useful over GValueArray
Comment 7 Sebastian Dröge (slomo) 2017-01-17 10:26:09 UTC
Review of attachment 343629 [details] [review]:

Looks good to me, except for the missing deserialization (serialization is there), which is the part that makes this useful over GValueArray
Comment 8 Tim-Philipp Müller 2017-01-17 10:28:02 UTC
You can just as well implement serialisation/deserialisation for GValueArray then, no?

Also, how does one pass this through g_object_set()/get() ?
Comment 9 Vivia Nikolaidou 2017-01-17 10:29:30 UTC
Manually wrap the values into the array (e.g. in a loop) and g_object_set_value.
Comment 10 Tim-Philipp Müller 2017-01-17 10:34:52 UTC
I still don't see why we have to use Gst's array here.
Comment 11 Sebastian Dröge (slomo) 2017-01-17 10:51:35 UTC
Otherwise we have to implement serialization and deserialization for GValueArray (which is deprecated btw), while we already have serialization at least for GST_TYPE_ARRAY and also use that one in caps and elsewhere for such things. Caps and properties seem similar in this regard.


Ideally this would of course be a GArray<GArray<float>> but that can't be safely put into properties.
Comment 12 Sebastian Dröge (slomo) 2017-01-19 10:56:20 UTC
So, let's get this in once there's also deserialization (which is the main point of using this instead of GValueArray)?
Comment 13 Tim-Philipp Müller 2017-01-19 11:06:44 UTC
If you're asking I still don't want this and I think we should take some time to think harder about how we can do this better ;)
Comment 14 Vivia Nikolaidou 2017-01-27 15:15:33 UTC
After internal discussion, we decided that the only thing that really makes sense here (about deserialization) is to either let the internal type of the array be detected automatically, or have it in parentheses before each internal element separately, e.g. (for doubles):

<<(double)1, <(double)0>, <1.0, 0.0>>
Comment 15 Vivia Nikolaidou 2017-01-31 16:45:23 UTC
Created attachment 344651 [details] [review]
0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch

Serialization is also there now! \o/ Moved some non-GstStructure code around from gststructure.c to gstvalue.c
Comment 16 Vivia Nikolaidou 2017-01-31 16:46:14 UTC
Created attachment 344653 [details] [review]
0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch

Oops, I must have attached the wrong file. Sorry for the noise.
Comment 17 Vivia Nikolaidou 2017-01-31 17:04:19 UTC
Created attachment 344654 [details] [review]
0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch

Fixed serialization: don't print type (inside lists and arrays) for GstStructures, but always print it otherwise.
Comment 18 Sebastian Dröge (slomo) 2017-01-31 17:12:12 UTC
(In reply to Vivia Nikolaidou from comment #17)
> Created attachment 344654 [details] [review] [review]
> 0001-paramspec-Add-gst_param_spec_array_list-for-GST_TYPE.patch
> 
> Fixed serialization: don't print type (inside lists and arrays) for
> GstStructures, but always print it otherwise.

So for normal serialization of values that contain a list/array, we will provide the type before each value. That is, we get e.g. "<(double) 1, (double) 2>". This is needed so that we can always do gst_value_deserialize(gst_value_serialize(value)) and get the same value back.

However for structures we did not do that, they did do magic for lists/arrays. And also ensure that every array/list element has the same type. That is, we get "(double) <1, 2>".

The patch keeps this behaviour (and should get some unit tests), although it is inconsistent. Existing code might depend on that. Just as before, GstStructure can also parse the other variant though ("<(double) 1, (double) 2>").
Comment 19 Vivia Nikolaidou 2017-02-02 15:19:00 UTC
Created attachment 344790 [details] [review]
value: Add GstValueArray serialize/deserialize test
Comment 20 Sebastian Dröge (slomo) 2017-02-22 19:18:57 UTC
I'll merge this tomorrow after splitting up the patch into 3 (3 different changes here...) and then write some more tests for the GstStructure serialization too.
Comment 21 Sebastian Dröge (slomo) 2017-02-23 19:03:46 UTC
commit 63775ac6e32b4d4d6642ed705342374a30aeaaa0
Author: Vivia Nikolaidou <vivia@toolsonair.com>
Date:   Thu Feb 23 20:52:39 2017 +0200

    value: Add deserialization for arrays/lists outside GstStructures
    
    This is mostly useful for properties of those types when used in
    gst-launch or similar.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=777375

commit a3cfcbfeded337778000ac061c8b6239208f448c
Author: Vivia Nikolaidou <vivia@toolsonair.com>
Date:   Thu Feb 23 20:47:30 2017 +0200

    value: Always add the type name to elements when serializing arrays/lists
    
    But only when serializing outside of GstStructures, because in case of
    GstStructure the type is already preprended to the array/list and the
    GstStructure API makes sure that they have the same "generic" type so
    deserialization works properly.
    
    This keeps serialization of GstStructures the same as before, and the
    GstCaps unit tests already test for that. However when serializing
    standalone arrays/lists get the types added now.

commit 33118f61180759adc7b2b3c78b19a9c1340d995d
Author: Vivia Nikolaidou <vivia@toolsonair.com>
Date:   Thu Feb 23 20:22:03 2017 +0200

    value: Move list/array serialization/deserialization functions from GstStructure to GstValue
    
    https://bugzilla.gnome.org/show_bug.cgi?id=777375

commit 978df5d0ffb5006da9ae28ca875b0d8a725fd925
Author: Vivia Nikolaidou <vivia@toolsonair.com>
Date:   Thu Feb 23 20:16:17 2017 +0200

    paramspecs: Add GstParamSpecArray for GST_TYPE_ARRAY typed properties
    
    These are mostly useful to get our automatic
    serialization/deserialization from strings and simple usage from
    gst-launch or similar.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=777375