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

Structure: Add Accessor for GstValueList Fields #162

Merged
merged 4 commits into from
Jun 10, 2019

Conversation

MaZderMind
Copy link
Contributor

Add an Accessor for Structure-Fields of Type GstValueList, which is used in the spectrum-Message of the spectrum-Element and in unfixed-fields of Caps .

Fixes #161

@neilcsmith-net
Copy link
Member

Thanks for the PR. However, as much as I have concerns about the existing API in Structure, this is probably not the right way to do this. See getValues(), getDoubles() etc. The Structure::getDoubles and Structure::getIntegers methods should probably be extended to use any Number type rather than specifically check for the GType. The logic for extracting into lists and arrays should probably go into GValueAPI at some point, but that will need a bit more thought - can stay in here for now.

@MaZderMind
Copy link
Contributor Author

@neilcsmith-net I'd like to give this a try but I'm not sure how to adequately solve the requirements you gave. Basicly insead of copying Values over to an ArrayList I should better extend Gst.ValueList similar to GValueArray based on JNA Structure -- correct?

@neilcsmith-net
Copy link
Member

The quick initial fix would be to move your code somehow into https://github.com/gstreamer-java/gst1-java-core/blob/master/src/org/freedesktop/gstreamer/Structure.java#L446 Maybe directly check the gtype and use lowlevel functions for both the array and list variants rather than getValue(). Keep the class check and cast though.

@MaZderMind
Copy link
Contributor Author

ftr: I added two Tests that pass with my original version, so that modifications can be tested against.

@MaZderMind
Copy link
Contributor Author

@neilcsmith-net I moved the Accessing-Code into getValues but is is more or less a complete separate execution branch.

I think this is okay, because GstValueLists seem to be an Alien in the GObject Type-System, in that it is a GStreamer specific extension (similar to Fractions or FOUR-CCs) but as the Maintainer I will follow your advise on Code-Structure and -Location.

@neilcsmith-net
Copy link
Member

Thanks! Yes, it's not the ideal place for it longer term, but it keeps the existing API promise for now. All this conversion from GValue / GTypes to Java types needs revising, but that's a much bigger job!

@neilcsmith-net
Copy link
Member

I've just checked out the code and had a closer look. Ideally we'd have similar code for the list and array versions, but we're missing a few things for that so I'll look at a bigger change after v1.1.

Could you just update the Javadoc - eg.

This method only currently supports lists of values inside a GValueArray - other native list types will be supported in future.

to something like

This method currently supports lists of values inside a GValueArray or GstValueList.

Then I'll squash and merge. Many thanks!

@MaZderMind
Copy link
Contributor Author

@neilcsmith-net yes, I didn't like the non-uniformity between Lists and Arrays either, but the GValueArray-Class contains enough lowlevel Code that I don't fully understand, so that I didn't trust myself to accurately port that do GstValueArray.

Corrected Javadoc, feel free to Squash as you like.

@neilcsmith-net neilcsmith-net merged commit f77c505 into gstreamer-java:master Jun 10, 2019
@neilcsmith-net
Copy link
Member

Done, thanks!

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.

Unable to Access GstValueList (List of type Float) in GstStructure
2 participants