GNOME Bugzilla – Bug 748814
discoverer: add serialization/deserialization methods
Last modified: 2015-05-19 16:58:08 UTC
[API] gst_discoverer_to_string [API] gst_discoverer_from_string + Serializes to XML + Parses back with GMarkupParser + Adds a test + Does not serialize potential GstToc (s)
Created attachment 302784 [details] [review] discoverer: Add serialization methods.
Not really a fan of this tbh, but if you must.. It feels like this is all rather ad-hoc and is pretty much going to be deprecated soon again seeing plans in the pipeline. Reminds me too much of all that horrid xml pipeline serialisation stuff. Why XML and not e.g. GVariant? Why serialise everything individually instead of tags and caps? Fractions should be serialised as one value and not numerator and denominator separately. The function names in the commit message don't match the actual API.
Created attachment 302812 [details] [review] discoverer: Add serialization methods. [API] gst_discoverer_info_to_string [API] gst_discoverer_info_from_string + Serializes to XML + Parses back with GMarkupParser + Adds a test + Does not serialize potential GstToc (s)
Created attachment 302934 [details] [review] discoverer: Add serialization methods. [API] gst_discoverer_info_to_string [API] gst_discoverer_info_from_string + Serializes with g_variant_print + Parses back with g_variant_parse + Adds a test + Does not serialize potential GstToc (s)
Comment on attachment 302934 [details] [review] discoverer: Add serialization methods. Thanks, I prefer the GVariant variant, since the parsing is less awkward IMHO. - as discussed on IRC, we should just provide to/from_variant(); if anyone wants to convert that to/from strings, they're free to do that using the GVariant API themselves, but returning the GVariant is more useful for those who just want to store/retrieve the data as a blob of bytes (otherwise they'd have to first convert the string we return back to a variant and then store that). - all these g_quark_to_string (_XYZ_QUARK) should just be string constants ("xyz") or defines that are string constants. Quarks make no sense here as far as I can tell and just incur unnecessary locking/lookup/memory overhead. - some of the code could probably be simplified, e.g. let's take serialize_common_stream_info(): - {sv} + "foo", g_variant_from_string("bar") could just be {ss} = "foo", "bar" unless you need the possibility to stash other types than strings in here later (you can still change it later and decide depending on the type then though). - not everything has to be stuffed into a dict, and fields can be made nullable, so if we don't have some piece of info we can just leave it as not set - I'd probably use something like (va(yv)), [I hope I remember the notation right here; I'm not exactly a GVariant ninja, so take with a grain of salt] where the first variant is the common info which could just be (msmsmsms) ie. four maybe strings (id, caps, tags, misc). We can still extend that later if needed. Followed by an array of child infos: the 'y' would be a byte/char to mark the type that follows: audio/video/sub/container (a/v/s/c) Perhaps even the top-level should have this byte marker for consistency, dunno. - similar thoughts apply to all the other infos - if you use dicts you store all the names ('framerate-denom') etc. in the serialisation, whereas if you made it e.g. mumum(uu)m(uu) for then the "schema" is implicit and you save a bit of overhead. Less nice to read when serialised of course, because one needs to know what the fields mean.
I, too, am far from being a GVariant ninja :) I went with my initial understanding of the API, and the notations you've written mostly look like chinese to me, I wouldn't mind if you implemented your suggestions, otherwise I'll get back to that patch soonish but I'll need some more time to discover GVariant, the current code is mostly based off the usage examples I adapted. Regarding the key-value approach, doesn't it also ensure extensibility aside from readability ?
Created attachment 303435 [details] [review] discoverer: Add serialization methods. [API] gst_discoverer_info_to_variant [API] gst_discoverer_info_from_variant [API] GstDiscovererSerializeFlags + Serializes as a GVariant + Adds a test + Does not serialize potential GstToc (s)
I've run the test under valgrind, it doesn't leak, and it tests most of the code paths (except for subtitles but I'm quite sure it doesn't leak either :)
Created attachment 303440 [details] [review] discoverer: Add serialization methods. [API] gst_discoverer_info_to_variant [API] gst_discoverer_info_from_variant [API] GstDiscovererSerializeFlags + Serializes as a GVariant + Adds a test + Does not serialize potential GstToc (s)
I'm happy with the API, so I think this can go in and we can still fine-tune the GVariant details before the 1.6 release if needed (didn't check in detail, but looks fine at first glance, and I assume you're satisfied with regard to extensability too?).
Please sprinkle some 'Since: 1.6' markers in the gtk-doc chunks.
Comment on attachment 303440 [details] [review] discoverer: Add serialization methods. Please push after adding gtk-doc Since: 1.6 markers.
Review of attachment 303440 [details] [review]: commit 2e423dd129f9ea509181a9f3dc952a29fbfc7251 Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Sun May 3 03:18:28 2015 +0200 discoverer: Add serialization methods. [API] gst_discoverer_info_to_variant [API] gst_discoverer_info_from_variant [API] GstDiscovererSerializeFlags + Serializes as a GVariant + Adds a test + Does not serialize potential GstToc (s) https://bugzilla.gnome.org/show_bug.cgi?id=748814