[vlc-devel] [PATCH 1/8] stream_extractor: split joint capability into two

[Filip Roséen]

Hi Rémi,

On 2017-02-17 20:13, Rémi Denis-Courmont wrote:

> Le torstaina 16. helmikuuta 2017, 22.44.11 EET Filip Roséen a écrit :
> > These changes splits the functionality for a stream-extractor into two
> > different objects, one being a stream_extractor_t (used to extract
> > data for an entity within a stream based on an identifier), and the
> > other, stream_directory_t, is to list entities within a stream.
> > 
> > Summary of changes:
> > 
> >   - update and refine documentation belonging to stream-extractors
> >   - stream_extractor_t is now stream_extractor_t and stream_directory_t
> 
> To me, that looks better than the union.
>
> Although AFAICT, stream_directory_t is a "subset" of stream_t, and 
> stream_extractor_t would also be a subset except for the identifier. Then you 
> don´t even need new types: you can just define a new pf_activate callback 
> prototype to carry the identifier.

Yes, and that approach also crossed my mind before implementing it the
first way, and now the second way; though I considered changes to
`pf_activate` to be a more intrusive than the path taken.

I also decided to use a separate type the first time around to make it
clear that `vlc_stream_directory_CreateMRL` (formerly
`vlc_stream_extractor_CreateMRL`) shall only be called with a
`vlc_stream_directory_t` (`stream_extractor_t` when I proposed a
single type).

Hopefully such decoupling allows for easier reasoning when writing a
new module, at least that is the idea. I remember when first starting
out with the codebase and the initial (although somewhat minor)
confusion to what a `stream_t` really was (until it was a somewhat
uniform handle to everything that had to do with a vlc-stream).

A uniform handle is both a blessing and a curse, and I think
decoupling allows for easier reasoning from a maintainer point of
view (given that you still always read from a `stream_t`, but what
"generates" content is hidden through *type-erasure*)

Irrelevant data-members
-----------------------

Another argument for not making `stream_t` the "internal" type for the
modules with "stream_directory" and "stream_extractor" functionality
is that many of the members in `stream_t` are not relevant to such
modules.

A module with the mentioned capability should not care about the
following members at all, so introducing a separate type allows for
easier reasoning to what a module-maintainer should care about (and
access).

 - `stream_t::psz_name`
 - `stream_t::psz_url`
 - `stream_t::psz_location`
 - `stream_t::psz_filepath`
 - `stream_t::p_input`

It would , hopefully, prevent misuse of such members to provide
"functionality" that should be elsewhere in the chain.

For a `stream_directory_t` the only relevant callback is `pf_readdir`,
so having a distinguished type that only exposes that *callback* is
(in my opinion) a plus.

> That being written, I have not actually checked if that would enable any 
> further simplification on the core side. I just hope it would allow treating 
> extractors and directories as regular streams for purpose other than 
> instantation.

It would allow for some simplification, though in my opinion somewhat
minor given the required changes (and mental processing of how modules
are initialized). I am not sure that I (personally) would like
extractors and directories to be "regular streams" given what was
written in previous sections.

I welcome more opinions on the matter, but I have my fingers crossed
for the currently proposed design to be accepted.

Thank you for your valuable feedback, \
Filip Roséen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170217/9edb7d94/attachment.html>