Binding for the GStreamer OpenGL API

[Florent Lioult]

Hi,

I’ve published a fork of gst1-java-core and a new project gst1-java-plugins-base. The first brings only a small set of changes to the core binding in order to be able to bind the GStreamer OpenGL API in the second project. Most of the changes are based on works made by Christophe Lafolet since I’m working with him on the same project aimed at directly using the output of the nvdec plugin in Java. We have a working integration now, but I’ve put in the gst1-java-plugins-base project a simpler and more portable example only based on Swing, JOGL and the glupload plugin.

In the prospect of pushing parts or all of these changes in gst1-java-core and provided the folk here are interested in it, I’d like to clarify the binding rules for gst1-java-core. Is it intended to include binding for other GStreamer projects (like gst-plugins-bad per instance) or only its core? If so, how an extension like my "gst1-java-plugins-base" should be handled?

Florent

[Neil C Smith]

On Mon, 28 Oct 2019 at 09:14, Florent Lioult <flo...@hihan.org> wrote:
> I’ve published a fork of gst1-java-core and a new project gst1-java-plugins-base. The first brings only a small set of changes to the core binding in order to be able to bind the GStreamer OpenGL API in the second project. Most of the changes are based on works made by Christophe Lafolet since I’m working with him on the same project aimed at directly using the output of the nvdec plugin in Java. We have a working integration now, but I’ve put in the gst1-java-plugins-base project a simpler and more portable example only based on Swing, JOGL and the glupload plugin.

Thanks, Florent. And welcome! This would be a valuable contribution
to the bindings. It's not the first attempt at binding OpenGL
support, but looks at a glance more thorough and up-to-date.

There are some obvious issues that I noticed just looking at the
changes in gst1-java-core. In particular things like re-adding
ClockTime in the mappings?! That cannot possibly function! And
relying on type mapping in new mappings should be avoided - eg. there
should probably be a GstContextPtr extending GstMiniObjectPtr used in
all mapping functions.

We would have to consider changes and reviewing in order to merge this
upstream. It should be done in line with plans developed for v1.0 to
replace lowlevel with auto-generated direct mapping. Although without
funding for that work it's currently slow, and not top of our agenda
at least.

We may need to look at tests for all the additional code too. And use
of @Gst.Since / Gst.checkVersion(..) where required.

> In the prospect of pushing parts or all of these changes in gst1-java-core and provided the folk here are interested in it, I’d like to clarify the binding rules for gst1-java-core. Is it intended to include binding for other GStreamer projects (like gst-plugins-bad per instance) or only its core? If so, how an extension like my "gst1-java-plugins-base" should be handled?

The idea of what -core means wasn't particularly well defined at the
start, but was more about Java dependencies than GStreamer ones. When
forking I wanted to remove all Swing and SWT dependencies, as well as
the examples and the various minimal function element wrappers. It
was not at first about limiting what GStreamer aspects were mapped.
In hindsight, webrtc should perhaps have been in a separate library.
But I'm open to this being either included in gst1-java-core or added
as gst1-java-gl - what are your thoughts? Anyone else?

What are your plans for supporting and maintaining this code should it
become part of the main library or be provided separately?

Obviously, the examples should be separate either way, and ideally not
include the JOGL binaries. In fact, would be good to get an LWJGL
based example in there as well - I still have some concerns about the
future viability of JOGL.

Best wishes,

Neil

--
Neil C Smith
Codelerity Ltd.
www.codelerity.com

Codelerity Ltd. is a company registered in England and Wales
Registered company number : 12063669
Registered office address : Office 4 219 Kensington High Street,
Kensington, London, England, W8 6BD

[Florent Lioult]

As long as it is unstable, keeping the GL+Video binding for gst-plugins-base in a separate project will be easier. Once stabilized however, I don't know which approach is the best. I've have a quick look at the binding for other languages, but it doesn't seem like there is a clear cut rule here. In addition, switching to a generative approach for the low-level binding could changes things a bit in this regard.

That being said, from a pure practical point of view, I don't see the need of separating the two bindings. Contrary to the good/bad/ugly plugins, the plugins-base is always around and makes for an essential part of GStreamer. In the same vein, having another project to store various examples doesn't seem necessary as long as the additional dependencies are kept in the test scope.

If we focus for now on the core changes I've introduced, could you give some precision on what is missing or not properly done? Beside the minor lost ClockTime problem (a merge problem, I don't even need it), I'm not sure to understand what you mean by "relying on type mapping in new mappings should be avoided". Do you mean to favor direct native calls over access to read structure? If so, how it relates to the "GstContextPtr extending GstMiniObjectPtr" usage? To ease the transition, I've trimmed down my changes by only keeping Context and NeedContextMessage and removing the Memory (saved the low-level API), Allocator and Query additions. The change set is now truly necessary and sufficient.

Lastly, regarding my long term plans of supporting the new code, I can't give you any guarantees ;) Surely the changes made in the two projects are dearly needed and will be used for a significant duration of time. So, yes, support will be provided for this GL binding in on way or another, but not necessarily by me.

[Neil C Smith]

Hi,

On Mon, 28 Oct 2019 at 18:22, Florent Lioult <flo...@hihan.org> wrote:
> As long as it is unstable, keeping the GL+Video binding for gst-plugins-base in a separate project will be easier. Once stabilized however, I don't know which approach is the best. I've have a quick look at the binding for other languages, but it doesn't seem like there is a clear cut rule here. In addition, switching to a generative approach for the low-level binding could changes things a bit in this regard.

If anything, we will most likely follow what Rust does. Generating
low-level will be a major change, but the lowlevel package is non-API
- the plan is to develop it in a separate library, similarly to Rust
FFI bindings. When we get more free time or funding for it anyway!
:-\

> That being said, from a pure practical point of view, I don't see the need of separating the two bindings. Contrary to the good/bad/ugly plugins, the plugins-base is always around and makes for an essential part of GStreamer. In the same vein, having another project to store various examples doesn't seem necessary as long as the additional dependencies are kept in the test scope.

As I said, the bigger concern to date is Java dependencies not
GStreamer ones when considering whether to separate or not. So,
merging into gst1-java-core makes some sense. OTOH, the Rust bindings
are more separated than we are right now. Pros and cons to both
approaches.

> If we focus for now on the core changes I've introduced, could you give some precision on what is missing or not properly done? Beside the minor lost ClockTime problem (a merge problem, I don't even need it), I'm not sure to understand what you mean by "relying on type mapping in new mappings should be avoided". Do you mean to favor direct native calls over access to read structure? If so, how it relates to the "GstContextPtr extending GstMiniObjectPtr" usage? To ease the transition, I've trimmed down my changes by only keeping Context and NeedContextMessage and removing the Memory (saved the low-level API), Allocator and Query additions. The change set is now truly necessary and sufficient.

eg. try not to use Context in GstContextAPI, but a GstContextPtr
instead. We're trying to remove references in lowlevel to classes
elsewhere.

> Lastly, regarding my long term plans of supporting the new code, I can't give you any guarantees ;) Surely the changes made in the two projects are dearly needed and will be used for a significant duration of time. So, yes, support will be provided for this GL binding in on way or another, but not necessarily by me.

That's not that reassuring! ;-) Given that the person who ends up
having to support these things right now is usually me. OpenGL
support is on my agenda, so I'm quite happy to have a contribution
that adds it in to the core library, but a big code drop that doesn't
meet future project requirements around mapping, tests, versioning,
etc. is only partially helpful.

See also the section on contributions in the readme.

"Contributions to the library are welcomed, either to fix / enhance
current features, or bring in new ones. There is also ongoing work to
rework the lowlevel bindings.

Before opening a Pull Request please raise an issue or discuss your
contribution on the mailing list. New features must have tests,
selectively applied if targeting features in versions of GStreamer
above 1.8. All Pull Requests will be automatically tested via CI, and
all tests must pass before merging will be considered.

If you are making a large contribution to benefit a commercial
project, sponsorship of integration and support time would be
appreciated."

Thanks and best wishes,

[Florent Lioult]

I do understand your concerns about the future maintenance of any addition to the binding. That’s one of the reasons why I’ve isolated the extension part for GL support (which could remains as such if you are not seeing it fit for inclusion) from the minimal changes required in the core.

Granted, my first proposal for the latter was incorporating unnecessary changes, but I think the latest is sensibly more compact and focused. Moreover, there is no problem for improving it to better comply with the project rules... as long as I am aware of them. Per instance, the removal of circular dependencies between low-level and the visible parts is a new one for me, but I’m probably missing some resources here. So, do not hesitate to clarify things up as you did and I will take then into account.

Finally, I must clarify that, if I’m not in a position of assuming a true maintainer role, this GL binding is not a fire & forget PoC, but would be used in production and taken care as such, probably more by my colleague Christophe in the long run though. That being said, I reckon that fixing bugs for a specific version and a specific usage is not quite the same as maintaining and evolving a library.

[Neil C Smith]

On Mon, 28 Oct 2019 at 22:30, Florent Lioult <flo...@hihan.org> wrote:
> Granted, my first proposal for the latter was incorporating unnecessary changes, but I think the latest is sensibly more compact and focused. Moreover, there is no problem for improving it to better comply with the project rules... as long as I am aware of them. Per instance, the removal of circular dependencies between low-level and the visible parts is a new one for me, but I’m probably missing some resources here. So, do not hesitate to clarify things up as you did and I will take then into account.

It's not particularly well documented yet! Hence why it's useful to
have a conversation in advance of contributions. I'll try and find
some time to update docs. But some key things.

* Take a look at how the Controller API addition was done -
https://github.com/gstreamer-java/gst1-java-core/pull/158
* Note use of Handle and GPointer subclasses. You may need to make
MiniObject.Handle protected. Initializer constructor should be
package private and create the Handle.
* Note use of @Gst.Since(minor =12) on ProxyControlBinding. This
should be done for any type or function requiring a GStreamer version
higher than 8 (our baseline). There is also a bug there - first line
of create() should be Gst.checkVersion(1,12); see Buffer for reference
here.
* Docs must be copied across from upstream, and a link to the upstream
docs included. Lowlevel API should have link to GStreamer source.

Without a PR I haven't looked at everything in your contribution, but
in general make sure newly added methods match the upstream function
names (but camel case) where possible. Why does set() exist rather
than returning a Structure?

> Finally, I must clarify that, if I’m not in a position of assuming a true maintainer role, this GL binding is not a fire & forget PoC, but would be used in production and taken care as such, probably more by my colleague Christophe in the long run though. That being said, I reckon that fixing bugs for a specific version and a specific usage is not quite the same as maintaining and evolving a library.

No, it isn't. And while I'm very keen to see additional maintainers
this has not happened to date. I'm happy to work with people with
code contributions, but if that contribution is large and needs
reviewing and fixing up prior to merging, then there's a limit to how
much of my free time I am willing to give to commercial users who need
that.

Best wishes,

[Florent Lioult]

I've updated my fork based on your remarks:

https://github.com/gstreamer-java/gst1-java-core/compare/master...Chatanga:master

I didn't used the Gst.Since annotation because both the Context and the Need-Context message are old stuff originating from the version 1.2.

Regarding the 'set' method in Context, I've added it to circumvent the package visibility of 'void setValue(String, GType, Object)' in Structure (the GL extension begin in a 'gl' sub-package). I've assumed that this method using a generic 'Object' was hidden for good reasons.

[Neil C Smith]

Hi,

On Tue, 29 Oct 2019 at 15:01, Florent Lioult <flo...@hihan.org> wrote:
> https://github.com/gstreamer-java/gst1-java-core/compare/master...Chatanga:master

Thanks! Mostly looks good. A few comments, and then perhaps create a
specific branch for this and open a pull request.

* Your test that asserts the context is different is potentially wrong
- Element::getContext should probably be using
Native.callerOwnsReturn() They should be the same Java object. And
you're probably leaking a ref otherwise. Needs testing though -
MiniObject memory handling is a bit awkward.
* The Context constructors with String should be creating a Handle
directly. Think of Natives.initializer as semi-deprecated.
* The upstream doc link should be second paragraph, like all the others.
* Remove the sentence about refcount on the setContext docs.
* Please revert the README changes for making a PR.
* Please ensure consistent formatting - 4 spaces.
* structure method - see below ...

> I didn't used the Gst.Since annotation because both the Context and the Need-Context message are old stuff originating from the version 1.2.

Yes, that was more for the other OpenGL work.

The use of Gst.checkVersion incidentally is to ensure that people
request the version of GStreamer they need at init() time.

> Regarding the 'set' method in Context, I've added it to circumvent the package visibility of 'void setValue(String, GType, Object)' in Structure (the GL extension begin in a 'gl' sub-package). I've assumed that this method using a generic 'Object' was hidden for good reasons.

Ah, the "good reason" was hiding lowlevel types from the public API.
There should be no references to lowlevel types (with a few exceptions
around GPointer subclasses) exposed in public / protected signatures.
We really need a higher level representation of GType.

So, what I suggest is moving your existing method into Structure
instead as an overload of the existing setValue