[vlc-devel] [PATCH 4/4] dlna: add a DLNA stream out

[Filip Roséen]

Hi Shaleen,

On 2018-09-22 13:55, Shaleen Jain wrote:

> On Thu, 2018-09-20 at 16:02 +0200, Filip Roséen wrote:
> > Hi Shaleen,
> > For what it is worth, do note that you are allowed to implement this
> > using c++11 (which would come in handy to shorten some of the paths
> > in your implementation).
> C++ has a lot of boilerplate but I'll try and use it more where I can
> (maybe you have a specific block in mind?).

This is not about the `c++` boilerplate per sey, I just wanted to
point out that c++11 is allowed, and that it would shorten the
implementation. If you want insight in how to shorten the current
implementation, even with `c++98`, I am available on IRC.

> > I can understand the rationale for unconditionally stopping any
> > previous instance on start, but it is not immediately obvious when
> > one sees an invocation of startSoutChain, and as such I would
> > personally prefer if the calleer is responsible for stopping any
> > previous stream prior to starting new ones (if that is intended).
> This is mainly because to enforce only ever having a single output
> stream running considering every chain could have a transcoding step.
> The soutChain start and stop are similar designs to that of chromecast.

You should be able to prove that only one instance is running, even
without this check. If it is logically possible to have more than one
instance spawning, than one could argue that the logic of the
implementation is flawed.

If the check is there as a "better safe than sorry" thing, I do not
agree with the rationale; you should be able to prove that you do not
initiate another call to create a new stream if a stream is running.

> > I would consider it a usage-error if stopSoutChain is invoked
> > with NULL, so is it not safe to leave out the above check - and, if
> > it is even possible to end up with such case, handle it ouside of
> > this function?
> p_out here is the output stream chain which can be null if the stream
> chain is not yet started. What is passed here is the input stream.
> Again similar design of chromecast.

My message was poorly worded, but why is `stopSoutChain` called in
cases where you cannot prove whether a stream is started or not? `c++`
has a well-defined way to manage clean-up for things (such as through
the use of destructors), so I am still not convinced why you need this
check.

About the "similarity" in design of chromecast; if you want to reuse
parts of the chromecast implementation, make helpers that would then
be used in both (if the similarities are actually code-duplication).

Also, please note that I never reviewed the chromecast module, the
only time I looked at it was for fixing bugs. If I state an opinion
here that would also apply to parts of another module within VLC, my
opinion applies to both places.

> > Maybe I am missing something, but I don’t see any case
> > where out_streams[i]->p_sub_id would be NULL, but maybe I read the
> > implementation a little bit too fast.
> Similar to what is done by chromecast.

See previous remark. Just because something is done the same way as
somewhere else does not always mean that it is recommended practice.

> The DLNA/UPNP spec specifies separate actions for syncronised playback
> to multiple devices which I plan to add once the single playback is
> merged and see how that could work with VLM.

This is not about the specification of `DLNA/UPNP`, it is about the
way you set up the *sout-path*. If we consider the path a "handle",
you have `stream.mp4` directly under root, which is the basics of the
problem (too generic).

> As for the webinterface, it works for me locally while streaming
> similtaneously but to be safe I have changed the http-port for dlna
> to 7070 to not be the same as the web interface default port. 

As mentioned, the linked tickets and the relevant commit should
explain the problem well enough (changing port helps, but only until
someone decided to change the port of
*whatever-module-that-might-collide-with-yours*).

The web-interface works for you as there are no collions in terms of
the "listener" path used by your implementation, but that doesn't mean
that such collision cannot happen in the future (nor that the, in my
opinion, necessary steps have been taken to mitigate such problems).

We need an easy way for modules to generate unique paths that will not
suffer from collisions with other modules as part of the *httpd
stack*, but for now I strongly suggest that caution is taken inside
the modules that do listen to paths like these.

Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180922/4b1bf8bf/attachment.html>