Skip to content

Conversation

@neilcsmith-net
Copy link
Member

This supports setting and getting GstObject properties in a serialized String format. It makes use of gst_value_serialize and gst_value_deserialize. This is in GstObject rather than GObject because it relies on GStreamer-specific functionality (with possible thought of splitting off GLib support in future).

This code is intended as an alternative to #187 and includes an adapted version of the test by @MaZderMind in there. It fixes #182 as well as hopefully other uses mentioned in #166 and elsewhere.

Returned String representation of enums is surprising, but also usable in setAsString.

@neilcsmith-net
Copy link
Member Author

Had concerns this didn't address #166 but looks like an upstream bug that doesn't allow mix-matrix to be set again. https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/481

Copy link
Contributor

@MaZderMind MaZderMind left a comment

Choose a reason for hiding this comment

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

Approve, although I'd like to see the free logic in finally blocks and would like to change/add to the tests.

Add test for GstValueArray from String
Fix invalid enum test to use setAsString
Add invalid enum as int test
@neilcsmith-net neilcsmith-net merged commit e24eb2a into gstreamer-java:master Apr 10, 2020
assertTrue(matrix.contains("0.4"));
assertTrue(matrix.contains("0.6"));
assertTrue(matrix.contains("0.8"));
convert.dispose();
Copy link
Contributor

@MaZderMind MaZderMind Apr 13, 2020

Choose a reason for hiding this comment

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

The other Elements are creared in @before and not disposed explicitly. We should adjust them to use a similar pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm alright with the mix at the moment, as this element is only needed within that one test. Explicit disposal is generally preferred - in many respects I wish the bindings didn't support implicit collection, but we are where we are! Will keep at the moment, but relook later - tests need some love in general.


@Test
public void setValueArrayFromString() {
if (!Gst.testVersion(1, 14)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Event with GStreamer 1.16 installed, this will never be true, becuase the actual version is limited to the requested version on Gst.init which in this Test is not specified and therefore defaults to Version.BASELINE which is only 1.8.0; so this Test will never be true, even when a more modern GStreamer is installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn! Yes, thanks, forgot to change the init line - in general tests should be using the pattern from https://github.com/gstreamer-java/gst1-java-core/blob/master/test/org/freedesktop/gstreamer/PromiseTest.java#L37

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the String-Array-Initialization in the linked Init-Call is unnecessary; Gst.init(Gst.getVersion(), "PromiseTest"); resolves to the same Constructor.

These are the Things that IntelliJ suggests. You might want to use a better IDE ;) :P (just kidding.)

Copy link
Member Author

Choose a reason for hiding this comment

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

😆 Yes, I didn't write that one - sure the author was probably using Eclipse! 😉

Key thing is use of Gst.getVersion() in future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened #191 fixing the things discussed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set Enum-Properties on Elements by their literal Name

2 participants