748401 - Clean API for SIPCC to isolate the SIPCC internals from WebRTC code

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rescorla (:ekr)]

Attachment: Enda's proposed API

This bug added partly to allow review of Enda's proposed code.

Comment 1

[Eric Rescorla (:ekr)]

Comment on attachment 617899 [details] [diff] [review]
Enda's proposed API

Review of attachment 617899 [details] [diff] [review]:
-----------------------------------------------------------------

I only really reviewed SDPSession,h.

::: emannion/sdp/SDPSession.h
@@ +1,5 @@
> +
> +#include <string>
> +
> +enum StatusCode { SDP_OK = 2001, SDP_CREATE_OFFER_FAIL = 2002, SDP_CREATE_ANSWER_FAIL = 2003 };
> +

StatusCodes should be 0 for success, nonzero for failure. 

It's probably an error to use StatusCode unnamespaced, since it's a pretty generic name.

The codes don't need to indicate the context in which the call failed. The caller knows that. What he wants to know is what went wrong.
Maybe CreateOffer() can only fail in one way, but CreateAnswer() can fail in at least two ways: (1) the offer is bogus (2) something
went wrong internally. That said, I can think of other things that could go wrong here, like "invalid hints object".

@@ +2,5 @@
> +#include <string>
> +
> +enum StatusCode { SDP_OK = 2001, SDP_CREATE_OFFER_FAIL = 2002, SDP_CREATE_ANSWER_FAIL = 2003 };
> +
> +class SDPSessionInterface {

I agree that this is not a good name. This is not too far from a PeerConnection.

@@ +4,5 @@
> +enum StatusCode { SDP_OK = 2001, SDP_CREATE_OFFER_FAIL = 2002, SDP_CREATE_ANSWER_FAIL = 2003 };
> +
> +class SDPSessionInterface {
> +public:
> +  virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0;

Where is the definition of PeerConnectionObserver?

@@ +10,5 @@
> +  virtual StatusCode CreateAnswer(const std::string& hints, const  std::string& offer, std::string* answer) = 0;
> +  virtual StatusCode SetLocalDescription(/*Action action,*/ const  std::string& sdp) = 0;
> +  virtual StatusCode SetRemoteDescription(/*Action action,*/ const std::string& sdp) = 0;
> +  virtual const std::string& localDescription() const = 0;
> +  virtual const std::string& remoteDescription() const = 0;

Virtual destructor needed here.

@@ +13,5 @@
> +  virtual const std::string& localDescription() const = 0;
> +  virtual const std::string& remoteDescription() const = 0;
> +
> +  virtual void AddStream(LocalMediaStreamInterface* stream);    
> +  virtual void RemoveStream(LocalMediaStreamInterface* stream);

I don't think we want to reference WebRTC constructs here. Instead we want to isolate the media stack from the signaling stack here. That means that we need an abstract API for the media system that doesn't involve #including any webrtc.org code. I.e., the same isolation we are doing here for SIPCC.

@@ +17,5 @@
> +  virtual void RemoveStream(LocalMediaStreamInterface* stream);
> +};
> +
> +
> +class SDPSessionFactoryInterface {

Why is this called "Interface"? 

I think there's a category error here. There is a single SDPSessionFactory class that returns an SDPSessionInterface * of whatever type is needed.

@@ +20,5 @@
> +
> +class SDPSessionFactoryInterface {
> +public:
> +  virtual SDPSessionInterface *CreateSDPSession() = 0;
> +  

How do you destroy an SDPSessionInterface? Do you just delete it?

@@ +30,5 @@
> +
> +class SDPSessionFactory : public SDPSessionFactoryInterface {
> +public:
> +   SDPSessionInterface *CreateSDPSession();
> +};

What is this bringing to the party?

The SDPSessionFactory shouldn't be subclassed. The switch-hitting to implementations is done in the .cpp file.
Is this a singleton?

::: emannion/sdp/SDPSessionImpl.cpp
@@ +62,5 @@
> +  return SDP_OK;
> +} 
> +
> +const std::string& SDPSessionSIPCC::localDescription() const {
> +  std::string sdp = "";

returning a reference to a temporary is bad....

Comment 2

[enda mannion (:emannion)]

Replying to your comment about StatusCode namespacing, and namespacing in general,  should everything from SDPSession.h back into SIPCC be in its own namespace?  Or do you just suggest namespacing the StatusCode enum.  We could use a namespace called SIPCC for all SDPSession and SIPCC.

Comment 3

[Eric Rescorla (:ekr)]

I'm hoping Randall will weigh in on Moz convention.

Comment 4

[Cullen Jennings]

I'm worried that we want these methods attached to an object more aligned with the PeerConnection instead of making the SDPSessionInterface. The problem SDP is best at reflecting a proposed change to the state not the state and the state will be in different places. Obviously, I should come up with a concrete proposal of what is making me nervous about this but I'm having a hard time getting my head around how with will interact with the media engine and global and line state in sipcc.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Moz conventions changed recently regarding namespaces, mostly to flatten the hierarchy. See the coding style page:

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#C.2B.2B_namespaces

However, for a large block of imported code, a top-level namespace may make sense.  Ehsan would be a good person to answer; cc-ing him (we have two primary large imports - sipcc (media/webrtc/signaling; ~200K lines) and webrtc (~500K lines and maybe parts of libjingle (~200K lines)).

Comment 6

[enda mannion (:emannion)]

I am putting together a MediaInterface abstract type that will move media state out of SDPSession but will be accessible from SDPSession, for updates to reflect SDP state.  I will post my proposed header files for this later today.

(In reply to Cullen Jennings from comment #4)
> I'm worried that we want these methods attached to an object more aligned
> with the PeerConnection instead of making the SDPSessionInterface. The
> problem SDP is best at reflecting a proposed change to the state not the
> state and the state will be in different places. Obviously, I should come up
> with a concrete proposal of what is making me nervous about this but I'm
> having a hard time getting my head around how with will interact with the
> media engine and global and line state in sipcc.

Comment 7

[enda mannion (:emannion)]

Attachment: Contains header file for MediaStackInterface, and the three files for the JSEP SDPSession interface.

Added 4 files for review.  I think it would be good if people could take a look at these before the meeting tomorrow, especially the MediaStreamInterface file.  I appologise I have not been following coding standards here much, I want to focus more on the functionality in the time I have. I may be way off the mark in the functionality I am putting into MediaStreamInterface, please let me know, at lease we can use it to discuss how JSEP may behave. 

SDPInterface will have a pointer to MediaStreamInterface, that it will use to add streams and tracks. It will also be able to control those tracks.  I added SDPSession::SetMediaInterface to set this pointer.

SDPSession will use the methods NumberOfStreams and GetLocalStreamTracks to retrieve stream data so it can build SDP for CreateOffer and CreateAnswer.

I have not completed the factory implementation, I will get to that.  In reply to earlier comments, SDPSessionSIPCC::localDescription()  and SDPSessionSIPCC::remoteDescription() are incomplete and returning a ref to a temp only as a temporary measure.

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 618451 [details] [diff] [review]
Contains header file for MediaStackInterface, and the three files for the JSEP SDPSession interface.

Please format this as a patch so that splinter will pick it up properly.

Comment 9

[(no longer active)]

(In reply to Randell Jesup [:jesup] from comment #5)
> Moz conventions changed recently regarding namespaces, mostly to flatten the
> hierarchy. See the coding style page:
> 
> https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#C.2B.
> 2B_namespaces
> 
> However, for a large block of imported code, a top-level namespace may make
> sense.  Ehsan would be a good person to answer; cc-ing him (we have two
> primary large imports - sipcc (media/webrtc/signaling; ~200K lines) and
> webrtc (~500K lines and maybe parts of libjingle (~200K lines)).

It would definitely make sense to put those two under namespace sipcc and webrtc, respectively.  But please avoid nested namespaces (like, no mozilla::sipcc and mozilla::webrt)  :-)

Comment 10

[enda mannion (:emannion)]

Attachment: Here are the same files as the last attachment in patch format

I hope this patch will spark discussion about how SIPCC and the MediaStreams will commmunicate and help produce a good design.  I just include the Abstract Interface for now, I will introduce a factory ASAP.