Re: [Wish] WHIP enhancements

[Sergio Garcia Murillo <[e-mail]>]

Hi Adam,

Comments inline

El jue, 25 feb 2021 a las 0:25, Adam Roach (<[e-mail]>) escribió:

> Thanks for the quick review of my proposal! Comments inline below.
>
>
> On 2/24/21 02:14, Sergio Garcia Murillo wrote:
> > The need for a way to explicitly terminate the session was already by
> > several people, so I think we should definitely include it.
> >
> > https://github.com/murillo128/webrtc-http-ingest-protocol/issues/2
> >
> > However, I would be against optionalities, as it makes the testing and
> > interoperability much more difficult.
>
>
> The scheme I'm looking for here is one of a very small set of core
> functions, plus optional fields that are ignored if an implementation
> doesn't understand them. To test interop, all that would be necessary is
> to check the proper functioning of the core functions (along with, I
> suppose, cursory checks that unknown fields are correctly ignored).
>

My main concern about this is that most people will only implement the core
package and don't do the optional part.
So I would prefer to have as less optionality as possible, especially if
the optional functionality is something that may be required by some people
in order to make it work.



> > In that regard, I am against including the trickle option as the
> > server should be accessible to the publisher, and the only case where
> > it could make sense is if the WHIP server is on a public server but
> > the media servers are behind NATed network without port forwarding.
> >
> > Also, I am not sure what would be the benefits of the ice restart
> > (especially if it is optional) compared to just restarting the whole
> > publishing session again, which is something that the service has to
> > implementa anyway.
>
>
> I'm definitely okay leaving both trickle ice and ice restart functions
> off for now, as long as we have some kind of extensibility mechanism.
> What I'm trying to avoid is some kind of future situation where RT-CDNs
> are hosted behind application load balancers, and the processing nodes
> themselves lack v4 unique addresses (and are therefore natted). I don't
> see it being an issue right now, but depending on how quickly we can
> rely on 100% IPv6 deployment to end users, it may become an issue in the
> future. I don't want to be painted into a corner when/if this happens.
>

That may be an interesting use case to support in order to be future proof
and would be easy to implement server side (just ignore the client updated
candidates). So my proposal would be either to not support it, or include
it on the core package.



> > On a last note, I would prefer to make the process less json-like and
> > more http-like, reusing HTTP headers and methods as much as possible.
>
>
> I'm certainly not caught up on JSON as a syntax here, although it does
> make things considerably easier in most modern development environments.
> I'm open to other options.
>
> Regarding our use of HTTP, I'd very much like to make sure we stay
> strictly within the bounds of BCP 56 (and, after discussions with the
> Area Directors, I'm fairly certain that publication of the
> post-last-call
> https://tools.ietf.org/html/draft-ietf-httpbis-bcp56bis-09 will be
> happening in fairly short order, so that's the reference we should be
> using). So re-using existing HTTP header fields to mean exactly what
> they're defined to mean is perfectly fine; but for any new information,
> I think we need to stay with the recommendations of BCP 56bis §4.7.
>

Definitely, we should follow BCP 56.


>
> > My initial thoughts about how to implement it were that the POST
> > request to start the publication returned a 201 created response with
> > a Location header indicating the URI to the entity representing the
> > publication.
>
> I don't have any objection in principle to using HTTP header fields for
> pointing to a created resource, as long as we can make this work for all
> of our use cases.
>
>
> > And then allow the following methods on the request
> >
> > - GET for retrieving information about the state of the publication
> (TBD).
> > - OPTIONS for keep alive
> > - UPDATE for updating the session (we would need to specify what would
> > we actually allow to be updated)
> > - DELETE for terminating the session
>
>
> Here's where things get a bit dicey. The observation in BCP 56bis §5.4.2
> that OPTIONS is not widely supported is highly relevant: as CDN ingest
> nodes are frequently behind application load balancers, requiring the
> support of OPTIONS can lead to significant operational headaches if the
> load balancers don't support OPTIONS natively. There's also a bit of a
> philosophical mismatch in that "OPTIONS" doesn't *really* mean "PING,"
> and if the notion is that we're using HTTP methods as our semantic
> operation point, we really want things that are good semantic matches.
>
> UPDATE is even more problematic, as HTTP doesn't have an UPDATE method,
> and the process of adding new methods to HTTP is politically difficult
> in the best circumstances (e.g., creating a method that has broad
> applicability to all resource types -- see the difficulty and insanely
> long timeline getting PATCH defined for an example).
>
> My bad, too much time working on SIP. Obviously defining any new http
methods or headers is something we should not do.


> So while I'm not married to any of the aspects of my proposal, I think
> we need a more explicit way of indicating what operations we need than
> HTTP methods; and, at the same time, I'm seriously concerned about the
> future and the viability of a protocol that has no extension points. Can
> we work to find common ground on those two points?
>
>
>
Best regards
Sergio