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.

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 ?)

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?

Not sure mandated registered types are a good idea, we had that with the multichannel enums in the caps in 0.10..

debugging caps printed as 3F5-24A will be a pain, I'd much rather have
option1,+option2,+option3,-option4 or something like that

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.

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.

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.

Another idea is to use your existing code, but apply it to any registered GFlag type, so you have both pointers free.

Or serialise it in binary form instead of hex (1011:0111)?

Binary form still doesn't tell us what it actually means

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.

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.

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.

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.

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.

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.

(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.

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.

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.

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.

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.

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.

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

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.

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.

That can also be added later in a backwards-compatible way on top of this.

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?

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.

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?

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.

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.

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).

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

> 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 '~' :)

(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 :)

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>)

We could instead use commas and declare that the first character of each is an operator, so we could so +flush,+skip,-snap-before

> 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?

Or another separator?

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.

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.

Updated flattened patch with bug fixes that make the derived types work. I kept the serialisation format the same - I think it's fine.

I'm getting ready to merge multiview changes that rely on this, so in the absence of any other comments, pushed to master :)