Skip to content

Add GLFilter infrastructure for use in plugins

Marijn Suijten requested to merge MarijnS95/gstreamer-rs:glfilter into master

Hi!

Awesome to see Rust bindings in this shape. Unfortunately GLFilter was missing and it took some effort to get to gripes with the codebase and implement the missing pieces. There are quite a few open issues and questions, see below. The implementation was tested with a very simple plugin and swizzle shader.

v1_16 version requirement on GLBaseFilter

GLBaseFilter only has two methods supported on v1_16 and v1_18 respectively. Gir simply takes the minimum to derive a requirement on the class regardless of a version component being available on the <class> element. This breaks compilation as GLFilter has no such requirement but depends on GLBaseFilter. It seems like these (helper) functions were added later but the base class should be available. It has been overridden in Gir_GstGL.toml with the value of min_version, is that the desired method?

Manually creating GLFilterImpl subclass

For the most part it seems like the subclass boilerplate should be autogenerated, but it looks to be done by hand, correct? I have yet to implement all virtual methods if that's the case.

It seems there is room for unintended behaviour, in particular <gst::ElementClass as IsSubclassable<T>>::override_vfuncs(self); if a subclass between ElementClass and T (BaseTransformClass and GLBaseFilterClass for GLFilter) suddenly implement IsSubclassable; it's easy to forget to update fully qualified cast.

filter and filter_texture need special behaviour (which seems why this has to be manually implemented). Only one of the two functions should be implemented and overridden, the other should remain NULL (in particular filter, as that prevents filter_texture from being called) https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/blob/1.16/gst-libs/gst/gl/gstglfilter.c#L997-1002. I anticipated to implement two traits but two impl<T: ImplForFilterOrFilterTexture> IsSubclassable<T> for GLFilterClass blocks (with ImplForFilterOrFilterTexture being one or the other) results in a conflicting implementation of trait. Any suggestion or example how this can best be implemented?

GLMemory

This <record> seems to be sparse enough for Gir to reject it with Missing memory management functions for GstGL.GLMemory, there's not even a get_type. This has been implemented manually but I doubt the pointers are correct. Seems I can implement this by either:

  • Storing the sys representation by value, copying in and out;
  • Storing a ptr, which requires to copy over or otherwise take ownership in from_glib_full.

Is there a way to properly generate an automatic implementation for this record? There are quite a few gst_gl_memory_ functions that I don't feel like implementing by hand (perhaps the struct can be written by hand while the functions are automatically generated?).

Generating associated functions for <function>

add_rgba_pad_templates (taking *mut gst_gl_sys::GstGLFilterClass as single argument) fails to generate. If the ignored comment is to be believed gir simply doesn't seem to be aware this type is available. It'd make little sense to generate a struct for its record in GstGL-1.0.gir and stuffing the name in manual = [] results in similar invalid code. Is easy to implement in gir? For now I've added the implementation outside of auto/ as I've yet to dive into gir and see whether it's something I can fix.

Looking forward to your suggestions and reviews!

TODO Based on the questions above and followup discussions below

  • Understand how to properly implement GLMemory:
    • Data representation;
    • Helper functions;
    • Proper subclassing (ie. extending GLBaseMemory);
  • Implement an fn configure akin to BaseTransform to override filter and filter_texture distinctly;
  • Validate mutability on (autogenerated) function arguments (eg. GLMemory);
  • [GIR] Fix mutability (not adhering to ref_mode) in autogenerated callbacks;
  • Expose public (readwrite) fields in a meaningful way (see miniobject):
    • Check mutability rules on &self where Self = GLFilter;
  • The copy functions should probably be unsafe. If you pass in an invalid texture id, for example, this will at best just crash;
  • The init_once function should be internal and simply called at the top of each function that needs it, maybe wrapped in a std::sync::Once.
    • GLMemory calls this now, GLBaseMemory should follow suit!

Followup, not blocking for this MR

  • [GIR] Make a WIP PR for generated methods/functions on enums in gir;
    • Current autogen progress seen here;
  • Understand why certain functions on enums are free rather than associated;
    • Actually move these functions, initial work done here;
  • Docs fixes should be applied to https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/ . They're just documentation comments inside the .c files :)

Fixes #220 (closed)

Edited by Marijn Suijten

Merge request reports