search

gstreamer-java / gst1-java-core

Java bindings for GStreamer 1.x
GNU Lesser General Public License v3.0
194 stars 72 forks source link

Clock.getCalibration sets the calibration, doesn't retrieve it. #212

Closed Gamebuster19901 closed 3 years ago

Gamebuster19901 commented 3 years ago

First, if this is intentional the javadoc is wrong. None of the parameters can be null because they're primitives.

I believe the method is supposed to call GSTCLOCK_API.gst_clock_get_calibration instead of GSTCLOCK_API.gst_clock_set_calibration because this is the getter not the setter. This would allow null to be passed as the parameters would be arrays instead of primitives...

I believe this method is intended to modify arrays that a developer passes into the method so they can retrieve the calibration.

https://github.com/gstreamer-java/gst1-java-core/blob/37e7555851880a68e5cad993750fc3e0990b3485/src/org/freedesktop/gstreamer/Clock.java#L183-L196

neilcsmith-net commented 3 years ago

Er, yes, that's definitely not right! Inherited from https://github.com/gstreamer-java/gstreamer-java/blob/master/gstreamer-java/src/org/gstreamer/Clock.java#L195 (ClockTime to long change makes no difference here). Need to deprecate that and work out how we can actually best support the required functionality.

neilcsmith-net commented 3 years ago

OK, looks like best option is to just provide a Clock.Calibration getCalibration() method and ignore the ability to pass in nulls entirely? Good shout for conversion to record in future.

Gamebuster19901 commented 3 years ago

Honestly, I'd recommend adding the following methods

getInternalTime() getExternalTime() getRate()

setInternalTime() setExternalTime() //if this actually makes sense, I don't actually know the difference between external and internal time. setRate()

neilcsmith-net commented 3 years ago

If this was implemented in Java I'd probably agree. But we generally try to mirror upstream method names as much as possible. I'm also unsure of usage of this method. How often is someone likely to only need one of the values? Crossing the Java<>Native boundary only once is probably preferable. Looking at other bindings, C# has out parameters, so no use(!), and Rust has tuple return types, and also ignores ability to pass in null. So a small data class (in future a record) would be my preferred way to map this.