Btw, I just wrote a patch to use GSt array (<s1,s2> syntax), but found that the feature is broken. It's really easy to make it crash, while that worked in now deprecated videoparse. I guess this new element is not well tested and should have stayed longer in -bad ...

(In reply to Nicolas Dufresne (stormer) from comment #1)
> Btw, I just wrote a patch to use GSt array (<s1,s2> syntax), but found that
> the feature is broken.

We had no deserialization for GST_TYPE_ARRAY until a few weeks ago though :)

> It's really easy to make it crash, while that worked
> in now deprecated videoparse. I guess this new element is not well tested
> and should have stayed longer in -bad ...

It's in -bad since 1.10 and seemed to have worked for everybody so far, and other than the old element it even has tests.

So best to add new tests for the things that are broken, and then fix them.

(In reply to Sebastian Dröge (slomo) from comment #2)
> (In reply to Nicolas Dufresne (stormer) from comment #1)
> > Btw, I just wrote a patch to use GSt array (<s1,s2> syntax), but found that
> > the feature is broken.
> 
> We had no deserialization for GST_TYPE_ARRAY until a few weeks ago though :)

That's why videoparse was using a string and parsing it manually. Moving away from that, and deprecating the original wasn't very nice here.

> 
> > It's really easy to make it crash, while that worked
> > in now deprecated videoparse. I guess this new element is not well tested
> > and should have stayed longer in -bad ...
> 
> It's in -bad since 1.10 and seemed to have worked for everybody so far, and
> other than the old element it even has tests.
> 
> So best to add new tests for the things that are broken, and then fix them.

Ok. Clearly no one have used the strides and offsets, it's probably too hard to use atm anyway. Was the frame-stride name discussed ? Cause cause stride really means a line, making frame-stride pretty weird. It's in fact the frame-size or buffer-size imho.

Go ahead with proposing better names and API. We still have some time before 1.12.

I need to fix GStreamer core to finish this work.

Created attachment 348037 [details] [review]
rawvideoparse: Use GstValueArray for strides and offsets

This allow using those property through gst-launch-1.0. This type
gained a deserilizer recently. The syntax is: <val1, val2, ...>.
Note that we also use the type int instead of uint to avoid having
to cast when specifying the values. The deserilizers assume
int by default.

Created attachment 348038 [details] [review]
rawvideoparse: Rename frame-stride in to frame-size

The term stride is confusing here, since the stride is always use
to signal the pixel row size of an image (including padding). Also
a frame may have a single stride, which adds to the confusion. This
patch uses frame-size, which simply indicate the frame size in the
case the images have some padding in between.

Review of attachment 348037 [details] [review]:

Is this fixing anything or just changing the API?

I don't think having only a GstValueArray property is a good idea. This makes it nearly impossible to use from bindings. See all the discussions we had about audiomixmatrix, where we now ended up with two properties: one GValueArray for bindings, one GstValueArray for gst-launch. It all sucks really, but this seems to be the least bad approach to make usage easy.

(In reply to Sebastian Dröge (slomo) from comment #8)
> Review of attachment 348037 [details] [review] [review]:
> 
> Is this fixing anything or just changing the API?

It makes it usable from gst-launch, which really matter for a development tool like the raw parser.

> 
> I don't think having only a GstValueArray property is a good idea. This
> makes it nearly impossible to use from bindings. See all the discussions we
> had about audiomixmatrix, where we now ended up with two properties: one
> GValueArray for bindings, one GstValueArray for gst-launch. It all sucks
> really, but this seems to be the least bad approach to make usage easy.

Or could I just go back to using a string, like I did when I added strides and offset in the origin videoparse element ?

(p.s. the feature is still broken in some cases, it will set the stride, and then update it wrongly with a larger value, additionally, it fails to set the GstVideoMeta from time to time, it does not detect if downstream supports videometa. There a lot of work to fix this element, the unit test is hardcoded to a fixed resolution and Y444)

(In reply to Nicolas Dufresne (stormer) from comment #9)

> > I don't think having only a GstValueArray property is a good idea. This
> > makes it nearly impossible to use from bindings. See all the discussions we
> > had about audiomixmatrix, where we now ended up with two properties: one
> > GValueArray for bindings, one GstValueArray for gst-launch. It all sucks
> > really, but this seems to be the least bad approach to make usage easy.
> 
> Or could I just go back to using a string, like I did when I added strides
> and offset in the origin videoparse element ?

In a second property, sure. Or a GstValueArray second property.

That way you can sanely use it from C and bindings (without stringly typed programming), and from gst-launch.

(In reply to Nicolas Dufresne (stormer) from comment #9)

> the unit test is hardcoded to a fixed resolution and Y444)

What would you propose instead? What do we gain from testing multiple resolutions and Y444? IIRC, the unit test already accounts for caps changes mid-stream.

There is special case that crash with unpacked 24bit RGB and i420. That's because VideoInfo requires stride bigger then the real minimum. Other then that, it seems a little buggy, and need more time to figure-out. The fact that the order in which we set the properties as an impact is also problematic to me.

In bug 753754, I'm proposing some new APIs that would allow having a single property. As of today, we could already set the property:

  Gst.util_set_object_arg (e, "plane-strides", "<320,160,160>")

Or if you have a Python list:
  Gst.util_set_object_arg(e, "plane-strides", '<{}>'.format(','.join(map(str, a))))

But you could not set that. You also could not get/set from GstStructure, but that's a different subject. I also added the ability to transform GST_TYPE_ARRAY/LIST to/from G_TYPE_VALUE_ARRAY. I then added to helpers to get/set those property using GValueArray. I know GValueArray is deprecated, but it's the only matching type in GLib for our list/array (even though we generally have the same type for the entire list). So the other way to get/set those property would be:

  a = GObject.ValueArray()
  a.append(320)
  a.append(160)
  a.append(160)
  Gst.util_set_object_arrray (e, "plane-strides", a)
  ret, b = Gst.util_get_object_array (e, "plane-strides")
  b.get_nth(0)
  ...

If that sound acceptable to you, we can then stay with a single property, and probably remove the second property in audiomixmatrix before 1.12 release.

Looks ok but ugly to me. And is not easily discoverable and not how people usually use GObject properties.

In C it's the same story. From C you would still use set_property() get_property() with a GValue instead of the normal API, which is different from how you usually learn to use GObject properties.


It's all workarounds for various deficiencies in GObject, so what can we do :)

I which I had a better idea. Note that this does not prevent from adding Python specific overrides to make it work better. For properties, if there was a type in GLib that is GArray + GType, GTypedArray ? that would solve it right away, since we would not need to rely on offline introspection any-more.

Ok, we now have the workaround, and proper Python overrides. That means in python you simply do:

  element.plane_strides = Gst.ValueArray([640,320,320])

Or
  list = element.set_property("plane-strides", Gst.ValueArray([640,320,320])

Note that the API is slightly asymmetric, since read element.plane_strides will give you a python list directly. The Gst.ValueArray and Gst.ValueList are like C cast here. Let me know if you find any issues.

Attachment 348037 [details] pushed as 2b4a173 - rawvideoparse: Use GstValueArray for strides and offsets
Attachment 348038 [details] pushed as d81a6da - rawvideoparse: Rename frame-stride in to frame-size