Skip to content
Snippets Groups Projects

Draft: Adapt bindings to the LibVLC 4.0 rework

Open Ayush Dey requested to merge deyayush6/libvlcpp:libvlc-4-rework into master

Adapt libvlcpp bindings to the LibVLC 4.0 rework (vlc!3503)

Overview of changes:

  • Callback event handlers now have their own class. Users can create an instance of this class by passing the mandatory callbacks as arguments, while optional callbacks can be assigned through the class methods. This provides flexibility for users to assign optional event handlers as needed.

  • The EventManager has been removed as part of the rework. All events are now handled directly by their respective binding's Callbacks class.

  • New Thumbnailer and Parser classes have been introduced for the bindings of their respective APIs.

  • All API calls have been updated in accordance with the rework.

TODO:

  • A wrapper function is still pending implementation. This function will be used during callback assignment, accepting raw libvlc struct objects from LibVLC and creating corresponding libvlcpp wrapper objects before exposing them to the user.
    Example: In the on_update callback within libvlc_media_player_watch_time_cbs, the goal is to accept a libvlc_media_player_time_point_t* object from LibVLC, create a MediaPlayer::TimePoint object, and expose it to the user, allowing further use of its methods for the libvlc_media_player_time_point_* APIs.

  • The LIBVLC_VERSION checks need to be removed. As discussed, the rework will not maintain backward compatibility with LibVLC 3.0 or earlier, due primarily to the removal of EventManager.

  • Documentation for MediaPlayerCallbacks needs to be added.

  • Tests using the updated API bindings need to be written.

Note: I'll complete the last three tasks after the code has been reviewed and finalised.

Edited by Ayush Dey

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #533530 failed

Merge request pipeline failed for 8ab46d24

Ready to merge by members who can write to the target branch.

Merge details

  • 32 commits will be added to master.
  • Source branch will be deleted.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
2079 int interpolate( int64_t system_now_us, int64_t *out_ts_us, double *out_pos )
2080 {
2081 return libvlc_media_player_time_point_interpolate( *this, system_now_us,
2082 out_ts_us, out_pos );
2083 }
2084
2085 /**
2086 * Get the date of the next interval
2087 *
2088 * \param system_now_us same system date used by interpolate()
2089 * \param interpolated_ts_us ts returned by interpolate()
2090 * \param next_interval_us next interval, in us
2091 * \return the absolute system date, in us, of the next interval
2092 * \version LibVLC 4.0.0 or later
2093 */
2094 int64_t getNextDate(int64_t system_now_us, int64_t interpolated_ts_us,
  • Alaric Senat
    Alaric Senat @asenat started a thread on commit 58eeaf53
  • 2156 */
    2157 template <typename OnSeek>
    2158 void setOnSeek(OnSeek&& onSeek)
    2159 {
    2160 static_assert(signature_match_or_nullptr<OnSeek, ExpectedOnSeek>::value,
    2161 "Mismatched on seek callback prototype");
    2162 cbs.on_seek = CallbackWrapper<(unsigned int)CallbackIdx::TimeSeek,
    2163 void(*)(void*, const libvlc_media_player_time_point_t*)>::
    2164 wrap(*m_callbacks, std::forward<OnSeek>(onSeek));
    2165 }
    2166
    2167 /** Returns a void pointer to the array of callbacks. */
    2168 void* getCallbacksPtr()
    2169 {
    2170 return m_callbacks.get();
    2171 }
  • Alaric Senat
  • Alaric Senat
  • Alaric Senat
  • Alaric Senat
    Alaric Senat @asenat started a thread on commit 41682c41
  • 142 */
    143 void destroy()
    144 {
    145 libvlc_thumbnailer_request_destroy( *this );
    146 }
    147
    148 /**
    149 * Set position for the thumbnailer request
    150 *
    151 * \param pos The position at which the thumbnail should be generated
    152 * \param fast_seek true for fast seek, false for precise seek
    153 */
    154 void setPosition( double pos, bool fast_seek )
    155 {
    156 libvlc_thumbnailer_request_set_position( *this, pos, fast_seek );
    157 }
    • Comment on lines +154 to +157

      @tguillem I'm not exactly sure about that but it seems that requests should be modified like that only before queueing? If that's the case, Maybe a request builder that only gives access to a unique_ptr would better represent the lifetime of the request objects.

      Edited by Alaric Senat
    • Indeed, you build the request and should not touch it after you send it.

      But the result callback uses the same request as an identifier, so I don't know it its work here. We can adapt the C API if needed.

    • Can we use a bool flag isQueued in the Request class? We could set this flag when the request is being queued and check it in the setter methods to determine whether the request has already been queued before proceeding with the setter operation.

      While unique_ptr can be used, I can't understand how ownership can be transferred back in the callback (since the callback uses the same request object)

    • But the result callback uses the same request as an identifier, so I don't know it its work here. We can adapt the C API if needed.

      It feels like the callback gives back the request object only for the target media to be available. We could just give access only to the media reference in the libvlc callback?

      I spent some time trying to have a satisfying ownership based solution for this issue. The thumbnailer is supposed to own the request except in case of cancellation, which make the wrapper awkward to write here.

      If we keep the libvlc API as-is, I suggest consuming the request in queue() and returning a special object containing a weak reference on request_t that would crash if called after the event completion (just like the C API)...

      class QueuedRequest {
      public:
          QueuedRequest(libvlc_thumbnailer_request_t&);
          void cancel() {/* ... destroy here */ }
      private:
          libvlc_thumbnailer_request_t &weak;
      };
      
      QueuedRequest queue( Request&& req );
    • @tguillem To be fair, the QueuedRequest or an opaque handle dedicated to cancellation on the request object could be implemented in libvlc. It makes the request only cancelable and would avoid users trying to modify it while being processed. Making it an actual safe to use weak reference, failing or doing nothing when the request is completed.

      Edited by Alaric Senat
    • So, we are talking about the following:

      LIBVLC_API libvlc_thumbnailer_request_id_t
      libvlc_thumbnailer_queue( libvlc_thumbnailer_t *thumbnailer,
                                libvlc_thumbnailer_request_t *req );

      libvlc_thumbnailer_queue take ownership of the request and return an opaque handle used only to cancel it ?

      For the implementation, libvlc_thumbnailer_queue will return the same pointer to req (but the user doesn't have to know it). The request will either be destroyed internally by the ended callback, or if the user cancel it.

    • The thing is, I don't know how you could protect from accidental double frees from the user by returning the same pointer.
      The scenario causing a double free right now is:

      • user queues a request
      • the request complete fast and gets destroyed automatically
      • user accidentally calls destroy on the dangling request handle

      If you think that it just shouldn't happen, I'm fine with having the same pointer as the handler.

      Edit: I just remembered that the request is refcounted, so my point above might be not possible by design.

      Edited by Alaric Senat
    • After some discussion with @chub and @asenat I will try the following:

      LIBVLC_API unsigned long
      libvlc_thumbnailer_queue( libvlc_thumbnailer_t *thumbnailer,
                                libvlc_thumbnailer_request_t *req );

      libvlc_thumbnailer_queue takes ownership of libvlc_thumbnailer_request_t, it returns a unique integer incremented for each request, that can be used to cancel a request.

    • Please register or sign in to reply
  • Looks promising, thank you!

    Don't forget to adapt the current examples (if they need it).

  • Ayush Dey added 27 commits

    added 27 commits

    • 5c09cbdd...2a1132b0 - 17 earlier commits
    • c4ad49c9 - MediaPlayer: use a class MediaPlayerCallbacks for callback handling
    • 661f9f73 - structures: accept const libvlc_chapter_description_t* in ChapterDescription constructor
    • 41739144 - MediaPlayer: expose structs from on_chapter_selection_changed callback
    • cb72dd55 - MediaListPlayer: remove MediaListPlayerEventManager class
    • 49f906ae - Media: remove MediaEventManager class
    • 3efbccca - EventManager: remove class EventManager
    • e20dfa14 - MediaPlayer: add on_media_attachments_added callback
    • 376831b6 - MediaTrack: rename libvlc_media_track_hold to retain
    • c576d906 - RendererDiscoverer: rename libvlc_renderer_item_hold to retain
    • 74189740 - test: adapt examples and add thumbnailer test

    Compare with previous version

  • Ayush Dey marked the checklist item A wrapper function is still pending implementation. This function will be used during callback assignment, accepting raw libvlc struct objects from LibVLC and creating corresponding libvlcpp wrapper objects before exposing them to the user. as completed

    marked the checklist item A wrapper function is still pending implementation. This function will be used during callback assignment, accepting raw libvlc struct objects from LibVLC and creating corresponding libvlcpp wrapper objects before exposing them to the user. as completed

  • Ayush Dey marked the checklist item Tests using the updated API bindings need to be written. as completed

    marked the checklist item Tests using the updated API bindings need to be written. as completed

  • Ayush Dey marked the checklist item Tests using the updated API bindings need to be written. as incomplete

    marked the checklist item Tests using the updated API bindings need to be written. as incomplete

  • Ayush Dey marked the checklist item Tests using the updated API bindings need to be written. as completed

    marked the checklist item Tests using the updated API bindings need to be written. as completed

  • Update:

    • A wrapper function has been added to convert libvlc struct objects into the corresponding libvlcpp wrapper objects before exposing them to the user.
    • Example tests have been adapted according to the rework.
  • Thomas Guillem
  • Thomas Guillem
  • Thomas Guillem mentioned in merge request vlc!6183 (merged)

    mentioned in merge request vlc!6183 (merged)

  • Ayush Dey added 28 commits

    added 28 commits

    • 74189740...82c7d442 - 18 earlier commits
    • 0e85e58d - MediaPlayer: use a class MediaPlayerCallbacks for callback handling
    • 92ef7437 - structures: accept const libvlc_chapter_description_t* in ChapterDescription constructor
    • f2e364a7 - MediaPlayer: expose structs from on_chapter_selection_changed callback
    • 5a1e1463 - MediaListPlayer: remove MediaListPlayerEventManager class
    • a8e605f9 - Media: remove MediaEventManager class
    • 8389288b - EventManager: remove class EventManager
    • 56ab1bb3 - MediaPlayer: add on_media_attachments_added callback
    • b15e1eff - MediaTrack: rename libvlc_media_track_hold to retain
    • f462b7f6 - RendererDiscoverer: rename libvlc_renderer_item_hold to retain
    • b5889806 - test: adapt examples and add thumbnailer test

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading