gstreamer-java/gst1-java-core

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

Gamebuster19901 opened this issue · 4 comments

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.

/**
* Gets the internal rate and reference time of clock. See {@link #setCalibration} for more information.
* <p>
* internal, external, rate_num, and rate_denom can be left NULL if the caller is not interested in the values.
*
* Thread safe.
* @param internal a reference internal time
* @param external a reference external time
* @param rateNumerator the numerator of the rate of the clock relative to its internal time
* @param rateDenominator the denominator of the rate of the clock
*/
public void getCalibration(long internal, long external, long rateNumerator, long rateDenominator) {
GSTCLOCK_API.gst_clock_set_calibration(this, internal, external, rateNumerator, rateDenominator);
}

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.

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.

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()

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.