Introduce typed callbacks for stream_t

This PR introduces typed callbacks as an alternative to the variadic argument pf_control.

The goal by introducing typed callbacks is to have a safer interface to plug into but also to move away from the mess around va_list (each compiler defines va_list differently, needs to have va_arg everywhere, limited support in safer languages, prone to errors, ...).

The driving factor behind this is to permit programming languages that don't have support for implementing a function that uses va_list to still be able to implement those callbacks.

As a first step this MR only transition the stream part of stream_t not it's demux part which will be done in a follow up MR.

A complete plan (still subject to change) can be found below !3200 (comment 365712).

---

This approach was suggested in the Rust RFC:

You can just add type-safe callbacks, and retain pf_control as fallback for legacy code. It wouldn't be the first time.

changed the description

Thanks

Thanks for your contribution!

When all of the following conditions are fulfilled, your MergeRequest will be reviewed by the Team:

• the check pipeline passes
• the MR is considered as 'mergeable' by gitlab

You can find more details about the acceptance process here.

This message was automatically generated by homer-bot.

added MRStatus::NotCompliant label

IMHO, all control requests should first be converted to type-safe wrappers. Then, pf_control can be replaced by type-safe function pointers.

Also, in light of Denis' points, an ops pointer to const should also probably preliminarily be added.

Which means there should be at the very least three different parts here (which may or may not be spliced in separate MRs).

changed milestone to %4.0

I agree this MR should be split in multiple parts. Basically you could have 1 commit per function you add, one for the replacement in the code. Then, last, remove the pf_control. I don't think we need to keep it.

In principles, I agree that we should remove pf_control and I was indeed instigating this on the Rust MR. But I think it's unrealistic to remove pf_control in the short term. There are just way too many modules involved to avoid a transition with both current and new callback styles in parallel.

The way it's been done for filters/encoders is typically one of the two methods:

1/

• Add the ops table
• Implement fallback on ops callback when left unimplemented
• Implement the ops in each modules, replacing the previous method and keeping NULL as previous pf_ table and set_callbacks
• Remove the fallback on ops and previous pf_ table call and only use ops
• Remove the pf_ table

2/

• Add the ops table
• Create a malloc-ed ops table whenever the ops is not defined, fallbacking to the previous pf_ table (and freeing it in close) and add the call to close
• Replace all the calls to pf_ foo in the core code by calls to the ops table
• Implement the ops in each modules, replace set_callbacks
• Remove the creation of the malloc-ed ops table
• Remove the pf_ table

Okay so based on the comments what about doing it like this:

1. Phase 1: Ops table
   • Create a struct vlc_stream_operations { /* the current controls struct field of this MR */ }
   • Add const struct vlc_stream_operations *ops; to stream_t
   • Implement fallback for individual callbacks based on pf_control
   • Change vlc_stream_vaControl and vlc_demux_vaControl to use the ops table
2. Phase 2: Typed calls
   • Create typed method of each callback
   • Replace the use vlc_stream_vaControl and vlc_demux_vaControl with the proper typed method calls
   • Remove altogether vlc_stream_vaControl, vlc_demux_vaControl and related methods
3. Phase 3: Bye pf_control and other pf_*
   • Introduce "table info" for constant information (mainly seek/fastseek/...)
   • Replace in each module STREAM_*/DEMUX_* with proper ops callbacks or table info entry
   • Remove pf_control, pf_read, pf_readdir, ...
   • Remove STREAM_* and DEMUX_*
   • Remove the fallbacks method
   • Celebrate

Doing the phase 1 would be enough for the Rust work and phase 2 would be really nice for Rust but not obligatory.

---

One small nit, what to do when pf_control isn't implemented and not all operations are implemented ?

I would lean towards returning an error/default value (VLC_EGENERIC or false for the can_* methods) because there are a lot of callbacks may of them returning VLC_EGENERIC directly in the C modules, this would also cut down the maintenance when one want to add a new callback (ie not needing to go trough every module). This is by the way the approach I used in the MR.

Instead of returning nothing (ie void), returning like I did in the MR the actual type seems better. (That is bool for the can_* methods.)

The previous way of doing it with controls was done in order to be completely ABI-compatible with older versions of plugins and still add new controls. Nowadays, we prefer the ABI tagging in vlc_plugins and only load the compatible ABI, while requesting a rebuild for the other non-compatible plugins.

What was happening for those values is that returning an error because the module didn't specify it would lead to use a default value of false (ie. not supported), so having an info table where you specify true at open when supported or do nothing to keep the default value will provide the same behaviour without the burden of creating symbols/callbacks for this.

added 1 commit

• c92c472a - Introduce typed callbacks for stream_t

Compare with previous version

changed title from Introduce typed callbacks for stream_t and demux_t to Introduce typed callbacks for stream_t

changed the description

I have updated the MR taking into account the different comments. The MR now implements the ops fallback mechanism to pf_control and is limited for the moment to the pure stream part of stream_t.

added MRStatus::InReview label and removed MRStatus::NotCompliant label