Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GStreamer #127

Merged
merged 48 commits into from
Dec 28, 2020
Merged

GStreamer #127

merged 48 commits into from
Dec 28, 2020

Conversation

mjakeman
Copy link
Member

@mjakeman mjakeman commented Dec 16, 2020

This PR implements the groundwork for GStreamer, as well as substantial amount of GStreamer itself (enough for a music player, with caveats). It implements parts of GStreamer, GstBase, GstAudio, GstVideo, and GstPbutils.

This PR depends on:

Remaining Tasks
Here's the laundry list of TODOs before we can merge this:

  • Rebase on develop: Needs changes to work with Scriban 3.0 and our reworked templates.
  • GIR Files: Regenerate relevant GStreamer gir files and add them to gir-files.
  • Object Struct Fields: Decide on safe vs unsafe access to GObject fields
    • See GstObject->Flags for an example of safe vs unsafe access
  • GhostPad Constructor Problems: GhostPad's constructor implements custom behaviour which we can't easily implement in a C# constructor (validation of construct time properties).
  • Need design for GstIterator: How should this work with C# Enumerable/Enumerator? Tentative design in WIP: GstIterator #157.
  • Need design for GstTagList: GstTagList needs a rethink now that implicit Values have been removed. (Edit: TagList's API has been simplified to use GValue directly for now. This seems to work fine).
  • Decide on Public API: Mainly regarding access e.g. Error, WrapPointerAs, GetHandle, etc.
    • We also need to decide on GStreamerGlueGetProperty/GStreamerGlueSetProperty.
    • Rename WrapPointerAs and GetHandle to WrapHandle/GetHandle
  • Fix Delegate Signatures (Closures Handling #119): We need a way to marshal between managed signatures and unmanaged signatures
    • As part of this, remove Timeout->FuncData as it does not work Dropped from this PR

Future Changes
GstMessage and many other structs depends on support for custom Value types in the generator. This will allow us to use ref struct rather than the complicated and unsustainable (maintenance-wise) marshalling I'm using at the moment. This can reduce method bodies from >20 lines to sub-5, as we won't need to explicitly marshal anymore. It's probably much safer this way as well, as there is little chance of memory corruption from forgetting to free or update something.

Fixes #112

@gircore/coreteam

firox263 and others added 27 commits November 1, 2020 17:32
Adds a small subset of functionality for the
aforementioned classes/structs. This includes a
naive implementation of GSourceFunc for Timeout.
Most of the changes here will be properly
implemented by #102. This commit ensures the
forthcoming GStreamer additions are designed
with .NET 5 in mind.
Patch to register aliases recursively. We
will get this for free with the new generator
architecture, so this is more of a temporary
workaround.
 * Handle in/out/ref conditions (naive attempt,
   needs work). Related fixes.
 * Start generating field structs and provide a
   mechanism for access.
 * Fix loop iteration limit for Constants
 * Groundwork for GStreamer
This commit adds more classes to GStreamer, as well
as starts generating the following plugins:
 * Gst.Audio
 * Gst.Base
 * Gst.Pbutils
 * Gst.Video

There is a lot of temporary glue present in GObject
to facilitate this. Before merging, we will need to
decide on a more permanent API for both GObject and
GStreamer.

Additionally, due to issues with the generator,
there is a lot of boilerplate here that should either
be auto generated, or avoided entirely (e.g. using
ref structs).
This causes memory corruption errors. We will need
to rethink marshalling altogether with the new
generator.
This fixes some longstanding issues with the
generator and cleans up some recent additions
from previous commits. It also includes several
marshalling related fixes.

 * It particularly focuses on making the type
   resolution code more organised after some
   GStreamer-related additions.

 * It has several changes with regards to
   in/out/inout gir annotations and the
   corresponding use of C# in/out/ref in
   generated code.

 * It fixes several issues with memory (e.g.
   AccessViolation exceptions), where
   marshalling wasn't being done correctly
   (or in the wrong way, hence the generator
   changes).

With this commit, GStreamer coverage is
sufficient for writing basic to moderate
complexity media playback code.
Some gir files appear to use any/none/utf8 as
types as opposed to the C Type directly. This commit
adds them to the generator purely for corner cases.
Maybe we can remove this at a later date?
Comments are licensed under LGPL, but gir.core is
MIT. We must distribute documentation separately.

See: https://gtk-rs.org/docs-src/faq (at bottom)
@mjakeman mjakeman added Enhancement New feature or request Discussion Plan the future labels Dec 16, 2020
@mjakeman mjakeman self-assigned this Dec 16, 2020
@badcel
Copy link
Member

badcel commented Dec 24, 2020

I fully agree on all points. I have one suggestion regarding the SetProperty methods.

We already have a private void SetGProperty(Value value, string? name) method and a corresponding getter method. I would suggest that we rename those methods to SetProperty / GetProperty to create the desired overloads. Additionally we should make those methods protected so they are accessable from Element. This looks like the most simple and clean approach to me. I think we would avoid some kind of descriptor for now. We can add it later if we see the need.

On a side note, I don't know why the name parameter is last in the parameter list? This looks kind of strange to me. I would suggest switching the order of the parameters: protected void SetProperty(string name, Value value).

From my point of view just go ahead all your suggested changes / planings are great. So let's make smaller iterations and get this into develop 👍


Less important remark regarding Element.cs, there is this code:

 // We intentionally throw an exception if the type of value cannot be wrapped
 // TODO: Support boxing arbitrary managed types
Value val;
if (value?.GetType().IsAssignableTo(typeof(GObject.Object)) ?? false)
    val = Value.From((Object) value!);
else
    val = Value.From(value);
                    
GStreamerGlueSetProperty(property, val);

I think we should in some later point in time verify if this code could somehow be part of the GObject library so the protected SetProperty API allows to set arbitrary objects and is doing the right thing. The signature would be s.th. like protected void SetProperty(string name, object value). I'm not sure if we can reach that goal, but perhaps you just add a TODO flag and we reconsider later?

This was referenced Dec 26, 2020
Removes the initial implementation of GstIterator.
Splits off an IEnumerable-based implementation into
a separate branch.
As discussed, this renames Try/WrapPointerAs
to Try/WrapHandle to better complement GetHandle.

This commit also includes
 * refactoring libraries to use the new method names
 * replacing occurences of the old method names in
   comments
 * adding xml documentation comments
 * refactoring TryWrapHandle to not throw exceptions
   and instead fail gracefully.
 * nullability changes to TryWrapHandle to better
   represent cases where wrapping may fail.
 * refactoring libraries to use new nullability
   rules.
As discussed, cleans up public API by making
GetProperty(string) and SetProperty(string, value)
public, and allowing arbitrary properties to be set
when given the string name.

This allows for the GStreamerGlue functions to be
removed. Also includes minor changes to xml
documentation comments.
@mjakeman mjakeman mentioned this pull request Dec 27, 2020
@mjakeman mjakeman marked this pull request as ready for review December 27, 2020 12:04
@mjakeman mjakeman requested a review from a team December 27, 2020 12:06
Copy link
Member

@badcel badcel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, this is amazing 👍 It looks like it involved a lot of research work, too.

I did not test if it works as there are no samples. But as the code itself looks good we can fix any remaining issues as they arise.

Nevertheless I left several comments which we should discuss but there is no show stopper.

I think we should add some simple samples but this is for another PR.

Libs/GLib/Classes/Filename.cs Outdated Show resolved Hide resolved
Libs/GLib/Classes/MainContext.cs Outdated Show resolved Hide resolved
Libs/GLib/Classes/MainLoop.cs Outdated Show resolved Hide resolved
Libs/GLib/Classes/Variant.cs Outdated Show resolved Hide resolved
Libs/GObject/Classes/Object.Properties.cs Outdated Show resolved Hide resolved
Libs/Gtk3/Classes/Window.cs Outdated Show resolved Hide resolved
Libs/Gtk3/Classes/Window.cs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Generator/Templates/GObject/class.sbntxt Outdated Show resolved Hide resolved
Generator/Templates/GObject/class.sbntxt Outdated Show resolved Hide resolved
 * Make Get/SetProperty protected
 * Change TryWrapHandle to use NotNullWhen
   instead of MaybeNullWhen.
 * Remove TargetFramework from project files
 * Add a PackageId for GStreamer Libs
 * Remove LangVersion and AllowUnsafeBlocks from
   GStreamer csproj
 * Fix whitespace problems
 * Fix TODO flags
 * Use HandleError more consistently
 * Implement IHandle for GLib.MainLoop
 * Remove TODO from class.sbntxt template
 * Attempt to fix indentation for Object Fields
 * Cleanup unnecessary comments
 * Enforce code style (properties at top of file)
 * Nullability annotation changes
 * Rename HandleError -> ThrowOnError to better
   indicate behaviour.
 * Be consistent with nullability and error
   handling for Gst.Parse.
 * Remove unnecessary 'Throws if error' comments.
@mjakeman mjakeman requested a review from a team December 28, 2020 07:35
@mjakeman
Copy link
Member Author

That should be everything, although I might have missed something here and there.

I'm working on a few samples at the moment, I'll open a new PR for that once we land this one.

@badcel

@badcel
Copy link
Member

badcel commented Dec 28, 2020

Great work this is quite big 👍

@badcel badcel merged commit 460e33d into gircore:develop Dec 28, 2020
@mjakeman mjakeman changed the title WIP: GStreamer GStreamer Dec 29, 2020
@Inrego
Copy link

Inrego commented Mar 16, 2023

I just now found this project, and it does indeed seem very interesting. However, still only the few first plugins seem to be implemented. Are there any plans to support more plugins, and if yes, what's the timeline? From what I gather, you've made a rather impressive code generator, so it sounds like it could be as simple as grabbing the remaining gir-files and generate source based on them?
The source of gir files you've previously linked seems dead, but here's a newer source I've found that seems rather up-to-date:
https://gitlab.freedesktop.org/gstreamer/gir-files-rs

@badcel
Copy link
Member

badcel commented Mar 16, 2023

Thank you. Currently GStreamer is not a priority for me as there are other things missing.

For GStreamer itself it is possible that there is some manual written code missing. As I don't have any experience with GStreamer this is all a bit blurry for me. If you know your way around GStreamer I'm grateful for any contribution and willing to support new contributors to get the needed code landed.

Adding new GStreamer bindings requires adding them to our gir files repository first. To do this the corresponding files must be added to the configuration and perhaps the CI script a little bit adopted. If this is done the gir files get updated automatically.

The next step would be to use the new gir files in the generator.

If you are interested in certain plugins feel free to add them to the gir files repository like described above.

If you want to see improved GStreamer support adding more sophisticated samples would help to unveil missing bits and pieces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Plan the future Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support GStreamer (Gst Plugins)
4 participants