<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">
code{white-space: pre-wrap;}
span.smallcaps{font-variant: small-caps;}
span.underline{text-decoration: underline;}
div.column{display: inline-block; vertical-align: top; width: 50%;}
</style>
</head>
<body>
<p>Hi Shaleen,</p>
<p>On 2018-09-22 13:55, Shaleen Jain wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> On Thu, 2018-09-20 at 16:02 +0200, Filip Roséen wrote:</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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).</code></pre>
<p>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?).</p>
</blockquote>
</blockquote>
<p>This is not about the <code>c++</code> 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 <code>c++98</code>, I am available on IRC.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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).</code></pre>
<p>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.</p>
</blockquote>
</blockquote>
<p>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.</p>
<p>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.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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?</code></pre>
<p>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.</p>
</blockquote>
</blockquote>
<p>My message was poorly worded, but why is <code>stopSoutChain</code> called in cases where you cannot prove whether a stream is started or not? <code>c++</code> 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.</p>
<p>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).</p>
<p>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.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
<p>Similar to what is done by chromecast.</p>
</blockquote>
</blockquote>
<p>See previous remark. Just because something is done the same way as somewhere else does not always mean that it is recommended practice.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
</blockquote>
<p>This is not about the specification of <code>DLNA/UPNP</code>, it is about the way you set up the <em>sout-path</em>. If we consider the path a “handle”, you have <code>stream.mp4</code> directly under root, which is the basics of the problem (too generic).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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. </code></pre>
</blockquote>
<p>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 <em>whatever-module-that-might-collide-with-yours</em>).</p>
<p>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).</p>
<p>We need an easy way for modules to generate unique paths that will not suffer from collisions with other modules as part of the <em>httpd stack</em>, but for now I strongly suggest that caution is taken inside the modules that do listen to paths like these.</p>
<p>Best Regards,<br />
Filip</p>
</body>
</html>