Closed Bug 729541 Opened 13 years ago Closed 12 years ago

Implement JSEP/etc signaling for WebRTC

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Version:
Other Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: jesup, Assigned: emannion)

References

Details

Attachments

(4 files, 10 obsolete files)

Unit test code for PeerConnectionImpl f=emannion
17.27 KB, patch
ekr
: review-
Details | Diff | Splinter Review
JSEP/PeerConnectionInterface - Signaling code side f=emannion
235.59 KB, patch
ekr
: review-
Details | Diff | Splinter Review
Remove SDP parameter from CreateAnswer
17.88 KB, patch
jesup
: review+
jesup
: checkin+
Details | Diff | Splinter Review
fix for syntax error in signaling_unittests
1.12 KB, patch
jesup
: review+
jesup
: checkin+
Details | Diff | Splinter Review
Implement JSEP (etc) signaling for PeerConnections based on Cisco sipcc library. See W3C spec and JSEP draft for details
Depends on: 731421
PATCH CONTENTS 1. SDP Mode startup invoked by CallControlManager::startSDPMode 2. Asynchronous startup of SIPCC and startup speed improvements 3. Four partially implemented JSEP API's, CreateOffer, CreateAnswer, SetLocalDescription and SetRemoteDescription 4. Partially implemented SIPCC interface called PeerConnection 5. Some code cleanup CreateOffer and CreateAnswer are mostly implemented but we may decide to use existing SDP component call state methods to generate the SDP, this needs some more time. SetLocalDescription and SetRemoteDescription are not implemented, the API's and structure of these methods are implemented. This methods will be completed when we have a better idea what they should actually do. The hints/Constraints API needs to be developed, so its not in this patch, that will happen soon, for audio and video on\off, not sure what else should be in constraints. Right now I am using globals to pass and retrieve offer and answer SDP from SIPCC, this will be improved in a future patch, for now this works. CreateOffer and CreateAnswer are synchronous in this patch as the spec mandated, this will probably change in a future patch, I implemented this with a workerthread and a monitor, this code will be removed. SIPCC startup is now asynchronous, this alowed me to remove the startup monitor and improved SIPCC startup considerably. There will be an OnDevice callback that will signal startup result. I also only allocate 2 call chains on startup instead of 51, which will dramatically reduce resource usage and speed startup. Appologies I jammed a lot of different functionality into this patch including some random code tidy up. Hope a few of you can take a look.
Attachment #622391 - Flags: feedback+
Assignee: nobody → emannion
This is a replacement patch for the previous one. PATCH CONTENTS 1. SDP Mode startup invoked by CallControlManager::startSDPMode 2. Asynchronous startup of SIPCC and startup speed improvements. 3. Four implemented JSEP API's, CreateOffer, CreateAnswer, SetLocalDescription and SetRemoteDescription. 4. Partially implemented SIPCC interface called PeerConnection. 5. CreateOffer and CreateAnswer are now asyncronous, the PeerConnection UI will receive a callback when opetration's are complete. 6. Some code cleanup NOTE This patch does not contain any UI code or any test code. That is coming in a separate patch very soon. REVIEW The main two areas I could use reviewing are a) the PeerConnection interface files, there are three. PeerConnection.h, PeerConnectionImpl.h and PeerConnectionImpl.cpp b) The structure of the 4 JSEP API's. This mainly involves the new code in fsmdef.c. I am not 100% sure the way I am using SIPCC state machine is the best way, but i'm pretty sure it is. Instead of calling existing fsmdef methods to match call states I created 4 new fsmdef JSEP methods and took the code from what I believe are the correct call state functions and placed it into each JSEP method. The other way would be to match the correct JSEP method based on action to an existing fsmdef call function. I believe there is too much non relavant code then getting called that will need to be disabled. But if you decide you ant me to go this way I think 2 days work will implement this other approach. fsmdef_ev_createoffer -- generates offer SDP, then releases call, not saving state. fsmdef_ev_createanswer -- negotiates offer SDP, generates answer SDP, then releases call, not saving state. fsmdef_ev_setlocaldesc -- based on action parameter, either offer or answer the appropriate lsm methods are invoked. offer -- big TODO here, as we need to inject SDP created from CreateOffer rather than just generate as before. For now I am generating it as before. answer -- set call state in lsm to CONNECTED which should start media. call state is preserved between setLocalDesc and setRemoteDesc calls. fsmdef_ev_setremotedesc -- based on action parameter, either offer or answer the appropriate lsm methods are invoked. offer -- negotiate offer SDP answer -- set call state in lsm to connected which should start media. TODO -- probably a lot more than this list 1. All the media layer below JSEP and lsm needs to be written. 2. When we call SetRemoteDescription we need to add the remote streams, again this depends on a new media interface. 3. Defensive coding against JSEP API's getting called in unusual order or with strange parameters. 4. localDescription and remoteDescription API's need to be written and decide where we store the SDP for them. Not difficult. 5. CreateOffer does not currently reserve resources. 6. No hints\constraints implemented yet.
Attachment #622391 - Attachment is obsolete: true
Attachment #624764 - Flags: feedback+
Removed test UI and added the same patch again. All comments in the last section still apply.
Attachment #624764 - Attachment is obsolete: true
Attachment #624765 - Flags: feedback+
Comment on attachment 624765 [details] [diff] [review] Updated: JSEP Patch including proposed SIPCC Interface partially implemented. Review of attachment 624765 [details] [diff] [review]: ----------------------------------------------------------------- Reviewed everything except PeerConnectionImpl.* and PeerConnection.h ::: media/webrtc/signaling/src/callcontrol/CallControlManagerImpl.cpp @@ +254,5 @@ > + > + CSFLogInfoS(logTag, "startSDPMode(" << user << " )"); > + if(phone != NULL) > + { > + setConnectionState(ConnectionStatusEnum::eReady); Enum is redundant @@ +261,5 @@ > + return false; > + } > + > + // Check preconditions. > + if(localIpAddress.empty() || localIpAddress == "127.0.0.1") I agree in most cases it indicates some type of error for the local address to be 127.0.0.1, are there cases (for example in test code) where it might be reasonable? @@ +276,5 @@ > + softPhone->setLocalAddressAndGateway(localIpAddress, defaultGW); > + phone->addCCObserver(this); > + > + phone->setP2PMode(true); > + phone->setSDPMode(true); It seems odd for both modes to be set, is this correct? @@ +283,5 @@ > + if (!bStarted) { > + setConnectionState(ConnectionStatusEnum::eFailed); > + } else { > + setConnectionState(ConnectionStatusEnum::eReady); > + } Does startService() block? I think it used to, but just wanted to flag this be verified. ::: media/webrtc/signaling/src/sipcc/core/common/init.c @@ +292,5 @@ > > PHNChangeState(STATE_FILE_CFG); > > /* initialize message queues */ > + I'd remove the whitespace-only changes form this file ::: media/webrtc/signaling/src/sipcc/core/gsm/fim.c @@ +530,5 @@ > /* > * No call chain, so get a new call chain, > * but only if the event is a call establishment event. > */ > + remove the whitespace changes from this file ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm.c @@ +311,5 @@ > > if (platThreadInit("GSMTask") != 0) { > return; > } > + Remove whitespace-only change ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c @@ +5303,5 @@ > > + if ((lcb == NULL) && (action != CC_ACTION_MWI)) { > + LSM_DEBUG(get_debug_string(DEBUG_INPUT_NULL), fname); > + return (CC_RC_ERROR); > + } Remove whitespace-only change ::: media/webrtc/signaling/src/sipcc/core/gsm/sm.c @@ +43,5 @@ > #include "fsm.h" > #include "text_strings.h" > #include "ccapi.h" > > + remove whitespace-only change ::: media/webrtc/signaling/src/sipcc/core/includes/ccapi.h @@ +67,5 @@ > > > // global sdp structure > typedef struct cc_global_sdp_ { > + char offerSDP[1024]; Arbitrary constant (and for SDP a small one). Constants like this should come from somewhere, be symbolic, and be provably correct or checked. @@ +843,5 @@ > cc_features_t feature_id; > cc_feature_data_t data; > boolean data_valid; > + cc_jsep_action_t action; > + char sdp[1024]; Ditto on size
I appreciate the review and I have implemented most of your comments, which will be in the next patch. Here are some comments on your comments. I will allow a localIP of 127.0.0.1 as loopback media is now working. Thanks. I removed setting the connection states in this method as we are starting asynchronously now. This may need re-visiting I am using 4096 for SDP size as in: #define SDP_SIZE 4096 Maybe this is not enough?
Unfortunately, there is no "enough" size for SDP. I've seen (contrived, but semi-plausible) 10K SDPs. 4K is reasonable for most SDPs, but if you put a 4K limit on it it MUST be rigidly checked for on each set/append/etc. Defensive programming. This is a potential attack route by feeding in remote SDPs that are over the bounds or enticing the implementation to generate a offer or response. We can guard against incoming over-length SDPs, but it's harder to guard against an OFFER pushing us over the bounds in a response, etc. Best is to make it dynamic and limited only by memory. Next best is to carefully use bounds checking on all modifications.
I agree with jesup: this should be dynamically allocated.
Comment on attachment 624765 [details] [diff] [review] Updated: JSEP Patch including proposed SIPCC Interface partially implemented. Review of attachment 624765 [details] [diff] [review]: ----------------------------------------------------------------- After looking at this, I'm concerned that this doesn't implement the right architecture. I'm less worried about the internal plumbing than I am about what the general paradigm is. Generally, what would be useful for me would be to see a document that described the general internal flow of the system. I.e., an architecture doc divorced from a lot of the plumbing details. ::: media/webrtc/signaling/include/CC_CallInfo.h @@ +330,5 @@ > @return int - the current call volume level, or -1 if it cannot be determined > */ > virtual int getVolume() = 0; > + > + virtual std::string getSDP() = 0; What does this do? The SDP for what? Please comment. ::: media/webrtc/signaling/include/CC_Service.h @@ +116,3 @@ > virtual bool setROAPProxyMode(bool mode) = 0; > virtual bool setROAPClientMode(bool mode) = 0; > Generally, this API seems problematic. These are not orthoganal settings. I.e., you can't be in P2PMode and SDPMode at once. Suggest replace with an enum. ::: media/webrtc/signaling/include/CallControlManager.h @@ +137,1 @@ > What is the user value here? There is no concept of a user in JSEP. ::: media/webrtc/signaling/include/PeerConnection.h @@ +52,5 @@ > + > + static PeerConnectionInterface *CreatePeerConnection(); > + virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0; > + virtual StatusCode CreateOffer(const std::string& hints, std::string *offer) = 0; > + virtual StatusCode CreateAnswer(const std::string& hints, const std::string& offer, std::string* answer) = 0; 1. I wouldn't do this with an observer class. What happens if I call CreateOffer() twice? How do I distinguish errors? 2. If you're going to have async calls, they should return void or at minimum return an error if the arguments are so bogus it can't proceed, like, say, no callbacks provided. I.e., errors should be indicated via one mechanism, not two, and since you mnust be able to generate async errors, that means you need to do them async. More generally, we should probably talk about the threading discipline. What threads are these callbacks executed on? For that matter, what threads are these calls made on? ::: media/webrtc/signaling/src/callcontrol/CallControlManagerImpl.cpp @@ +261,5 @@ > + return false; > + } > + > + // Check preconditions. > + if(localIpAddress.empty() || localIpAddress == "127.0.0.1") More generally, you need to suppress any code which references the local IP address when you are in JSEP/SDP mode. The only component of the system which gets to know about IP addresses is ICE and it will tell you what you need to know. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +64,5 @@ > + > + pcobserver_ = observer; > + addr_ = GetLocalActiveInterfaceAddressSDP(); > + ccm_ = CSF::CallControlManager::create(); > + ccm_->setLocalIpAddressAndGateway(addr_,""); See other comments about the IP address. @@ +75,5 @@ > + return PC_OK; > +} > + > +StatusCode PeerConnectionImpl::CreateOffer(const std::string& hints, std::string *offer) { > + *offer = call_->createOffer(CC_SDP_DIRECTION_SENDRECV, hints); I thought we had talked about inferring the video direction from the streams which had been added? @@ +126,5 @@ > + } else if (CREATEANSWER == info->getCallState()) { > + std::string sdpstr = info->getSDP(); > + if (pcobserver_) > + pcobserver_->OnCreateAnswerSuccess(sdpstr); > + } You should have some checks here (at least asserts?) to make sure you are handling every state. @@ +139,5 @@ > +#include <fcntl.h> > +#include <netdb.h> > + > +// POSIX Only Implementation > +std::string GetLocalActiveInterfaceAddressSDP() What is this stuff doing? Is there a reason we need to have any knowledge of the active IP address? ::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c @@ +257,5 @@ > + CCAPP_DEBUG(DEB_L_C_F_PREFIX, DEB_L_C_F_PREFIX_ARGS(SIP_CC_PROV, GET_CALL_ID(call_handle), > + GET_LINE_ID(call_handle), fname)); > + > + const string_t numbers = "1234"; > + return cc_invokeFeature(call_handle, CC_FEATURE_CREATEOFFER, video_pref, JSEP_NO_ACTION, numbers); Where is video_pref coming from? ::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call.c @@ +151,5 @@ > + init_empty_str(gROAPSDP.offerAddress); > + init_empty_str(gROAPSDP.answerSDP); > + init_empty_str(gROAPSDP.offerSDP); > + gROAPSDP.audioPort = audioPort; > + gROAPSDP.videoPort = videoPort; What's this port stuff? @@ +153,5 @@ > + init_empty_str(gROAPSDP.offerSDP); > + gROAPSDP.audioPort = audioPort; > + gROAPSDP.videoPort = videoPort; > + } > + return CC_CallFeature_CreateOffer(handle, video_pref); Under what circumstances would CreateOffer() be legitimate if you weren't in sdpmode? ::: media/webrtc/signaling/src/sipcc/core/common/ui.c @@ +1599,5 @@ > // do nothing. > } > + > + > +void ui_create_offer(call_events event, line_t nLine, callid_t nCallID, What do the changes in this file do? When would these fxns be called? ::: media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c @@ +1243,5 @@ > + /* Copy the caller ID */ > + cc_cp_caller(&pmsg->data.call_info.caller_id, > + &data->call_info.caller_id); > + } > + /* What is a caller ID? ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c @@ +2779,5 @@ > + cc_causes_t cause = CC_CAUSE_NORMAL; > + cc_msgbody_info_t msg_body; > + cc_feature_t *msg = (cc_feature_t *) event->msg; > + line_t line = msg->line; > + callid_t call_id = msg->call_id; you appear to have a whitespace problem here. @@ +2986,5 @@ > + > + fsm_change_state(fcb, __LINE__, FSMDEF_S_CALL_SENT); > + > + } else if (JSEP_ANSWER == action) { > + Are you enforcing that set_remote was called first here? I'm not seeing it.
(In reply to Eric Rescorla from comment #8) > After looking at this, I'm concerned that this doesn't implement the right > architecture. > > I'm less worried about the internal plumbing than I am about what the > general paradigm > is. Generally, what would be useful for me would be to see a document that > described > the general internal flow of the system. I.e., an architecture doc divorced > from a lot of > the plumbing details. Agreed. I was mostly micro-reviewing in my look at it so far. > ::: media/webrtc/signaling/include/PeerConnection.h > @@ +52,5 @@ > > + > > + static PeerConnectionInterface *CreatePeerConnection(); > > + virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0; > > + virtual StatusCode CreateOffer(const std::string& hints, std::string *offer) = 0; > > + virtual StatusCode CreateAnswer(const std::string& hints, const std::string& offer, std::string* answer) = 0; > > 1. I wouldn't do this with an observer class. What happens if I call > CreateOffer() twice? How do I distinguish errors? > 2. If you're going to have async calls, they should return void or at > minimum return an error if the arguments are so bogus it can't proceed, > like, say, no callbacks provided. > I.e., errors should be indicated via one mechanism, not two, and since you > mnust be able to generate async errors, that means you need to do them async. > > More generally, we should probably talk about the threading discipline. What > threads are these callbacks executed on? For that matter, what threads are > these calls made on? Agreed - PeerConnection is the part I haven't reviewed yet. As for the observer issue: Enda I believe is copying the design currently used in libjingle's PeerConnection code (see peerconnectionimpl.h).
No longer blocks: 694807
No longer depends on: 731421
Both sets off comments are brilliant and really useful. I have implemented some of your comments and now for the next while I am going to look at drawing up some kind of SIPCC design to represent when it is doing. This is really the main feedback I wanted, as I am breaking from the normal call states and events and creating new ones for SDPMode/JSEP and I wanted to run this by you. Here are my replys to your comments: Please take a look. > 1. I wouldn't do this with an observer class. What happens if I call CreateOffer() twice? How do I distinguish errors? Jesup is correct I am taking some of the work done in PeerConnection in libjingle into account, this applies to how we set the PeerConnection observer. The next patch you will see I have added callbacks for all JSEP API's, this is not from libjingle. If you call CreateOffer twice you should get the correct onError callback with the correct call to CorrectOffer. I don't see how they can be out of order, or is it a big issue if they are? > More generally, we should probably talk about the threading discipline. What threads are these callbacks executed on? For that matter, what threads are these calls made on? Yes, this is something I wanted to discuss, right now I am relying on the SIPCC ccapp north side thread to both handoff to and to get events from SIPCC on. I was thinking we may need another thread in the PeerConnection interface similar to what Jesup has already done in the PeerConnection patches. I do know the ccapp thread works very well, but maybe there are too many layers of indirection between it and the browser UI to be acceptable. > + phone->setP2PMode(true); > + phone->setSDPMode(true); Agreed, I am now only starting in SDPMode and widened what it prevents starting up in SIPCC, instead of using P2P mode to prevent stuff happening. In next patch. > More generally, you need to suppress any code which references the local IP address when you are in JSEP/SDP mode. > The only component of the system which gets to know about IP addresses is ICE and it will tell you what you need to know. > + ccm_->setLocalIpAddressAndGateway(addr_,""); I did not want to remove this code until we are ready to hook up ICE. Right now we have to some degree SDP that will work in a non NAT env. I was hoping to not break that until do all that work as a chunk. > + > +StatusCode PeerConnectionImpl::CreateOffer(const std::string& hints, std::string *offer) { > + *offer = call_->createOffer(CC_SDP_DIRECTION_SENDRECV, hints); > I thought we had talked about inferring the video direction from the streams which had been added? I had not intended in changing this until we got around to either the constraints or MediaStream work. But yes this will change. > + CCAPP_DEBUG(DEB_L_C_F_PREFIX, DEB_L_C_F_PREFIX_ARGS(SIP_CC_PROV, GET_CALL_ID(call_handle), > + GET_LINE_ID(call_handle), fname)); > + > + const string_t numbers = "1234"; > + return cc_invokeFeature(call_handle, CC_FEATURE_CREATEOFFER, video_pref, JSEP_NO_ACTION, numbers); > Where is video_pref coming from? This will be removed when we move to constraints. > + init_empty_str(gROAPSDP.offerAddress); > + init_empty_str(gROAPSDP.answerSDP); > + init_empty_str(gROAPSDP.offerSDP); > + gROAPSDP.audioPort = audioPort; > + gROAPSDP.videoPort = videoPort; > What's this port stuff? This will be removed when we move to constraints. These are the port settings currently used in SDP. > Under what circumstances would CreateOffer() be legitimate if you weren't in sdpmode? Maybe we can prevent access to JSEP when not in SDPMode or at least return a sutable error Other comments will be in next Patch.
(In reply to enda mannion (:emannion) from comment #10) > > 1. I wouldn't do this with an observer class. What happens if I call CreateOffer() twice? How do I distinguish errors? > > Jesup is correct I am taking some of the work done in PeerConnection in > libjingle into account, this applies to how we set the PeerConnection > observer. The next patch you will see I have added callbacks for all JSEP > API's, this is not from libjingle. > If you call CreateOffer twice you should get the correct onError callback > with the correct call to CorrectOffer. I don't see how they can be out of > order, or is it a big issue if they are? In general the caller shouldn't need to count on ordering. I want to provide a context object and have it passed back to me. > > More generally, we should probably talk about the threading discipline. What threads are these callbacks executed on? For that matter, what threads are these calls made on? > > Yes, this is something I wanted to discuss, right now I am relying on the > SIPCC ccapp north side thread to both handoff to and to get events from > SIPCC on. I was thinking we may need another thread in the PeerConnection > interface similar to what Jesup has already done in the PeerConnection > patches. I do know the ccapp thread works very well, but maybe there are > too many layers of indirection between it and the browser UI to be > acceptable. I'm not sure I'm processing what you are currently doing. In general, my intuition is that we should have a totally separate PC thread. See: http://www.websequencediagrams.com/?lz=dGl0bGUgU2lnbmFsaW5nIFRocmVhZHMgKENyZWF0ZU9mZmVyKQpwYXJ0aWNpcGFudCAiRE9NACAHIiBhcyBET00AEw1QQyBhcyBQQwAnDkNDQVBQX1Rhc2sAMAUACgUASg1TVFMgYXMgU1RTCgpET00gLT4gUEM6IERpc3BhdGNoAIECDlBDIC0-AD8GOiBGRUFUVVJFX0NSRUFURV9PRkZFUgAbB1BDOiAAgUMGIFRyYW5zcG9ydEZsb3dcbltEVExTLCBJQ0VdCgCBFwUAYBEAXg1ET00ACRIKbm90ZSBsZWZ0IG9mAB0GSlMgY2FsbGJhY2tcbndpdGggb2ZmZXIKClNUUwCBQhFJQ0UgY2FuZGlkYXRlcwBVFgAUEABPJUlDRQoKCg&s=default > > More generally, you need to suppress any code which references the local IP address when you are in JSEP/SDP mode. > > The only component of the system which gets to know about IP addresses is ICE and it will tell you what you need to know. > > > + ccm_->setLocalIpAddressAndGateway(addr_,""); > > I did not want to remove this code until we are ready to hook up ICE. Right > now we have to some degree SDP that will work in a non NAT env. I was hoping > to not break that until do all that work as a chunk. But this local address is about SIP signaling, not about SDP, no?
> I'm not sure I'm processing what you are currently doing. In general, my > intuition is that we should have a totally separate PC thread. See: What is STS> I guess its the GSM state machine? By PC do you mean a worker thread running in the PeerConnection backend? If I put together a design of SIPCC, can you tell me what you are looking for? Right now my plan is some block diagram's and include some of the state transition flows I have done already? > But this local address is about SIP signaling, not about SDP, no? That IP address is used for signaling and also as the local IP address in SDP.
(In reply to enda mannion (:emannion) from comment #12) > > I'm not sure I'm processing what you are currently doing. In general, my > > intuition is that we should have a totally separate PC thread. See: > > What is STS> I guess its the GSM state machine? STS is the SocketTransportService. The main networking thread. > By PC do you mean a worker > thread running in the PeerConnection backend? Yes. > If I put together a design of SIPCC, can you tell me what you are looking > for? Right now my plan is some block diagram's and include some of the state > transition flows I have done already? More an overall description of the changes you plan to make.
Over the weekend an issue occurred with the BMO database which resulted in duplication of dependencies. The dependency issue may have resulted in "Depends On" and "Blocks" values being removed while updating a bug. This issue should now be resolved, however dependencies may need to be manually restored to some bugs. This bug had dependencies removed during the failure period and will need verification that the dependency removal(s) were intentional. Please help out by taking a look at this bug and adding anything back that was mistakenly removed.
Blocks: webrtc
Blocks: 694807
No longer blocks: webrtc
Depends on: 731421
This patch will now build on Alder default (thanks Ethan) and also runs on Default. I use my own provate UI patch for testing. You will get SDP back from CreateOffer and CreateAnswer but it is generated the old way, we still have to wire up the MediaStreams to generate SDP correctly. There are not a whole bunch of other changes except I added readyState to the PeerConnection interface based on work done in libjingle that seemd to match what I had in mind. It may be worth taking a look at that for review. I also prevent ending the call after CreateOffer and CreateAnswer are called now all three JSEP API's can be called on the same call, we must discuss how we will handle the ordering of these API's I have some ideas.
Attachment #624765 - Attachment is obsolete: true
Attachment #627722 - Flags: feedback-
NEW CONTENTS * 4 main JSEP API's are reshuffeled somewhat to match more closely the Arch wiki sequence diagrams. * Startup and shutdown improvements with a new sipcc_state enum. * Error and Success callbacks for all 4 JSEP methods.
Attachment #627722 - Attachment is obsolete: true
For the GTest patch: The bin will be obj.../dist/bin/signaling_unittests If you're on Linux, you'll need the patch from here - https://bugzilla.mozilla.org/show_bug.cgi?id=758843 Also on Linux you'll need to set your LD_LIBRARY_PATH to your obj.../dist/bin directory.
Attachment #628914 - Attachment is obsolete: true
Attachment #629329 - Attachment is obsolete: true
Comment on attachment 634208 [details] [diff] [review] JSEP/PeerConnectionInterface - Signaling code side f=emannion Review of attachment 634208 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/include/PeerConnection.h @@ +96,5 @@ > + > + // When SDP is parsed and a candidate line is found this method is called. > + // It should hook back into the media transport to notify it of ICE candidates listed in the SDP > + // PeerConnectionImpl does not parse ICE candidates, just pulls them out of the SDP. > + virtual void FoundIceCandidate(const std::string& strCandidate) = 0; You need an ICE Completed event. @@ +130,5 @@ > + // for each of the codecs supported on this system or > + // only the minimum codecs from the standard will be listed > + // The codecs listed will turn into a= lines in SDP > + // CodecInfo will be deleted on destruction of peerconnection object. > + virtual void AddCodec(bool isVideo, CodecInfo* pCodecInfo) = 0; I'm a little puzzled as to what this does. Like, who is going to be in a position to call this? Surely the consumer of this API wants every codec that he can possibly have so that that way when I get a new copy of webrtc.org with opus, I want opus to just show up, not to have to do something to make it happen. @@ +136,5 @@ > + // Initialize() should be called before any of the JSEP calls > + // It will startup and allocate everything needed for the state machine > + // It is asynch - observer->OnStateChange() will be called with the states > + // kStarting and then kStarted when it's ready to be used. > + virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0; If this is async, then why does it not return void? If something goes wrong is there any time that a StatusCode of success would not be returned? Also, where is StatusCode declared? @@ +142,5 @@ > + // JSEP calls > + virtual StatusCode CreateOffer(const std::string& hints) = 0; > + virtual StatusCode CreateAnswer(const std::string& hints, const std::string& offer) = 0; > + virtual StatusCode SetLocalDescription(Action action, const std::string& sdp) = 0; > + virtual StatusCode SetRemoteDescription(Action action, const std::string& sdp) = 0; My comments about StatusCode apply here as well. @@ +149,5 @@ > + virtual const std::string& localDescription() const = 0; > + virtual const std::string& remoteDescription() const = 0; > + > + // Adds the stream created by GetUserMedia > + virtual void AddMediaStream(int mediaStreamId) = 0; I'm a little puzzled by the mediaStreamId thing here. Why is this an integer? This should be a MediaStream. @@ +150,5 @@ > + virtual const std::string& remoteDescription() const = 0; > + > + // Adds the stream created by GetUserMedia > + virtual void AddMediaStream(int mediaStreamId) = 0; > + virtual void AddMediaTrack(int mediaStreamId, int mediaTrackId, bool isVideo) = 0; Why isn't isVideo (like any other property) something you can tell from looking at the track? @@ +168,5 @@ > + > + // puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state, etc. > + virtual void Shutdown() = 0; > + > + virtual ~PeerConnectionInterface() {}; I would feel happier if you had a pure virtual destructor here so that people had to think about it.
Attachment #634208 - Flags: review-
Comment on attachment 634207 [details] [diff] [review] Unit test code for PeerConnectionImpl f=emannion Review of attachment 634207 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +73,5 @@ > + > + > +class TestObserver : public sipcc::PeerConnectionObserver > +{ > +private: Any particular reason for this definition order? I'm used to public, then private, then protected, with the constructors first. @@ +74,5 @@ > + > +class TestObserver : public sipcc::PeerConnectionObserver > +{ > +private: > + static const int observerWaitTimeout = 100; // In seconds This value seems ridiculously long. What could posisble take 100 seconds. @@ +101,5 @@ > + { > + pc = peerConnection; > + pLock = PR_NewLock(); > + pCondVar = PR_NewCondVar(pLock); > + } These can be in the initializer list. @@ +114,5 @@ > + { > + PR_Lock(pLock); > + printf("WAITING\n"); > + PRStatus status = PR_WaitCondVar(pCondVar, PR_SecondsToInterval(observerWaitTimeout)); > + printf("DONE WAITING\n"); Life would probably be better if you use C++ iostreams rather than C style stdio. @@ +123,5 @@ > + > + bool NotifyObserverCalled() > + { > + PR_Sleep(PR_MillisecondsToInterval(100)); > + What is this doing? @@ +293,5 @@ > + public: > + SignalingTest() {} > + ~SignalingTest() {} > + > + void Init() Why are you using Init()? You should be using SetUp() and TearDown(). Then you don't need to explicitly call thist. @@ +299,5 @@ > + size_t found = 2; > + ASSERT_TRUE(found > 0); > + > + pc = sipcc::PeerConnectionInterface::CreatePeerConnection(); > + ASSERT_TRUE(pc != NULL); ASSERT_NE(NULL, pc) or ASSERT_TRUE(pc) @@ +306,5 @@ > + > + pObserver = new TestObserver(pc); > + ASSERT_TRUE(pObserver != NULL); > + > + ASSERT_TRUE(pc->Initialize(pObserver) == PC_OK); ASSERT_EQ() @@ +424,5 @@ > + ::testing::InitGoogleTest(&argc, argv); > + > + int result = RUN_ALL_TESTS(); > + > + printf("COMPLETE\n"); You don't need this, gtest will do it.
Attachment #634207 - Flags: review-
(In reply to Eric Rescorla from comment #22) > Comment on attachment 634208 [details] [diff] [review] > JSEP/PeerConnectionInterface - Signaling code side f=emannion > > Review of attachment 634208 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/include/PeerConnection.h > @@ +96,5 @@ > > + > > + // When SDP is parsed and a candidate line is found this method is called. > > + // It should hook back into the media transport to notify it of ICE candidates listed in the SDP > > + // PeerConnectionImpl does not parse ICE candidates, just pulls them out of the SDP. > > + virtual void FoundIceCandidate(const std::string& strCandidate) = 0; > > You need an ICE Completed event. Agreed > > @@ +130,5 @@ > > + // for each of the codecs supported on this system or > > + // only the minimum codecs from the standard will be listed > > + // The codecs listed will turn into a= lines in SDP > > + // CodecInfo will be deleted on destruction of peerconnection object. > > + virtual void AddCodec(bool isVideo, CodecInfo* pCodecInfo) = 0; > > I'm a little puzzled as to what this does. Like, who is going to be in a > position to call this? Surely the consumer of this API wants every codec > that he can possibly have so that that way when I get a new copy of > webrtc.org with opus, I want opus to just show up, not to have to do > something to make it happen. > Ideally Initialize() would take an MediaEngine pointer which could be used to enumerate the available codecs. Unfortunately, we're in gkmedias so the PC code in content/media will have to walk through the codec list and call AddCodec() for each. The SIPCC code requires the codecs be listed before it starts up rather than just before createOffer/Answer, but that could probably be changed. > @@ +136,5 @@ > > + // Initialize() should be called before any of the JSEP calls > > + // It will startup and allocate everything needed for the state machine > > + // It is asynch - observer->OnStateChange() will be called with the states > > + // kStarting and then kStarted when it's ready to be used. > > + virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0; > > If this is async, then why does it not return void? If something goes wrong > is there any time that a StatusCode of success would not be returned? Also, > where is StatusCode declared? > Yes, it should return void and StatusCode should be moved from peer_connection_types.h > @@ +142,5 @@ > > + // JSEP calls > > + virtual StatusCode CreateOffer(const std::string& hints) = 0; > > + virtual StatusCode CreateAnswer(const std::string& hints, const std::string& offer) = 0; > > + virtual StatusCode SetLocalDescription(Action action, const std::string& sdp) = 0; > > + virtual StatusCode SetRemoteDescription(Action action, const std::string& sdp) = 0; > > My comments about StatusCode apply here as well. > > @@ +149,5 @@ > > + virtual const std::string& localDescription() const = 0; > > + virtual const std::string& remoteDescription() const = 0; > > + > > + // Adds the stream created by GetUserMedia > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > I'm a little puzzled by the mediaStreamId thing here. Why is this an > integer? This should be a MediaStream. > It would be great to be able to take a MediaStream* but it's not available to gkmedias. The code in content/media will have an AddStream(MediaStream*), but in the SIPCC code we're using handles for streams and when we need an action done on the stream we'll have to call back to code in content/media using the handle. It's ugly yes, so if you have a better idea that still works with the current state of gkmedias that would be great. > @@ +150,5 @@ > > + virtual const std::string& remoteDescription() const = 0; > > + > > + // Adds the stream created by GetUserMedia > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > + virtual void AddMediaTrack(int mediaStreamId, int mediaTrackId, bool isVideo) = 0; > > Why isn't isVideo (like any other property) something you can tell from > looking at the track? Currently you can't find out from a MediaTrack whether it's audio or video. Although I think you can guess from the track ID. We can't take a MediaTrack* either in gkmedias code. > > @@ +168,5 @@ > > + > > + // puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state, etc. > > + virtual void Shutdown() = 0; > > + > > + virtual ~PeerConnectionInterface() {}; > > I would feel happier if you had a pure virtual destructor here so that > people had to think about it. Agreed.
PATCH INFORMATION ICE Methods Method: virtual void FoundIceCandidate(const std::string& strCandidate) = 0; On the PeerConnectionObserver class which has all the async callbacks from SIPCC there is a new API called: FoundIceCandidate I am not sure if this is the best name as all other methods begin with On such as OnAddStream(). FoundIceCandidate will get invoked when new ICE candidates are discovered by the ICE engine and these are to be trickled by the signaling mechanism to the remote side. This API I think will also need another parameter to indicate that gathering is complete as per section 4.5 the JSEP spec. Method: virtual void AddIceCandidate(const std::string& strCandidate) = 0; This method is on the actual PeerConnectionInterface. It is used to set an ICE candidate received from the remote side into the ICE engine. I am not sure if this method needs to take multiple candidates rather than a string that indicates it takes only one at a time. When this method is called the candidate needs to be set into the ICE agent, this is what needs more discussion. My thoughts are. The PeerConnectionInterface maintains the ICE engine as in controls its lifetime. The PCInterface also creates the necessary ICE transport objects and streams. When we recieve candidates from ICE we will store them in the C++ code in VcmSIPCCBinding, this is where the GSM thread will reach out to grab them to generate SDP. The GSM Thread will also be the point where remote candidates are received and from here they will be bubbled up to the PeerConnection Interface via a callback. This is not yet implemented. We also need a method to set local candidates into VCMSIPCCBinding so they can be used to generate local SDP. Maybe we need new sections in the Architecture wiki for all this.
Target Milestone: --- → mozilla15
Version: Trunk → Other Branch
(In reply to Ethan Hugg [:ehugg] from comment #24) > > I'm a little puzzled as to what this does. Like, who is going to be in a > > position to call this? Surely the consumer of this API wants every codec > > that he can possibly have so that that way when I get a new copy of > > webrtc.org with opus, I want opus to just show up, not to have to do > > something to make it happen. > > > > Ideally Initialize() would take an MediaEngine pointer which could be used > to enumerate the available codecs. Unfortunately, we're in gkmedias so the > PC code in content/media will have to walk through the codec list and call > AddCodec() for each. The SIPCC code requires the codecs be listed before it > starts up rather than just before createOffer/Answer, but that could > probably be changed. Sure, but this shouldn't be the responsibility of the caller, who surely has no idea about the MediaEngine. > > > @@ +142,5 @@ > > > + // JSEP calls > > > + virtual StatusCode CreateOffer(const std::string& hints) = 0; > > > + virtual StatusCode CreateAnswer(const std::string& hints, const std::string& offer) = 0; > > > + virtual StatusCode SetLocalDescription(Action action, const std::string& sdp) = 0; > > > + virtual StatusCode SetRemoteDescription(Action action, const std::string& sdp) = 0; > > > > My comments about StatusCode apply here as well. > > > > @@ +149,5 @@ > > > + virtual const std::string& localDescription() const = 0; > > > + virtual const std::string& remoteDescription() const = 0; > > > + > > > + // Adds the stream created by GetUserMedia > > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > > > I'm a little puzzled by the mediaStreamId thing here. Why is this an > > integer? This should be a MediaStream. > > > > It would be great to be able to take a MediaStream* but it's not available > to gkmedias. > The code in content/media will have an AddStream(MediaStream*), but in the > SIPCC code we're using handles for streams and when we need an action done > on the stream we'll have to call back to code in content/media using the > handle. It's ugly yes, so if you have a better idea that still works with > the current state of gkmedias that would be great. > > > @@ +150,5 @@ > > > + virtual const std::string& remoteDescription() const = 0; > > > + > > > + // Adds the stream created by GetUserMedia > > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > > + virtual void AddMediaTrack(int mediaStreamId, int mediaTrackId, bool isVideo) = 0; > > > > Why isn't isVideo (like any other property) something you can tell from > > looking at the track? > > Currently you can't find out from a MediaTrack whether it's audio or video. Then that should be fixed, I guess. > Although I think you can guess from the track ID. We can't take a > MediaTrack* either in gkmedias code. Same response as above.
(In reply to Eric Rescorla from comment #22) > Comment on attachment 634208 [details] [diff] [review] > JSEP/PeerConnectionInterface - Signaling code side f=emannion > > Review of attachment 634208 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/include/PeerConnection.h > @@ +96,5 @@ > > + > > + // When SDP is parsed and a candidate line is found this method is called. > > + // It should hook back into the media transport to notify it of ICE candidates listed in the SDP > > + // PeerConnectionImpl does not parse ICE candidates, just pulls them out of the SDP. > > + virtual void FoundIceCandidate(const std::string& strCandidate) = 0; > > You need an ICE Completed event. > > @@ +130,5 @@ > > + // for each of the codecs supported on this system or > > + // only the minimum codecs from the standard will be listed > > + // The codecs listed will turn into a= lines in SDP > > + // CodecInfo will be deleted on destruction of peerconnection object. > > + virtual void AddCodec(bool isVideo, CodecInfo* pCodecInfo) = 0; > > I'm a little puzzled as to what this does. Like, who is going to be in a > position to call this? Surely the consumer of this API wants every codec > that he can possibly have so that that way when I get a new copy of > webrtc.org with opus, I want opus to just show up, not to have to do > something to make it happen. > > @@ +136,5 @@ > > + // Initialize() should be called before any of the JSEP calls > > + // It will startup and allocate everything needed for the state machine > > + // It is asynch - observer->OnStateChange() will be called with the states > > + // kStarting and then kStarted when it's ready to be used. > > + virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0; > > If this is async, then why does it not return void? If something goes wrong > is there any time that a StatusCode of success would not be returned? Also, > where is StatusCode declared? Initilize does not hand off straight away to SIPCC and there are some actions that may fail, therefore I think we need StatusCode here. Unless we implement threading in the PC Interface which maybe we should discuss. Should we have a worker thread on the PC interface to hand off all calls from and to the browser thread? > > @@ +142,5 @@ > > + // JSEP calls > > + virtual StatusCode CreateOffer(const std::string& hints) = 0; > > + virtual StatusCode CreateAnswer(const std::string& hints, const std::string& offer) = 0; > > + virtual StatusCode SetLocalDescription(Action action, const std::string& sdp) = 0; > > + virtual StatusCode SetRemoteDescription(Action action, const std::string& sdp) = 0; > > My comments about StatusCode apply here as well. I think I agree here regarding changing these to void. > > @@ +149,5 @@ > > + virtual const std::string& localDescription() const = 0; > > + virtual const std::string& remoteDescription() const = 0; > > + > > + // Adds the stream created by GetUserMedia > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > I'm a little puzzled by the mediaStreamId thing here. Why is this an > integer? This should be a MediaStream. > > @@ +150,5 @@ > > + virtual const std::string& remoteDescription() const = 0; > > + > > + // Adds the stream created by GetUserMedia > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > + virtual void AddMediaTrack(int mediaStreamId, int mediaTrackId, bool isVideo) = 0; > > Why isn't isVideo (like any other property) something you can tell from > looking at the track? > > @@ +168,5 @@ > > + > > + // puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state, etc. > > + virtual void Shutdown() = 0; > > + > > + virtual ~PeerConnectionInterface() {}; > > I would feel happier if you had a pure virtual destructor here so that > people had to think about it. I was doing that originally but the PeerConnectionInterface destructor was not getting called so I added this impl.
(In reply to Ethan Hugg [:ehugg] from comment #24) > (In reply to Eric Rescorla from comment #22) > > Comment on attachment 634208 [details] [diff] [review] > > JSEP/PeerConnectionInterface - Signaling code side f=emannion > > > > Review of attachment 634208 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: media/webrtc/signaling/include/PeerConnection.h > > @@ +96,5 @@ > > > + > > > + // When SDP is parsed and a candidate line is found this method is called. > > > + // It should hook back into the media transport to notify it of ICE candidates listed in the SDP > > > + // PeerConnectionImpl does not parse ICE candidates, just pulls them out of the SDP. > > > + virtual void FoundIceCandidate(const std::string& strCandidate) = 0; > > > > You need an ICE Completed event. > Agreed > > > > @@ +130,5 @@ > > > + // for each of the codecs supported on this system or > > > + // only the minimum codecs from the standard will be listed > > > + // The codecs listed will turn into a= lines in SDP > > > + // CodecInfo will be deleted on destruction of peerconnection object. > > > + virtual void AddCodec(bool isVideo, CodecInfo* pCodecInfo) = 0; > > > > I'm a little puzzled as to what this does. Like, who is going to be in a > > position to call this? Surely the consumer of this API wants every codec > > that he can possibly have so that that way when I get a new copy of > > webrtc.org with opus, I want opus to just show up, not to have to do > > something to make it happen. > > > > Ideally Initialize() would take an MediaEngine pointer which could be used > to enumerate the available codecs. Unfortunately, we're in gkmedias so the > PC code in content/media will have to walk through the codec list and call > AddCodec() for each. The SIPCC code requires the codecs be listed before it > starts up rather than just before createOffer/Answer, but that could > probably be changed. > > > @@ +136,5 @@ > > > + // Initialize() should be called before any of the JSEP calls > > > + // It will startup and allocate everything needed for the state machine > > > + // It is asynch - observer->OnStateChange() will be called with the states > > > + // kStarting and then kStarted when it's ready to be used. > > > + virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0; > > > > If this is async, then why does it not return void? If something goes wrong > > is there any time that a StatusCode of success would not be returned? Also, > > where is StatusCode declared? > > > > Yes, it should return void and StatusCode should be moved from > peer_connection_types.h src/sipcc/includes/peer_connection_types.h I placed this file here so it can be accessed internally in SIPCC and also at the top level interface. I use StatusCode in the GSM thread when returning errors from the JSEP API's, I also need StatusCode at the top level API. I thought this approach better but I can maintain two enums if StatusCode needs to be moved to the top level. > > > @@ +142,5 @@ > > > + // JSEP calls > > > + virtual StatusCode CreateOffer(const std::string& hints) = 0; > > > + virtual StatusCode CreateAnswer(const std::string& hints, const std::string& offer) = 0; > > > + virtual StatusCode SetLocalDescription(Action action, const std::string& sdp) = 0; > > > + virtual StatusCode SetRemoteDescription(Action action, const std::string& sdp) = 0; > > > > My comments about StatusCode apply here as well. > > > > @@ +149,5 @@ > > > + virtual const std::string& localDescription() const = 0; > > > + virtual const std::string& remoteDescription() const = 0; > > > + > > > + // Adds the stream created by GetUserMedia > > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > > > I'm a little puzzled by the mediaStreamId thing here. Why is this an > > integer? This should be a MediaStream. > > > > It would be great to be able to take a MediaStream* but it's not available > to gkmedias. > The code in content/media will have an AddStream(MediaStream*), but in the > SIPCC code we're using handles for streams and when we need an action done > on the stream we'll have to call back to code in content/media using the > handle. It's ugly yes, so if you have a better idea that still works with > the current state of gkmedias that would be great. > > > @@ +150,5 @@ > > > + virtual const std::string& remoteDescription() const = 0; > > > + > > > + // Adds the stream created by GetUserMedia > > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > > + virtual void AddMediaTrack(int mediaStreamId, int mediaTrackId, bool isVideo) = 0; > > > > Why isn't isVideo (like any other property) something you can tell from > > looking at the track? > > Currently you can't find out from a MediaTrack whether it's audio or video. > Although I think you can guess from the track ID. We can't take a > MediaTrack* either in gkmedias code. > > > > > @@ +168,5 @@ > > > + > > > + // puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state, etc. > > > + virtual void Shutdown() = 0; > > > + > > > + virtual ~PeerConnectionInterface() {}; > > > > I would feel happier if you had a pure virtual destructor here so that > > people had to think about it. > > Agreed.
(In reply to enda mannion (:emannion) from comment #27) > Initilize does not hand off straight away to SIPCC and there are some > actions that may fail, therefore I think we need StatusCode here. I don't understand why they can't fail asynchronously. > Unless we > implement threading in the PC Interface which maybe we should discuss. > Should we have a worker thread on the PC interface to hand off all calls > from and to the browser thread? I don't understand this. You can't block the main thread of the browser while you're doing stuff. Moreover, you have to return errors asynchronously to the browser anyway. > > @@ +149,5 @@ > > > + virtual const std::string& localDescription() const = 0; > > > + virtual const std::string& remoteDescription() const = 0; > > > + > > > + // Adds the stream created by GetUserMedia > > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > > > I'm a little puzzled by the mediaStreamId thing here. Why is this an > > integer? This should be a MediaStream. > > > > @@ +150,5 @@ > > > + virtual const std::string& remoteDescription() const = 0; > > > + > > > + // Adds the stream created by GetUserMedia > > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > > + virtual void AddMediaTrack(int mediaStreamId, int mediaTrackId, bool isVideo) = 0; > > > > Why isn't isVideo (like any other property) something you can tell from > > looking at the track? > > > > @@ +168,5 @@ > > > + > > > + // puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state, etc. > > > + virtual void Shutdown() = 0; > > > + > > > + virtual ~PeerConnectionInterface() {}; > > > > I would feel happier if you had a pure virtual destructor here so that > > people had to think about it. > > I was doing that originally but the PeerConnectionInterface destructor was > not getting called so I added this impl. Why do you need it to be called?
(In reply to enda mannion (:emannion) from comment #28) > (In reply to Ethan Hugg [:ehugg] from comment #24) > > (In reply to Eric Rescorla from comment #22) > > > Comment on attachment 634208 [details] [diff] [review] > > > JSEP/PeerConnectionInterface - Signaling code side f=emannion > > > > > > Review of attachment 634208 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: media/webrtc/signaling/include/PeerConnection.h > > > @@ +96,5 @@ > > > > + > > > > + // When SDP is parsed and a candidate line is found this method is called. > > > > + // It should hook back into the media transport to notify it of ICE candidates listed in the SDP > > > > + // PeerConnectionImpl does not parse ICE candidates, just pulls them out of the SDP. > > > > + virtual void FoundIceCandidate(const std::string& strCandidate) = 0; > > > > > > You need an ICE Completed event. > > Agreed > > > > > > @@ +130,5 @@ > > > > + // for each of the codecs supported on this system or > > > > + // only the minimum codecs from the standard will be listed > > > > + // The codecs listed will turn into a= lines in SDP > > > > + // CodecInfo will be deleted on destruction of peerconnection object. > > > > + virtual void AddCodec(bool isVideo, CodecInfo* pCodecInfo) = 0; > > > > > > I'm a little puzzled as to what this does. Like, who is going to be in a > > > position to call this? Surely the consumer of this API wants every codec > > > that he can possibly have so that that way when I get a new copy of > > > webrtc.org with opus, I want opus to just show up, not to have to do > > > something to make it happen. > > > > > > > Ideally Initialize() would take an MediaEngine pointer which could be used > > to enumerate the available codecs. Unfortunately, we're in gkmedias so the > > PC code in content/media will have to walk through the codec list and call > > AddCodec() for each. The SIPCC code requires the codecs be listed before it > > starts up rather than just before createOffer/Answer, but that could > > probably be changed. > > > > > @@ +136,5 @@ > > > > + // Initialize() should be called before any of the JSEP calls > > > > + // It will startup and allocate everything needed for the state machine > > > > + // It is asynch - observer->OnStateChange() will be called with the states > > > > + // kStarting and then kStarted when it's ready to be used. > > > > + virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0; > > > > > > If this is async, then why does it not return void? If something goes wrong > > > is there any time that a StatusCode of success would not be returned? Also, > > > where is StatusCode declared? > > > > > > > Yes, it should return void and StatusCode should be moved from > > peer_connection_types.h > > src/sipcc/includes/peer_connection_types.h > > I placed this file here so it can be accessed internally in SIPCC and also > at the top level interface. I use StatusCode in the GSM thread when > returning errors from the JSEP API's, I also need StatusCode at the top > level API. I thought this approach better but I can maintain two enums if > StatusCode needs to be moved to the top level. Is there a reason you're not just returning nsresult?
(In reply to Eric Rescorla from comment #29) > (In reply to enda mannion (:emannion) from comment #27) > > Initilize does not hand off straight away to SIPCC and there are some > > actions that may fail, therefore I think we need StatusCode here. > > I don't understand why they can't fail asynchronously. is it ok to have a new observer methods for this. virtual void OnInitilizeSuccess(StatusCode code) = 0; virtual void OnInitilizeError(StatusCode code) = 0; > > > Unless we > > implement threading in the PC Interface which maybe we should discuss. > > Should we have a worker thread on the PC interface to hand off all calls > > from and to the browser thread? > > I don't understand this. You can't block the main thread of the browser > while you're doing stuff. Moreover, you have to return errors asynchronously > to the browser anyway. Ya but I am wondering is there going to be a thread doing this in the PeerConnection UI above tis interface or in this interface the backend? > > > > > @@ +149,5 @@ > > > > + virtual const std::string& localDescription() const = 0; > > > > + virtual const std::string& remoteDescription() const = 0; > > > > + > > > > + // Adds the stream created by GetUserMedia > > > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > > > > > I'm a little puzzled by the mediaStreamId thing here. Why is this an > > > integer? This should be a MediaStream. > > > > > > @@ +150,5 @@ > > > > + virtual const std::string& remoteDescription() const = 0; > > > > + > > > > + // Adds the stream created by GetUserMedia > > > > + virtual void AddMediaStream(int mediaStreamId) = 0; > > > > + virtual void AddMediaTrack(int mediaStreamId, int mediaTrackId, bool isVideo) = 0; > > > > > > Why isn't isVideo (like any other property) something you can tell from > > > looking at the track? > > > > > > @@ +168,5 @@ > > > > + > > > > + // puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state, etc. > > > > + virtual void Shutdown() = 0; > > > > + > > > > + virtual ~PeerConnectionInterface() {}; > > > > > > I would feel happier if you had a pure virtual destructor here so that > > > people had to think about it. > > > > I was doing that originally but the PeerConnectionInterface destructor was > > not getting called so I added this impl. > > Why do you need it to be called? I am calling shutdown in the destructor. But I also have a shutdown API that covers this.
(In reply to Eric Rescorla from comment #30) > (In reply to enda mannion (:emannion) from comment #28) > > (In reply to Ethan Hugg [:ehugg] from comment #24) > > > (In reply to Eric Rescorla from comment #22) > > > > Comment on attachment 634208 [details] [diff] [review] > > > > JSEP/PeerConnectionInterface - Signaling code side f=emannion > > > > > > > > Review of attachment 634208 [details] [diff] [review]: > > > > ----------------------------------------------------------------- > > > > > > > > ::: media/webrtc/signaling/include/PeerConnection.h > > > > @@ +96,5 @@ > > > > > + > > > > > + // When SDP is parsed and a candidate line is found this method is called. > > > > > + // It should hook back into the media transport to notify it of ICE candidates listed in the SDP > > > > > + // PeerConnectionImpl does not parse ICE candidates, just pulls them out of the SDP. > > > > > + virtual void FoundIceCandidate(const std::string& strCandidate) = 0; > > > > > > > > You need an ICE Completed event. > > > Agreed > > > > > > > > @@ +130,5 @@ > > > > > + // for each of the codecs supported on this system or > > > > > + // only the minimum codecs from the standard will be listed > > > > > + // The codecs listed will turn into a= lines in SDP > > > > > + // CodecInfo will be deleted on destruction of peerconnection object. > > > > > + virtual void AddCodec(bool isVideo, CodecInfo* pCodecInfo) = 0; > > > > > > > > I'm a little puzzled as to what this does. Like, who is going to be in a > > > > position to call this? Surely the consumer of this API wants every codec > > > > that he can possibly have so that that way when I get a new copy of > > > > webrtc.org with opus, I want opus to just show up, not to have to do > > > > something to make it happen. > > > > > > > > > > Ideally Initialize() would take an MediaEngine pointer which could be used > > > to enumerate the available codecs. Unfortunately, we're in gkmedias so the > > > PC code in content/media will have to walk through the codec list and call > > > AddCodec() for each. The SIPCC code requires the codecs be listed before it > > > starts up rather than just before createOffer/Answer, but that could > > > probably be changed. > > > > > > > @@ +136,5 @@ > > > > > + // Initialize() should be called before any of the JSEP calls > > > > > + // It will startup and allocate everything needed for the state machine > > > > > + // It is asynch - observer->OnStateChange() will be called with the states > > > > > + // kStarting and then kStarted when it's ready to be used. > > > > > + virtual StatusCode Initialize(PeerConnectionObserver* observer) = 0; > > > > > > > > If this is async, then why does it not return void? If something goes wrong > > > > is there any time that a StatusCode of success would not be returned? Also, > > > > where is StatusCode declared? > > > > > > > > > > Yes, it should return void and StatusCode should be moved from > > > peer_connection_types.h > > > > src/sipcc/includes/peer_connection_types.h > > > > I placed this file here so it can be accessed internally in SIPCC and also > > at the top level interface. I use StatusCode in the GSM thread when > > returning errors from the JSEP API's, I also need StatusCode at the top > > level API. I thought this approach better but I can maintain two enums if > > StatusCode needs to be moved to the top level. > > Is there a reason you're not just returning nsresult? We aggreed to create the StatusCode enum way back.
Depends on: 767380
Depends on: 768325
No longer depends on: 768325
Depends on: 786158
Attached file Remove SDP parameter from CreateAnswer (obsolete) —
This patch removed the SDP parameter from CreateAnswer as per recent PeerConnection API change. This patch depends on the constraints patch. bug 784515
Attachment #670771 - Flags: feedback?(ethanhugg)
Uploaded again, clicked the patch check box this time.
Attachment #670771 - Attachment is obsolete: true
Attachment #670771 - Flags: feedback?(ethanhugg)
Attachment #671070 - Flags: feedback?(ethanhugg)
Also had a merge conflict when merged on m-c tip.
Attachment #671070 - Attachment is obsolete: true
Attachment #671070 - Flags: feedback?(ethanhugg)
Attachment #671120 - Flags: feedback?(ethanhugg)
Updated to build on m-c tip after the changes for whitespace patch.
Attachment #671120 - Attachment is obsolete: true
Attachment #671120 - Flags: feedback?(ethanhugg)
Attachment #671459 - Flags: feedback?(ethanhugg)
Comment on attachment 671459 [details] [diff] [review] Remove SDP parameter from CreateAnswer Patch relies on patch 784515. Sending to EKR for review.
Attachment #671459 - Flags: feedback?(ethanhugg) → review?(ekr)
Re-based off new hints patch and m-c tip
Attachment #671459 - Attachment is obsolete: true
Attachment #671459 - Flags: review?(ekr)
Attachment #673149 - Flags: review?(ekr)
Attachment #673149 - Flags: review?(ekr) → review?(rjesup)
Attachment #673149 - Flags: review?(rjesup) → review+
Attachment #673149 - Flags: checkin?(rjesup)
This really should have been on it's own bug, not this older master bug. Let's close this bug with this patch. https://hg.mozilla.org/integration/mozilla-inbound/rev/4b997a0e6b5f
Attachment #673149 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/4b997a0e6b5f
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attachment #674540 - Flags: review?(rjesup)
Reopening for syntax error in signaling unittest.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Bug 803318 should really be pushed before this one.
Enda, this one was pushed already. Can you explain why 803318 should have been pushed before? If it's just because of merge work, then Jesup already did that (hence the syntax error in the unittest).
Attachment #674540 - Flags: review?(rjesup) → review+
Attachment #674540 - Flags: checkin?(rjesup)
Comment on attachment 674540 [details] [diff] [review] fix for syntax error in signaling_unittests https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd4c4bac9f1
Attachment #674540 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/5bd4c4bac9f1
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: