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 746373 - New fundamental type for caps - GstFlagSet
New fundamental type for caps - GstFlagSet
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-17 22:28 UTC by Jan Schmidt
Modified: 2015-05-25 06:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstvalue: Add GstFlagSet type (23.23 KB, patch)
2015-03-17 22:29 UTC, Jan Schmidt
none Details | Review
gstvalue: Add GstFlagSet type (24.25 KB, patch)
2015-03-18 02:36 UTC, Jan Schmidt
none Details | Review
gstvalue: Add GstFlagSet type (24.25 KB, patch)
2015-03-19 12:39 UTC, Jan Schmidt
none Details | Review
flagset: Make GstFlagSet a derivable type, for debugging purposes. (10.47 KB, patch)
2015-03-19 12:39 UTC, Jan Schmidt
none Details | Review
gstvalue: Add GstFlagSet type (24.25 KB, patch)
2015-03-19 12:44 UTC, Jan Schmidt
none Details | Review
flagset: Make GstFlagSet a derivable type, for debugging purposes. (10.47 KB, patch)
2015-03-19 12:44 UTC, Jan Schmidt
none Details | Review
flagset: Make GstFlagSet a derivable type, for debugging purposes. (16.92 KB, patch)
2015-03-19 12:49 UTC, Jan Schmidt
none Details | Review
flagset: Make GstFlagSet a derivable type, for debugging purposes. (16.92 KB, patch)
2015-03-19 13:24 UTC, Jan Schmidt
none Details | Review
flagset: Add flag-nicks form, switch back to 32-bit (16.65 KB, patch)
2015-03-22 11:56 UTC, Jan Schmidt
none Details | Review
gstvalue: Add GstFlagSet type (50.13 KB, patch)
2015-05-09 12:50 UTC, Jan Schmidt
none Details | Review

Description Jan Schmidt 2015-03-17 22:28:16 UTC
GstFlagSet is a new type for caps, which is like a bitmask field, but which can be negotiated more efficiently.

A GstFlagSet is a bitfield of flags, plus a bitmask that indicates which flags are important. Together, they allow an element to more compactly express support for various flag values than with a GstBitmask.

For example, an element that wants to support receiving caps where a particular flag must be declared, but can be either true or false - the receiver doesn't care. Currently, we could add a boolean field to the caps, and the receiver can put field = (boolean) { TRUE, FALSE } in its caps. When there are many such properties of the stream, the number of extra caps fields grows quickly, and if there are interdependencies between the properties, then the caps has to become a list of intersecting values.

With a GstFlagSet, and element can express combinations of flags, using the mask to indicate a 'dont care' value for any given flag.

A fixed GstFlagSet has a mask of (guint64)(-1) to indicate all flags bits must exactly match when intersecting caps.
Comment 1 Jan Schmidt 2015-03-17 22:29:10 UTC
Created attachment 299658 [details] [review]
gstvalue: Add GstFlagSet type

GstFlagSet is a new type designed for negotiating sets
of boolean capabilities flags, consisting of a 64-bit
flags bitfield and 64-bit mask field. The mask field
indicates which of the flags bits an element needs to have
as specific values, and which it doesn't care about.

This allows efficient negotiation of arrays of boolean
capabilities.
Comment 2 Nicolas Dufresne (ndufresne) 2015-03-17 22:48:39 UTC
Would be nice to show how this would be shown in parse launch syntax to, and maybe a real life scenario where it would be appropriate to use (I suppose there is in the 3D world ?)
Comment 3 Olivier Crête 2015-03-17 23:45:32 UTC
I'd really like if such a thing also mandated that the flags be part of a registered GFlags type so they can be printed cleanly instead of %x-%x... And some example use-case where the current caps are not enough?
Comment 4 Tim-Philipp Müller 2015-03-17 23:57:11 UTC
Not sure mandated registered types are a good idea, we had that with the multichannel enums in the caps in 0.10..
Comment 5 Olivier Crête 2015-03-18 00:18:03 UTC
debugging caps printed as 3F5-24A will be a pain, I'd much rather have
option1,+option2,+option3,-option4 or something like that
Comment 6 Jan Schmidt 2015-03-18 02:36:56 UTC
Created attachment 299671 [details] [review]
gstvalue: Add GstFlagSet type

GstFlagSet is a new type designed for negotiating sets
of boolean capabilities flags, consisting of a 64-bit
flags bitfield and 64-bit mask field. The mask field
indicates which of the flags bits an element needs to have
as specific values, and which it doesn't care about.

This allows efficient negotiation of arrays of boolean
capabilities.
Comment 7 Jan Schmidt 2015-03-18 03:54:18 UTC
Added the new API to the docs, and fixed a bug I noticed reviewing my own code.

I can't think of any sensible way to associate the bitfield with a GFlags gtype. There's no extra space in a GValue for storing that, it makes deserialisation harder, and means you can't use a flagset in pad template caps, because you can't deserialise from the registry before the desired type is loaded.

The current serialisation is like GstBitmask, with a mask field appending via ':'

One of the examples in the unit test:

test/x-caps,field=ffd81d:fffff0

My use case in the multiview branch is to handle flags regarding interpretation of the stereo views, such as whether or not one of the views is horizontally or vertically flipped. GstFlagSet lets a sink compactly specify that (for example) the left view must be horizontally mirrored for display, letting us do that transformation upstream if necessary.
Comment 8 Olivier Crête 2015-03-18 04:12:20 UTC
Is 56a7b:7bc85 compatible with 35fbc:67f25? yYu can't tell by just looking at it. And wtf does ffd81f mean? I'm very much opposed to merging something as undebuggable as this. Maybe you can instead allocate an extra structure to describe each bit and have a much more verbose serialized format. With a bit of cleverness, you can probably attach the description of the bits to the fieldname, or just only do 32 bits per field and put the description in the other pointer.

For the record, the only reason the bitmask are not a complete disaster is that we only allow equality.
Comment 9 Olivier Crête 2015-03-18 04:15:50 UTC
Another idea is to use your existing code, but apply it to any registered GFlag type, so you have both pointers free.
Comment 10 Tim-Philipp Müller 2015-03-18 10:11:37 UTC
Or serialise it in binary form instead of hex (1011:0111)?
Comment 11 Olivier Crête 2015-03-18 15:14:11 UTC
Binary form still doesn't tell us what it actually means
Comment 12 Jan Schmidt 2015-03-19 12:39:43 UTC
Created attachment 299805 [details] [review]
gstvalue: Add GstFlagSet type

GstFlagSet is a new type designed for negotiating sets
of boolean capabilities flags, consisting of a 64-bit
flags bitfield and 64-bit mask field. The mask field
indicates which of the flags bits an element needs to have
as specific values, and which it doesn't care about.

This allows efficient negotiation of arrays of boolean
capabilities.
Comment 13 Jan Schmidt 2015-03-19 12:39:52 UTC
Created attachment 299806 [details] [review]
flagset: Make GstFlagSet a derivable type, for debugging purposes.

Add a gst_register_flagset() function, which associates a new
GstFlagSet derived type with an existing GFlags gtype. When
serializing a GstFlagSet with an associated set of GFlags,
also serialize a human-readable form of the flags for
easier debugging.
Comment 14 Jan Schmidt 2015-03-19 12:44:31 UTC
Created attachment 299808 [details] [review]
gstvalue: Add GstFlagSet type

GstFlagSet is a new type designed for negotiating sets
of boolean capabilities flags, consisting of a 64-bit
flags bitfield and 64-bit mask field. The mask field
indicates which of the flags bits an element needs to have
as specific values, and which it doesn't care about.

This allows efficient negotiation of arrays of boolean
capabilities.
Comment 15 Jan Schmidt 2015-03-19 12:44:38 UTC
Created attachment 299809 [details] [review]
flagset: Make GstFlagSet a derivable type, for debugging purposes.

Add a gst_register_flagset() function, which associates a new
GstFlagSet derived type with an existing GFlags gtype. When
serializing a GstFlagSet with an associated set of GFlags,
also serialize a human-readable form of the flags for
easier debugging.
Comment 16 Jan Schmidt 2015-03-19 12:49:28 UTC
Created attachment 299810 [details] [review]
flagset: Make GstFlagSet a derivable type, for debugging purposes.

Add a gst_register_flagset() function, which associates a new
GstFlagSet derived type with an existing GFlags gtype. When
serializing a GstFlagSet with an associated set of GFlags,
also serialize a human-readable form of the flags for
easier debugging.
Comment 17 Jan Schmidt 2015-03-19 12:57:00 UTC
Gah. Sorry for the extra commit noise - it's late and I'm jet-lagged.

I came up with this change, that hopefully addresses Olivier's concern. Attached as a second patch for easier reviewing. I'll push it upstream as one commit.

I've added a gst_register_flagset() function, which takes the GType of an existing GFlags and associates it with a new type. When serializing a GstFlagSet, it checks if there's an associated GFlags type, and if so appends a 'human readable' form of the flags, like this:

Given flags and mask respectively of

GST_SEEK_FLAG_FLUSH | GST_SEEK_FLAG_TRICKMODE | GST_SEEK_FLAG_TRICKMODE_KEY_UNITS

and 

GST_SEEK_FLAG_FLUSH | GST_SEEK_FLAG_TRICKMODE | GST_SEEK_FLAG_TRICKMODE_NO_AUDIO;

it will be serialised as:

11:111:+GST_SEEK_FLAG_FLUSH+GST_SEEK_FLAG_TRICKMODE/GST_SEEK_FLAG_TRICKMODE_NO_AUDIO

+ means the flag must be set, / means it must not be set. I thought about using '-', but '-' is a valid character in the nick for GFlags, so it might get confusing.

For deserialisation, the human-readable form is ignored - only the hex strings at the start are important.

For static caps and pad templates, it's still possible to use the GST_TYPE_FLAG_SET as the GValue type, and it will be serialized only as the hex values, and therefore be deserialisable from the registry. For intersection, it doesn't matter what the final GstFlagSet type is - it's possible to intersect a derived GstFlagSet type with a GValue containing a 'raw' GstFlagSet.

In the future, I think we could add extra types as a registry feature, to be loaded as a plugin that will provide a GType by name - then we could serialise the full human-readable form in the registry for gst-inspect and friends.
Comment 18 Nicolas Dufresne (ndufresne) 2015-03-19 13:01:38 UTC
(In reply to Jan Schmidt from comment #17)
> 
> it will be serialised as:
> 
> 11:111:+GST_SEEK_FLAG_FLUSH+GST_SEEK_FLAG_TRICKMODE/
> GST_SEEK_FLAG_TRICKMODE_NO_AUDIO

We should try and follow the other flags convention when come to human representation. So far the convention seems to be lower caps and no name-space.
Comment 19 Jan Schmidt 2015-03-19 13:08:35 UTC
It's actually a cut and paste of the GFlags serialization code. The only change is to print "/" instead of "+" when the flag is negated.
Comment 20 Nicolas Dufresne (ndufresne) 2015-03-19 13:12:26 UTC
Actually, we do have an inconsitency, it's lower in properties, but capital in caps. Anyway, the namespace don't show up for video format= and audio format=. I also don't recall having to add the namespace for channel mask.
Comment 21 Jan Schmidt 2015-03-19 13:20:36 UTC
The inconsistency is in gst-inspect - it serialises property strings using the GFlags nick.

The de-serialisation code seems to check both the value and the nick, so either will work.

Using the nick definitely produces an easier-to-read string, and since it's only for humans (not for deserialising) that's more important than consistency with the GFlags serialisation.

I'm not sure what you mean about 'video format= and audio format='. Those aren't flags types, they're strings.
Comment 22 Jan Schmidt 2015-03-19 13:24:23 UTC
Created attachment 299818 [details] [review]
flagset: Make GstFlagSet a derivable type, for debugging purposes.

Add a gst_register_flagset() function, which associates a new
GstFlagSet derived type with an existing GFlags gtype. When
serializing a GstFlagSet with an associated set of GFlags,
also serialize a human-readable form of the flags for
easier debugging.
Comment 23 Jan Schmidt 2015-03-19 13:34:23 UTC
Using the GFlags nick instead of name yields:

11:111:+flush+trickmode/trickmode-no-audio

I think that looks OK, although maybe it should concatenate in the reverse order. It relies on g_flags_get_first_value to split out the bits, which enumerates in the order the flags were declared when g_flags_register_static was called, which is typically lowest numeric value first in sane code.

There's no guarantee of any ordering in the flags value array anyway, but just reversing the order of the flags at least makes them in the same order as the bits that represent them in the flags/mask hex values.
Comment 24 Olivier Crête 2015-03-19 15:56:59 UTC
In your example, the key-units is still invisible to the human reader. I'd just drop the binary form from the serialisation, maybe something a serialisation like: +flush,+trickmode,trickmode-key-units,/trickmode-no-audio
Comment 25 Jan Schmidt 2015-03-19 19:36:25 UTC
Speak for yourself, human reader. I can see it just fine - it's not there.

It wasn't specified in the mask, so actually it was removed from the flags. There's no need to keep a 'dont-care' value, so it gets normalised away - like simplifying a GstFraction.

"I'd just drop the binary form from the serialisation, maybe something a serialisation like: +flush,+trickmode,trickmode-key-units,/trickmode-no-audio" misses the whole point - apart from not wasting CPU cycles on text parsing, the binary form can be parsed even if the flags type doesn't exist - ie from the registry.
Comment 26 Jan Schmidt 2015-03-19 20:24:06 UTC
To make it easier to use in filter caps strings on gst-launch, it might be useful to also support a mode where you can parse from the text description - but that will still depend on the relevant flags GType having been registered - which won't be possible without the new registry feature I mentioned above.
Comment 27 Jan Schmidt 2015-03-19 20:24:46 UTC
That can also be added later in a backwards-compatible way on top of this.
Comment 28 Olivier Crête 2015-03-19 20:30:50 UTC
The problem is that writing the binary form in the pad templates is not trivial, and it's quite easy to get it wrong. The serialized form is mostly for humans, to write caps by hand and such, wasting a bit of CPU time parsing it seems like no problem at all Having the information duplicated means that it's almost guaranteed to get out of sync. If you start with 011:010:myflag, it's way to end up with something like 011:110:+myflag,/myflag2, which looks ok to human readers, but actually is parsed to something else. Not being usable from gst-launch would be a big regression. Possibly they should just be registered in the core.

How many of these flags do you have? Is the overhead from using a couple of G_TYPE_BOOLEAN so much?
Comment 29 Tim-Philipp Müller 2015-03-19 20:40:56 UTC
Olivier, what you want is not really possible unless we register all possible types in core. We had the same problem with the audio position enums in 0.10 btw, so it's not a particularly new problem. We can make plugin features for types in future if we really want to, it can all be solved, but it feels a bit over the top to block on this, especially seeing that our primary use case for this so far is just to negotiate a small number of options. 

Using multiple booleans is just fugly IMO, and flag sets are really a better fit to the problem space.
Comment 30 Tim-Philipp Müller 2015-03-19 20:45:23 UTC
Also, I think it should be possible to use nicks in the pad template strings in the code, they'll just not be serialised or deserialised like that in the registry?
Comment 31 Jan Schmidt 2015-03-22 11:56:13 UTC
Created attachment 300071 [details] [review]
flagset: Add flag-nicks form, switch back to 32-bit

Make it so that it's possible to parse a GFlags style
serialisation of a flagset, without the hex portion on the
front. ie, +flag1/flag2/flag3+flag4, to indicate that
flag1 & flag4 must be set, and flag2/flag3 must be unset,
and any other flags are don't-care.

Make flagsets 32-bits again, sine that's all a GFlags can hold.
Comment 32 Tim-Philipp Müller 2015-03-24 12:45:59 UTC
I think that last patch addresses all usability/debuggability concerns raised then?

We can use foo=(GstBarFlags)+blue/red+pink in template caps now, right?

In practice, pretty much all caps in debug logs will have the with-nick serialisation because plugins will be loaded and types registered.

Only exception is when caps subset checks are done against element factory details from the registry, where only the numeric variant will be stored. I think that's fine.
Comment 33 Tim-Philipp Müller 2015-03-24 12:47:26 UTC
The only remaining nit I have is that I don't like / as operator for 'this must be unset', it just looks weird. But I'm sure you have already considered all possibilities. I think '~' might be nicer if we can't use '-', since it's close to '-' (even if it has a different meaning as C operator).
Comment 34 Jan Schmidt 2015-03-24 13:02:09 UTC
The reason not to use '-' is that it's the standard word separator for flags nicks. Even having something visually similar will make it harder to read strings.

Compare:

+flush+skip/snap-before

to:

+flush+skip~snap-before

Equally, ! is a bad choice for something we'll want to use in parse_launch() strings, since it's the link operator.

I picked / because it's like the 'not' operator for written boolean expressions - with a bar over the top of the expression for negation
Comment 35 Tim-Philipp Müller 2015-03-24 13:11:33 UTC
> Compare:
> 
> +flush+skip/snap-before
> 
> +flush+skip~snap-before

Ok, but we have that problem already with e.g.:

/flush/skip+snap-before/bar

The problem is the visual impact of the '-' and there's not much we can do about that in any case.

I can live fine with the '/' though, just wanted to float the '~' :)
Comment 36 Jan Schmidt 2015-03-24 13:16:36 UTC
(In reply to Tim-Philipp Müller from comment #35)
> > Compare:
> > 
> > +flush+skip/snap-before
> > 
> > +flush+skip~snap-before
> 
> Ok, but we have that problem already with e.g.:
> 
> /flush/skip+snap-before/bar

I'm not sure which problem you're illustrating here :)
Comment 37 Tim-Philipp Müller 2015-03-24 13:24:11 UTC
Awkward readability if there's a - in any nicks. Your examples made it look like that would be worse with a ~ than with a /, but it's just as bad with a + ;)

(</bikeshed>)
Comment 38 Olivier Crête 2015-03-25 15:53:29 UTC
We could instead use commas and declare that the first character of each is an operator, so we could so +flush,+skip,-snap-before
Comment 39 Tim-Philipp Müller 2015-03-26 10:15:04 UTC
> We could instead use commas and declare that the first character
> of each is an operator, so we could so +flush,+skip,-snap-before

I suspect this would make it awkward to use in caps strings, because then you'd need to escape stuff so that GstStructure doesn't misinterpret the commas as field dividers?
Comment 40 Olivier Crête 2015-03-26 15:16:11 UTC
Or another separator?
Comment 41 Tim-Philipp Müller 2015-03-26 15:24:46 UTC
foo-bar=(string)+flush|+skip|-snap-before|-bar

foo-bar=(string)+flush/+skip/-snap-before/-bar

would both work, I suppose, even if | is a shell thing of course.

Both seem marginally more readable to me than:

foo-bar=(string)+flush+skip/snap-before/bar


Dunno, I'll leave it up to Jan. I think it's fine as is too.
Comment 42 Jan Schmidt 2015-05-09 12:50:37 UTC
Created attachment 303123 [details] [review]
gstvalue: Add GstFlagSet type

GstFlagSet is a new type designed for negotiating sets
of boolean capabilities flags, consisting of a 32-bit
flags bitfield and 32-bit mask field. The mask field
indicates which of the flags bits an element needs to have
as specific values, and which it doesn't care about.

This allows efficient negotiation of arrays of boolean
capabilities.

The standard serialisation format is FLAGS:MASK, with
flags and mask fields expressed in hexadecimal, however
GstFlagSet has a gst_register_flagset() function, which
associates a new GstFlagSet derived type with an existing
GFlags gtype. When serializing a GstFlagSet with an
associated set of GFlags, it also serializes a human-readable
form of the flags for easier debugging.

It is possible to parse a GFlags style serialisation of a
flagset, without the hex portion on the front. ie,
+flag1/flag2/flag3+flag4, to indicate that
flag1 & flag4 must be set, and flag2/flag3 must be unset,
and any other flags are don't-care.
Comment 43 Jan Schmidt 2015-05-09 12:53:59 UTC
Updated flattened patch with bug fixes that make the derived types work. I kept the serialisation format the same - I think it's fine.
Comment 44 Jan Schmidt 2015-05-25 06:31:35 UTC
I'm getting ready to merge multiview changes that rely on this, so in the absence of any other comments, pushed to master :)