Skip to content
Snippets Groups Projects

RFC: libVLC: use microseconds (us) for libvlc_time_t ?

Open Thomas Guillem requested to merge tguillem/vlc:libvlc-new-apis-us into master
6 unresolved threads

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

Members who can merge are allowed to add commits.

Merge request pipeline #518624 passed

Merge request pipeline passed for 87677937

Test coverage 18.61% (0.15%) from 1 job
Ready to merge by members who can write to the target branch.
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Thomas Guillem changed milestone to %4.0

    changed milestone to %4.0

    • I'd use a structure instead of typedef, so as to include the timescale information in it, and provide conversion functions instead, since the time is becoming "public implementation". That's a bit complementary to your changes though, but would justify them.

    • Author Maintainer

      We plan to only use one unique timescale, so I don't see the necessity to add more information each time we pass a timing. Furthermore, passing an integer is more convenient than passing a structure. Providing functions / helper/ macro that convert to seconds, ms is a good idea.

    • 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.

    • Author Maintainer

      I see. For internal code (CORE), it might be more convenient to use an integer (as it is) since there is a lot of arithmetic, but it might be a good idea for a public API like libVLC yes.

      Should we pass this future struct by copy or const pointer for API that need a time ?

    • copy is fine with me.

    • 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).

    • How would timezones or such have any effect on this? Isn't this about monotonic times and durations, not wallclock?

    • 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 Charmet
    • I 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 then libvlc_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 to vlc_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
    • Author Maintainer

      So, let's go with

      struct libvlc_time
      {
          int64_t value;
          int32_t timescale;
      };

      defined in a libvlc_time.h with some helpers.

      Should helpers be static inline headers or symbols ?

    • IMO static inline are better because they can be optimized at compilation. It's only worth using symbols when the code becomes tricky. Also they won't break ABI when changing.

    • Author Maintainer

      Time operation becomes tricky due to overflow handling, and <stdckdint.h> is not always available, so we need to use the compat code, so new symbols it is...

    • Please register or sign in to reply
  • Steve Lhomme
    Steve Lhomme @robUx4 started a thread on commit 87677937
117 117 libvlc_picture_get_height( const libvlc_picture_t* pic );
118 118
119 119 /**
120 * Returns the time at which this picture was generated, in milliseconds
120 * Returns the time at which this picture was generated, in microiseconds (us)
  • Steve Lhomme
    Steve Lhomme @robUx4 started a thread on commit 87677937
  • 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;
  • Steve Lhomme
    Steve Lhomme @robUx4 started a thread on commit 87677937
  • 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;
  • Steve Lhomme
    Steve Lhomme @robUx4 started a thread on commit 87677937
  • 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) */
  • Thomas Guillem mentioned in merge request !6114 (closed)

    mentioned in merge request !6114 (closed)

  • Please register or sign in to reply
    Loading