RFC: libVLC: use microseconds (us) for libvlc_time_t ?
Only the last commit should be reviewed.
-
libvlc_clock()
returned int64_t and used us. - New player watch timer API used us.
- Lot of set/get time use
libvlc_time_t
as ms. - timeout are using int as ms (not changed)
**Questions: **
- OK with the change ?
- Should we keep timeout as int / ms ? (Or use
libvlc_time_t
/us)
If accepted, will be merged on top of !3503 (In order to have all major libVLC changes in one shot)
Merge request reports
Activity
changed milestone to %4.0
added Component::Bindings: LibVLC (native API) label
The point is not to expose multiple timescale, but to hide the detials of our timescale and signal the user it should use the conversion functions. It would also allow specifying different timescale from the user side so that we convert them properly, eg for set_time. It's similar to other API in C exposing clock time (CMTime for instance, or like ffmpeg)
An integer is more convenient than a structure for arithmetics, but then the user is also more keen to use it wrongly, and it's much less explicit. I feel this is akin to the change we've made changing mdate to vlc_tick_t.
I'm not 100% attached to this idea so this is not blocking though, I was answering for the "RFC" part.
I don't have strong opinion here so I'll defer to the majority, but since it's a RFC... I personally find time structures to be a PITA to handle. It forces a lot of null check (to prevent divisions by 0) and overflow checks inside the library. Then we have to decide what to do in case of invalid/overflow values function by function. Handling time is very error prone (it get worse with timezones, summer time and split seconds handling) so I'd rather keep it out of libvlc and use the integer with helper functions (that could return the possible overflow state).
It was just an example of why I don't like handling time in libs and this is why it comes inside parenthesis. Again I'm just expressing my personal taste.
Edited by Denis CharmetI like structures insofar as it matches how Rust and C++ work, and allows for better handling of arithmetic. The problem is that C does not allow operating overloading, so I can understand where @typx is coming from though.
With that said, the elephant in the room is the UB with arithmetic overflow in time handling. That's more with
vlc_tick_t
thenlibvlc_time_t
though.I understand the arithmetic point but one of the point is also preventing the user from doing time arithmetic on the values returned by libvlc, if the clock frequency cannot be guaranteed/is an internal detail, so that the user creates its own time infrastructure to manipulate this. For instance, converting the libvlc_time to millisecond and then convert back.
I think @typx point was more about handling invalid cases purposedly setup by the libvlc user, namely
libvlc_media_player_set_time(player, (libvlc_time_t){ .timescale = 0 });
. I'm fine with considering this always maps to zero or to invalid depending on how we want to handle that. To be fair, there's also high chance there are existing path incorrectly mapping the user input to VLC_TICK_INVALID or VLC_TICK_MIN/MAX error path instead of other paths, so I feel like this should be handled the same.Since we then map that to tick internally, we don't risk meeting unwanted arithmetics/divise by zero error if we only use specific function to map from/to.
My point was indeed that structures introduce new error path (multiplication overflows/divisions by zero/divisions by big numbers returning unwanted VLC_TICK_INVALID) which needs to be handled inside every function that will convert the
libvlc_time_t
tovlc_tick_t
But let's be clear, my opinion was by no mean a blocking one and I'm fine with structures if you all think it's better, I just want to emphasize that the internal conversions functions would have to use the overflow functions and document what happens when time conversion error happens. (I'm fine with returning -EINVAL it's just that I thought that returning it at a conversion function level allows to know what parameter is invalid, namely the time).
Edited by Denis Charmet
120 120 121 121 static inline libvlc_time_t libvlc_time_from_vlc_tick(vlc_tick_t time) 122 122 { 123 return MS_FROM_VLC_TICK(time + VLC_TICK_FROM_US(500)); 123 return time; 120 120 121 121 static inline libvlc_time_t libvlc_time_from_vlc_tick(vlc_tick_t time) 122 122 { 123 return MS_FROM_VLC_TICK(time + VLC_TICK_FROM_US(500)); 123 return time; 124 124 } 125 125 126 126 static inline vlc_tick_t vlc_tick_from_libvlc_time(libvlc_time_t time) 127 127 { 128 return VLC_TICK_FROM_MS(time); 128 return time; 66 66 67 67 typedef struct libvlc_title_description_t 68 68 { 69 int64_t i_duration; /**< duration in milliseconds */ 69 libvlc_time_t i_duration; /**< duration in microseconds (us) */ Sounds good to me
Edited by Steve Lhomme
added MRStatus::InReview label
mentioned in merge request !6114 (closed)
added MRStatus::NotCompliant label and removed MRStatus::InReview label