792188 - Import media/webrtc/signaling from alder to m-c

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

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

Attachment: Rollup media/webrtc/signaling patch

To do this, we need to review the changes made to media/webrtc/signaling to change it from a SIP/call-control stack into a JSEP/webrtc signaling and call-control stack.

Please attach all bugs relevant to this as "Depends on".  I'll attach a current "roll-up" patch that includes all changes since import.  It was obtained via 

hg diff -r 6e0d5d7dd645 media/webrtc/signaling

with a tip on alder of 108238:44a5894b1ec8

Initial reviewers are ekr, myself and ted (for build issues).  Looking for more...

Comment 1

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

Attachment: Output of hg log -r 6e0d5d7dd645:tip media/webrtc/signaling

This should have all the changesets that are part of the rollup patch for reference

Comment 2

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

Comment on attachment 662277 [details] [diff] [review]
Rollup media/webrtc/signaling patch

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

Reviewed through Wrapper.h.  So far nothing but nits.

::: media/webrtc/signaling/include/CC_Call.h
@@ +307,5 @@
> +        
> +        virtual int createAnswer(const std::string & hints, const std::string & offersdp) = 0;
> +        
> +        virtual int setLocalDescription(cc_jsep_action_t action, const std::string & sdp) = 0;
> +        

Spurious whitespace (this and 3 preceding blank lines)

::: media/webrtc/signaling/include/CC_CallInfo.h
@@ +357,5 @@
> +           get status code 
> +           @param [in] handle - call info handle
> +           @return code
> +         */
> +        virtual cc_int32_t getStatusCode() = 0;        

spurious whitespace in this change

::: media/webrtc/signaling/include/CSFVideoControl.h
@@ +51,5 @@
>  	class ECC_API VideoControl
>  	{
>  	public:
> +		virtual ~VideoControl() {};
> +		

spurious whitespace

::: media/webrtc/signaling/sipcc-config.mk
@@ +29,5 @@
> +# use your version of this file under the terms of the MPL, indicate your
> +# decision by deleting the provisions above and replace them with the notice
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.

mpl2 please

::: media/webrtc/signaling/src/callcontrol/CallControlManagerImpl.h
@@ +79,5 @@
>          virtual bool registerUser( const std::string& deviceName, const std::string& user, const std::string& password, const std::string& domain );
>  
>          virtual bool startP2PMode(const std::string& user);
> +        
> +        virtual bool startSDPMode();   

spurious whitespace

@@ +104,5 @@
>  
>  	private: // Data Storage
>  
>          // Observers
> +		LockNSPR m_lock;;

remove second ;

::: media/webrtc/signaling/src/common/AutoLockNSPR.h
@@ +35,5 @@
> + public:
> +  AutoLockNSPR(LockNSPR& lock) : lock_(lock) {
> +    lock_.Acquire();
> +  }
> +  ~AutoLockNSPR() { 

Various trailing WS

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 662277 [details] [diff] [review]
Rollup media/webrtc/signaling patch

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

::: media/webrtc/signaling/test/Makefile.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +DEPTH = ../../../..

DEPTH = @DEPTH@

@@ +10,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE = test_signaling
> +
> +LIBS = $(EXTRA_DSO_LIBS) \

conventionally for multi-line lists the first value goes on the second line.

Comment 4

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 Cleanup for import of signaling code

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 665143 [details] [diff] [review]
Patch 1 Cleanup for import of signaling code


Fixed the problems listed in comments 1 and 3.

Removed sipcc-config.mk entirely as it is no longer used.

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 665143 [details] [diff] [review]
Patch 1 Cleanup for import of signaling code


Patch 1 pushed to Alder - https://hg.mozilla.org/projects/alder/rev/46e1ab723d88

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - removal of WebrtcMediaProvider

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 666056 [details] [diff] [review]
Patch 2 - removal of WebrtcMediaProvider


I was able to remove everything in the media/webrtc directory

Comment 9

[Ethan Hugg [:ehugg]]

Attachment: Patch 3 - Change to MutexAutoLock

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 666100 [details] [diff] [review]
Patch 3 - Change to MutexAutoLock


Full removal of AutoLockNSPR.  Requires Patch 2 applied.

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 666056 [details] [diff] [review]
Patch 2 - removal of WebrtcMediaProvider

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

lgtm

Comment 12

[Eric Rescorla (:ekr)]

Comment on attachment 666100 [details] [diff] [review]
Patch 3 - Change to MutexAutoLock

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

lgtm

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 666056 [details] [diff] [review]
Patch 2 - removal of WebrtcMediaProvider


Patch 2 pushed to alder - http://hg.mozilla.org/projects/alder/rev/98b8d7e180bc

Comment 14

[Ethan Hugg [:ehugg]]

Attachment: Patch 4 - Un-duplicate code in ui.c and ccapi.c

Comment 15

[Ethan Hugg [:ehugg]]

Comment on attachment 666609 [details] [diff] [review]
Patch 4 - Un-duplicate code in ui.c and ccapi.c


Removed duplicate code with static subroutines and eliminated fname in favor of __FUNCTION__

Comment 16

[Ethan Hugg [:ehugg]]

Comment on attachment 666100 [details] [diff] [review]
Patch 3 - Change to MutexAutoLock


Patch 3 pushed to Alder - https://hg.mozilla.org/projects/alder/rev/02555ba14aba

Comment 17

[Suhas]

Attachment: Patch 5

Comment 18

[Suhas]

Attachment: Patch 5 MediaConduit Rollup

Comment 19

[Suhas]

Attachment: Patch 5 MediaConduit Rollup

Comment 20

[Ethan Hugg [:ehugg]]

Comment on attachment 666775 [details] [diff] [review]
Patch 5 MediaConduit Rollup

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

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +100,5 @@
>      CSFLogError(logTag, "%s Unable to initialize VoENetwork", __FUNCTION__);
>      return kMediaConduitSessionNotInited;
>    }
>  
> +  if(!(mPtrVoECodec = VoECodec::GetInterface(mVoiceEngine)))

Why do we do a ->Release on the others but not this one in the dtor.  Is that correct?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +111,3 @@
>    }
>  
> +  if( !(mPtrViECodec = ViECodec::GetInterface(mVideoEngine)))

Is it correct that this is the only one we don't release in the dtor?

Comment 21

[Ethan Hugg [:ehugg]]

Attachment: Patch 6 - Move Wrapper back to using its own autolock

Comment 22

[Ethan Hugg [:ehugg]]

Comment on attachment 666812 [details] [diff] [review]
Patch 6 - Move Wrapper back to using its own autolock


This appears to fix the leak reports of Mutex that made mochitests fail for debug builds as well as the runtime warnings seen when running unittests.

Comment 23

[Eric Rescorla (:ekr)]

Comment on attachment 666812 [details] [diff] [review]
Patch 6 - Move Wrapper back to using its own autolock

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

lgtm

::: media/webrtc/signaling/src/common/Wrapper.h
@@ +84,5 @@
> +class LockNSPR {
> +public:
> +  LockNSPR() : lock_(NULL) {
> +    lock_ = PR_NewLock();
> +    PR_ASSERT(lock_);

MOZ_ASSERT? My fault, but still needs to be fixed.

@@ +121,5 @@
>  {
>  private:
>      typedef std::map<typename T::Handle, typename T::Ptr>      	HandleMapType;
>  	HandleMapType 	handleMap;
> +    LockNSPR	handleMapMutex;

Indent

Comment 24

[Ethan Hugg [:ehugg]]

Attachment: Patch 6 - Move Wrapper back to using its own autolock

Comment 25

[Ethan Hugg [:ehugg]]

Comment on attachment 666847 [details] [diff] [review]
Patch 6 - Move Wrapper back to using its own autolock


Patch 6 pushed to Alder - http://hg.mozilla.org/projects/alder/rev/c303bd22f4a1

Comment 26

[Suhas]

Attachment: Patch 5 MediaConduit Rollup, UnitTests Music generation

Comment 27

[Suhas]

Comment on attachment 666887 [details] [diff] [review]
Patch 5 MediaConduit Rollup, UnitTests Music generation

1. Added Release() for V*Codec()
2. Added music generation code from xiph.org
3. Generating input and output audio files in the obj/*/test/directory now
4. Removed resource_manager.h dependency
5. Removed webrtc_standalone.cpp dependency.

Comment 28

[Ethan Hugg [:ehugg]]

Attachment: Patch 7 Rollup fix of cpr, src-common, misc and core/includes

Comment 29

[Ethan Hugg [:ehugg]]

Comment on attachment 667064 [details] [diff] [review]
Patch 7 Rollup fix of cpr, src-common, misc and core/includes


Addresses comments in Rietveld 9001, 11001 and 13001.

Comment 30

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

Comment on attachment 666887 [details] [diff] [review]
Patch 5 MediaConduit Rollup, UnitTests Music generation

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

The space issues do not have to be addressed now (though if it's convenient, that's always good to do).  M-x delete-trailing-whitespace in emacs :-)

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +29,4 @@
>      return NULL;
>    } 
> +  CSFLogDebug(logTag,  "%s Successfully created AudioConduit ", __FUNCTION__);
> +  return obj;   

trailing spaces here (and elsewhere)

::: media/webrtc/signaling/src/media-conduit/AudioConduit.h
@@ +34,5 @@
>  /**
>   * Concrete class for Audio session. Hooks up  
>   *  - media-source and target to external transport 
>   */
> +class WebrtcAudioConduit:public AudioSessionConduit			

trailing spaces (and elsewhere)

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ +51,5 @@
>  struct VideoCodecConfig 
>  {
>  
> +  /*
> +   * The data-types for these properties mimic the 

trailing space

::: media/webrtc/signaling/src/media-conduit/MediaConduitErrors.h
@@ +11,5 @@
>  enum MediaConduitErrorCode
>  {
> +kMediaConduitNoError = 0,              // 0 for Success,greater than 0 imples error
> +kMediaConduitSessionNotInited = 10100, // Session not initialized.10100 serves as
> +                                       // base for the conduit errors 

trailing spaces (and elsewhere)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +26,4 @@
>      return NULL;
> +  } 
> +  CSFLogDebug(logTag,  "%s Successfully created VideoConduit ", __FUNCTION__);
> +  return obj;  

trailing spaces (and elsewhere)

@@ +32,5 @@
>  WebrtcVideoConduit::~WebrtcVideoConduit()
>  {
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
>  
> +  for(unsigned int i=0; i < mRecvCodecList.size(); i++)

In theory this should be size_type (it's a std::vector<>)

@@ +67,5 @@
> +  if(mPtrViECodec)
> +  {
> +    mPtrViECodec->Release();
> +  }
> +  

Many lines in here have trailing spaces

@@ +101,4 @@
>    // TRACING
>    mVideoEngine->SetTraceFilter(webrtc::kTraceAll);
>    mVideoEngine->SetTraceFile( "Vievideotrace.out" );
>  #endif

For future reference, we should have a way to enable this for debugging without recompiling (at least in DEBUG builds; probably in opt builds too - and a user-invokable way as well).  File a bug

@@ +370,5 @@
>    // we treat as success if atleast one codec was applied and reception was
>    // started successfully.
>    for(unsigned int i=0 ; i < codecConfigList.size() && codecConfigList[i]; i++)
>    { 
> +    //if the codec param is invalid or diplicate, return error

duplicate

@@ +610,2 @@
>                                                webrtc::VideoCodec& cinst)
> + { 

no space before { (or } at the end)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +37,5 @@
>  /**
>   * Concrete class for Video session. Hooks up  
>   *  - media-source and target to external transport 
>   */
> +class WebrtcVideoConduit:public VideoSessionConduit      

spaces...

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +228,5 @@
>    return 0;
>  }
>  
> +//Code from xiph.org to generate music of predefined length
> +void AudioSendAndReceive::GenerateMusic(int16_t* buf, int len)

We can ignore spacing issues in this imported block

@@ +272,3 @@
>     int sampleLengthInBytes = PLAYOUT_SAMPLE_LENGTH * sizeof(short);
> +   //generated audio buffer
> +   inbuf = (short *)malloc(sizeof(short)*SAMPLES*2);

Sorry - please either check the result or use moz_xmalloc()

@@ +280,5 @@
> +   FILE* outFile   = fopen( oFile.c_str(), "wb+");
> +   //Create input file with the music
> +   WriteWaveHeader(PLAYOUT_SAMPLE_FREQUENCY, 1, inFile);
> +   GenerateMusic(inbuf, SAMPLES);
> +   fwrite(inbuf,1,SAMPLES*2*2,inFile);

pedantic: SAMPLES*sizeof(inbuf[0])*CHANNELS  (I assume the second 2 is channels)

Comment 31

[Suhas]

Attachment: Patch 5 MediaConduit Rollup, Final

Comment 32

[Ethan Hugg [:ehugg]]

Attachment: Patch 4 - Un-duplicate code in ui.c and ccapi.c

Comment 33

[Ethan Hugg [:ehugg]]

Comment on attachment 667128 [details] [diff] [review]
Patch 5 MediaConduit Rollup, Final


Patch 5 pushed to Alder - https://hg.mozilla.org/projects/alder/rev/9dc21cc0ac52

Comment 34

[Suhas]

Attachment: Patch 9 PeerConnection and Ecc Rollup V1

Comment 35

[enda mannion (:emannion)]

Attachment: Patch 8 - GSM rollup of SIPCC changes

Comment 36

[Ethan Hugg [:ehugg]]

Attachment: Patch 10 rollup fixes to core/sdp and core/ccapp

Comment 37

[Suhas]

Attachment: Patch 9 PeerConnection and Ecc Rollup V2

Comment 38

[Suhas]

Comment on attachment 667788 [details] [diff] [review]
Patch 9 PeerConnection and Ecc Rollup V2

Incorporated more changes on discussing with Ethan.

Comment 39

[Eric Rescorla (:ekr)]

Comment on attachment 667312 [details] [diff] [review]
Patch 4 - Un-duplicate code in ui.c and ccapi.c

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

lgtm

::: media/webrtc/signaling/src/sipcc/core/common/ui.c
@@ +70,5 @@
>  /*--------------------------------------------------------------------------
>   * Local definitions
>   *--------------------------------------------------------------------------
>   */
> +#define CCAPP_F_PREFIX "CCAPP : %s : " // requires 1 arg: __FUNCTION__

You don't actually need to change this one :)

@@ +712,1 @@
>      msg.update.ccFeatUpd.data.notification.priority = priority;

What the heck happened to ui_set_notification here?

Comment 40

[Ethan Hugg [:ehugg]]

Comment on attachment 667312 [details] [diff] [review]
Patch 4 - Un-duplicate code in ui.c and ccapi.c

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

::: media/webrtc/signaling/src/sipcc/core/common/ui.c
@@ +712,1 @@
>      msg.update.ccFeatUpd.data.notification.priority = priority;

Are you referring to the grayish line that tells you where you are in the file?  Better check the colors on your monitor.

Comment 41

[Eric Rescorla (:ekr)]

Comment on attachment 667788 [details] [diff] [review]
Patch 9 PeerConnection and Ecc Rollup V2

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

lgtm with the following changes.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +129,5 @@
>      // SIPCC is up
>      if (PeerConnectionImpl::kStarting == mSipccState ||
>          PeerConnectionImpl::kIdle == mSipccState) {
>        ChangeSipccState(PeerConnectionImpl::kStarted);
> +    } else {

Why are you calling Cleanup() here?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +159,5 @@
>                  } else if (hint == nsDOMMediaStream::HINT_CONTENTS_VIDEO) {
>                    mObserver->OnAddStream(stream, "video");
> +                } else if ((hint == nsDOMMediaStream::HINT_CONTENTS_AUDIO) &&
> +                                   (hint == nsDOMMediaStream::HINT_CONTENTS_VIDEO)){
> +                  CSFLogErrorS(logTag, __FUNCTION__ << "Audio & Video not supported");

This logic seems wrong. 

Since _AUDIO != _VIDEO, this third arm can never fire.

@@ +1204,2 @@
>  {
> +  if(aIndex < 0 || aIndex >= (int) mLocalSourceStreams.Length()) {

I would prefer C++  style casts here ad below.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +440,5 @@
>  	nsRefPtr<mozilla::DataChannelConnection> mDataConnection;
>  #endif
>  
>    // Singleton list of all the PeerConnections
> +  static std::map<const std::string, PeerConnectionImpl *> mPeerconnections;

This is not a member.

Comment 42

[Suhas]

Comment on attachment 667788 [details] [diff] [review]
Patch 9 PeerConnection and Ecc Rollup V2

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +129,5 @@
>      // SIPCC is up
>      if (PeerConnectionImpl::kStarting == mSipccState ||
>          PeerConnectionImpl::kIdle == mSipccState) {
>        ChangeSipccState(PeerConnectionImpl::kStarted);
> +    } else {

What should be our handling of incorrect state in this case .. i thought we shut down PC completely in the error scenario ..

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +159,5 @@
>                  } else if (hint == nsDOMMediaStream::HINT_CONTENTS_VIDEO) {
>                    mObserver->OnAddStream(stream, "video");
> +                } else if ((hint == nsDOMMediaStream::HINT_CONTENTS_AUDIO) &&
> +                                   (hint == nsDOMMediaStream::HINT_CONTENTS_VIDEO)){
> +                  CSFLogErrorS(logTag, __FUNCTION__ << "Audio & Video not supported");

you are right

@@ +1204,2 @@
>  {
> +  if(aIndex < 0 || aIndex >= (int) mLocalSourceStreams.Length()) {

sure ..will change

Comment 43

[Eric Rescorla (:ekr)]

(In reply to Suhas from comment #42)
> Comment on attachment 667788 [details] [diff] [review]
> Patch 9 PeerConnection and Ecc Rollup V2
> 
> Review of attachment 667788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
> @@ +129,5 @@
> >      // SIPCC is up
> >      if (PeerConnectionImpl::kStarting == mSipccState ||
> >          PeerConnectionImpl::kIdle == mSipccState) {
> >        ChangeSipccState(PeerConnectionImpl::kStarted);
> > +    } else {
> 
> What should be our handling of incorrect state in this case .. i thought we
> shut down PC completely in the error scenario ..

Yeah, I guess this os OK.


> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +159,5 @@
> >                  } else if (hint == nsDOMMediaStream::HINT_CONTENTS_VIDEO) {
> >                    mObserver->OnAddStream(stream, "video");
> > +                } else if ((hint == nsDOMMediaStream::HINT_CONTENTS_AUDIO) &&
> > +                                   (hint == nsDOMMediaStream::HINT_CONTENTS_VIDEO)){
> > +                  CSFLogErrorS(logTag, __FUNCTION__ << "Audio & Video not supported");
> 
> you are right
> 
> @@ +1204,2 @@
> >  {
> > +  if(aIndex < 0 || aIndex >= (int) mLocalSourceStreams.Length()) {
> 
> sure ..will change

Comment 44

[Eric Rescorla (:ekr)]

Comment on attachment 667640 [details] [diff] [review]
Patch 8 - GSM rollup of SIPCC changes

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

I'm finding this patch really hard to understand because of all the whitespace changes mixed with real changes.

Wy did the DTLS setup move? I'm concerned about a change like that at this time. Is there another way to accomplish that goal?

Comment 45

[Suhas]

Attachment: Patch 9 PeerConnection and Ecc Rollup V2

Comment 46

[Suhas]

Final Peer Connection and ECC rollup patch uploaded.

Comment 47

[Suhas]

Attachment: Patch 5 MediaConduit Rollup, Final

Comment 48

[Suhas]

Comment on attachment 667829 [details] [diff] [review]
Patch 5 MediaConduit Rollup, Final

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

Updated patch with Music Generation memory overwriting fix

Comment 49

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

Attachment: MediaPipeline changes for YUV buffer handling, put ImageContainer back to normal

Comment 50

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

Comment on attachment 667872 [details] [diff] [review]
MediaPipeline changes for YUV buffer handling, put ImageContainer back to normal

Verified with local_video_test that it works and no assertions fire

Comment 51

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

Comment on attachment 667064 [details] [diff] [review]
Patch 7 Rollup fix of cpr, src-common, misc and core/includes

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

::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c
@@ +265,2 @@
>  
> +#if 0

If there isn't already, file a bug on this

::: media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_socket.c
@@ +1114,5 @@
>  }
>  
>  void cpr_set_sockun_addr(cpr_sockaddr_un_t *addr, const char *name, pid_t pid)
>  {
> +  /* COPIED FROM OTHER PLATFOMR AS A PLACE HOLDER */

If you're bothering to fix indent, fix the spelling too :-)

Comment 52

[enda mannion (:emannion)]

Attachment: Patch 8 - GSM rollup of SIPCC changes, revised

I think moving gsmsdp_configure_dtls_data_attributes into the negotiation code is the right thing to do and it fixes the problem where we can fail negotiation if DTLS is not negotiated.  I debugged the vcmTxStartICE vcmRxStartICE to ensure we get the data.  I also revised this patch as I tested an offer without DTLS and now it correctly fails if this is the case.

Comment 53

[Ethan Hugg [:ehugg]]

Comment on attachment 667829 [details] [diff] [review]
Patch 5 MediaConduit Rollup, Final


We already pushed patch 5, so need these changes as a separate patch.

Comment 54

[Eric Rescorla (:ekr)]

Comment on attachment 667872 [details] [diff] [review]
MediaPipeline changes for YUV buffer handling, put ImageContainer back to normal

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

lgtm

Comment 55

[Ethan Hugg [:ehugg]]

Comment on attachment 667312 [details] [diff] [review]
Patch 4 - Un-duplicate code in ui.c and ccapi.c


Patch 4 pushed to Alder - http://hg.mozilla.org/projects/alder/rev/be94786366d7

Comment 56

[enda mannion (:emannion)]

Attachment: Patch 8 - GSM rollup of SIPCC changes, revised

More comments to explain ICE and DTLS negotiation.

Comment 57

[Suhas]

Attachment: Patch 11 MediaConduitTest Fix

Comment 58

[Ethan Hugg [:ehugg]]

Comment on attachment 667064 [details] [diff] [review]
Patch 7 Rollup fix of cpr, src-common, misc and core/includes

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

::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c
@@ +265,2 @@
>  
> +#if 0

bug 740254 should be the bug for this.

Comment 59

[Ethan Hugg [:ehugg]]

Comment on attachment 667064 [details] [diff] [review]
Patch 7 Rollup fix of cpr, src-common, misc and core/includes


Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/3def6825961f

Comment 60

[Ethan Hugg [:ehugg]]

Attachment: Patch 9 PeerConnection and Ecc Rollup V2

Comment 61

[Ethan Hugg [:ehugg]]

Comment on attachment 668018 [details] [diff] [review]
Patch 9 PeerConnection and Ecc Rollup V2


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/f20d554ff2e6

Comment 62

[Ethan Hugg [:ehugg]]

Attachment: Patch 10 rollup fixes to vcmSIPCCBinding, core/sdp and core/ccapp

Comment 63

[Eric Rescorla (:ekr)]

Comment on attachment 668066 [details] [diff] [review]
Patch 10 rollup fixes to vcmSIPCCBinding, core/sdp and core/ccapp

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

lgtm with one nit

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +44,1 @@
>  #include "plstr.h"

PR_ASSERT -> MOZ_ASSERT

Comment 64

[Ethan Hugg [:ehugg]]

Attachment: Patch 10 rollup fixes to vcmSIPCCBinding, core/sdp and core/ccapp

Comment 65

[Ethan Hugg [:ehugg]]

Comment on attachment 668076 [details] [diff] [review]
Patch 10 rollup fixes to vcmSIPCCBinding, core/sdp and core/ccapp


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/d2887e2d8ddf

Comment 66

[Ethan Hugg [:ehugg]]

Comment on attachment 667978 [details] [diff] [review]
Patch 8 - GSM rollup of SIPCC changes, revised


Enda - this patch seems to consistently fail FullCallTrickle of the unittests on my Linux box, could you investigate?

Comment 67

[Ethan Hugg [:ehugg]]

Comment on attachment 667980 [details] [diff] [review]
Patch 11 MediaConduitTest Fix


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/e699103b8c77

Comment 68

[Eric Rescorla (:ekr)]

Attachment: Patch 12: Change MediaPipeline/SrtpFlow to match Jesup's comments

Comment 69

[Eric Rescorla (:ekr)]

Comment on attachment 668245 [details] [diff] [review]
Patch 12: Change MediaPipeline/SrtpFlow to match Jesup's comments

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

Adding derf because screwing with the SRTP code makes me paranoid.

Comment 70

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

Comment on attachment 668245 [details] [diff] [review]
Patch 12: Change MediaPipeline/SrtpFlow to match Jesup's comments

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +89,5 @@
>    }
>  }
>  
>  nsresult MediaPipeline::TransportReady(TransportFlow *flow) {
> +  bool rtcp = (flow == rtp_transport_.get()) ? false : true;

Remove .get()

@@ +94,5 @@
> +  State *state = rtcp ? &rtcp_state_ : &rtp_state_;
> +
> +  if (*state != MP_CONNECTING) {
> +    MOZ_MTLOG(PR_LOG_ERROR, "Transport ready for flow in wrong state:" <<
> +              (rtcp ? "rtcp" : "rtp"));    

trailing space

@@ +212,5 @@
>    return NS_OK;
>  }
>  
>  nsresult MediaPipeline::TransportFailed(TransportFlow *flow) {
> +  bool rtcp = (flow == rtp_transport_.get()) ? false : true;

remove .get()

@@ +380,5 @@
>  
>    if (main_thread_) {
>      main_thread_->Dispatch(WrapRunnable(
> +        stream_->GetStream(), &MediaStream::AddListener, listener_),
> +                           NS_DISPATCH_SYNC);

random indent

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +92,2 @@
>    // Separate class to allow ref counting
>    class PipelineTransport : public TransportInterface {

hard to see, but either protected should be left-justified, or the class following should be indented.  Minor

Comment 71

[Eric Rescorla (:ekr)]

Comment on attachment 667978 [details] [diff] [review]
Patch 8 - GSM rollup of SIPCC changes, revised

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

Would it be possible to add a unit test to verify that we get an error if the other side doesn't offer DTLS?

Other than that and the changes below, once you fix trickle ICE I think this is fine.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +5903,5 @@
>        if(!result)
>          return (CC_CAUSE_ERROR);
> +
> +      /* Set ICE parameters into ICE engine */
> +      if (candidate_ct > 0) {

You need to call this in any case because it might contain the ufrag and pwd.

@@ +6017,5 @@
> +                return CC_CAUSE_ERROR;
> +            }
> +
> +            /* Here we have DTLS data */
> +            cause = CC_CAUSE_OK;

Is there a way cause could get set to ! OK?

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +789,5 @@
>                  LSM_DEBUG(get_debug_string(LSM_DBG_INT1), dcb->call_id, 
>                            dcb->line, fname, "port closed", 
>                            media->src_port);
>  
> +                config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));

Just want to make sure that this gets filled in even if no config is set. I.e., that you can remove this assignment,

Comment 72

[Timothy B. Terriberry (:derf)]

Comment on attachment 668245 [details] [diff] [review]
Patch 12: Change MediaPipeline/SrtpFlow to match Jesup's comments

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

r+ with a few comments.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +89,5 @@
>    }
>  }
>  
>  nsresult MediaPipeline::TransportReady(TransportFlow *flow) {
> +  bool rtcp = (flow == rtp_transport_.get()) ? false : true;

Also, what is wrong with just (flow != rtp_transport_) instead of the ternary?

@@ +102,4 @@
>    nsresult res;
>  
>    MOZ_MTLOG(PR_LOG_DEBUG, "Transport ready for flow " << (rtcp ? "rtcp" : "rtp"));
> +  

Whitespace-only change.

@@ +121,5 @@
>    res = dtls->ExportKeyingMaterial(kDTLSExporterLabel, false, "",
>                                     srtp_block, sizeof(srtp_block));
> +  if (NS_FAILED(res)) {
> +    MOZ_MTLOG(PR_LOG_ERROR, "Failed to compute DTLS-SRTP keys. This is an error");
> +    MOZ_CRASH();

Do we really want to crash here? If so, do we need the rest of the code in this block? With gcc, at least, this calls abort(), which should be declared with __attribute__((__noreturn__)).

@@ +125,5 @@
> +    MOZ_CRASH();
> +    *state = MP_CLOSED;
> +    return res;
> +  }
> +  

Whitespace.

@@ +220,3 @@
>  
> +  MOZ_MTLOG(PR_LOG_DEBUG, "Transport closed for flow " << (rtcp ? "rtcp" : "rtp"));
> +  

Whitespace.

@@ +347,5 @@
>  bool MediaPipeline::IsRtp(const unsigned char *data, size_t len) {
>    if (len < 2)
>      return false;
>  
> +  // TODO([e-mail]): this needs updating in light of RFC5761 (rtcp-mux)

There is code for this in media/webrtc/trunk/src/modules/rtp_rtcp/source/rtp_utility.cc (probably moved slightly after the latest webrtc.org merge). See RTPHeaderParser::RTCP(). We should probably be consistent with what that does.

@@ +602,5 @@
>  nsresult MediaPipelineReceiveAudio::Init() {
>    MOZ_MTLOG(PR_LOG_DEBUG, __FUNCTION__);
>    if (main_thread_) {
>      main_thread_->Dispatch(WrapRunnable(
> +        stream_->GetStream(), &MediaStream::AddListener, listener_),

More random indent.

Comment 73

[enda mannion (:emannion)]

Attachment: Patch 8 - GSM rollup of SIPCC changes, revised

Adding a validation check on setting ice candidates broke the trickle ice test, fixed now.

Comment 74

[Ethan Hugg [:ehugg]]

Comment on attachment 668400 [details] [diff] [review]
Patch 8 - GSM rollup of SIPCC changes, revised


Patch 8 pushed to Alder - https://hg.mozilla.org/projects/alder/rev/eb0c52d34294

Comment 75

[Eric Rescorla (:ekr)]

Attachment: Revised mediapipeline. Merged and with jesup/derf comments

This is a hand-applied version of the previous patch accomodating comments from derf and jesup.

Comment 76

[Eric Rescorla (:ekr)]

Comment on attachment 668735 [details] [diff] [review]
Revised mediapipeline. Merged and with jesup/derf comments

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

This did not apply clean... Needs manual checking.

Comment 77

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

Comment on attachment 668735 [details] [diff] [review]
Revised mediapipeline. Merged and with jesup/derf comments

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

r+, but I'd like to see the RTCP change done and not have to worry about it later.  If not done, file a bug.  The others are minor, but if you're going in, address them please.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +123,5 @@
> +  if (NS_FAILED(res)) {
> +    MOZ_MTLOG(PR_LOG_ERROR, "Failed to compute DTLS-SRTP keys. This is an error");
> +    MOZ_CRASH();
> +    *state = MP_CLOSED;
> +    return res;

You didn't address derf's comment here.

 * MOZ_CRASH crashes the program, plain and simple, in a Breakpad-compatible
 * way, in both debug and release builds.

So dump the code after, or at least #if it out.
Or switch to MOZ_ASSERT(0,"Failed to compute DTLS-SRTP keys");

@@ +356,2 @@
>    if ((data[1] >= 200) && (data[1] <= 204))
>      return false;

Derf's comment wasn't addressed.  Try this:
  // check if this is a RTCP packet
  // should match rtp_utility.cc in media/webrtc/trunk/src
  switch (data[1]) {
    case 192:
      return false;
      break;
    case 193:
      // may be RTP
      return true;
      break;
    case 195:
    case 200:
    case 201:
    case 202:
    case 203:
    case 204:
    case 205:
    case 206:
    case 207:
      return false;
      break;
  }

@@ +621,5 @@
>    MOZ_MTLOG(PR_LOG_DEBUG, __FUNCTION__);
>    if (main_thread_) {
>      main_thread_->Dispatch(WrapRunnable(
> +        stream_->GetStream(), &MediaStream::AddListener, listener_),
> +                           NS_DISPATCH_SYNC);

Derf's comment wasn't addressed, but it's only a random indent.

Comment 78

[Eric Rescorla (:ekr)]

Attachment: derf/jesup comments on mediapipeline

Comment 79

[Ethan Hugg [:ehugg]]

Attachment: Patch 13 - Change how peerconnection handle is created

Comment 80

[Eric Rescorla (:ekr)]

Comment on attachment 668803 [details] [diff] [review]
Patch 13 - Change how peerconnection handle is created

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

lgtm

Comment 81

[Ethan Hugg [:ehugg]]

Comment on attachment 668803 [details] [diff] [review]
Patch 13 - Change how peerconnection handle is created


Patch 13 pushed to Alder - http://hg.mozilla.org/projects/alder/rev/df60314985a8

Comment 82

[Ethan Hugg [:ehugg]]

Comment on attachment 668803 [details] [diff] [review]
Patch 13 - Change how peerconnection handle is created


Pushed Path 13 to Alder - http://hg.mozilla.org/projects/alder/rev/df60314985a8

Comment 83

[Ethan Hugg [:ehugg]]

Attachment: Patch 13 - Change how peerconnection handle is created

Comment 84

[Ethan Hugg [:ehugg]]

Comment on attachment 668809 [details] [diff] [review]
Patch 13 - Change how peerconnection handle is created


Patched handle_bin size on Alder - http://hg.mozilla.org/projects/alder/rev/83bb17db05c6

Comment 85

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b40fe6bb9b90
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b08abdcc8a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de22a14e8c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c84f1661607
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bea9e9d2e2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b524d51e6b

Comment 86

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f1b524d51e6b
https://hg.mozilla.org/mozilla-central/rev/4bea9e9d2e2c
https://hg.mozilla.org/mozilla-central/rev/7c84f1661607
https://hg.mozilla.org/mozilla-central/rev/8de22a14e8c8
https://hg.mozilla.org/mozilla-central/rev/2b08abdcc8a9
https://hg.mozilla.org/mozilla-central/rev/b40fe6bb9b90

Comment 87

[Philip Chee]

Build failure Building with Visual Studio 2008SP1 (VC9)

c:/t1/hg/objdir-sm/mozilla/media/webrtc/signaling/signaling_sipcc/../../../../../../comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_platform_tcp.c(524) : error C2065: 'EWOULDBLOCK' : undeclared identifier
c:/t1/hg/objdir-sm/mozilla/media/webrtc/signaling/signaling_sipcc/../../../../../../comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_platform_tcp.c(524) : error C2065: 'EINPROGRESS' : undeclared identifier
c:/t1/hg/objdir-sm/mozilla/media/webrtc/signaling/signaling_sipcc/../../../../../../comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_platform_tcp.c(858) : error C2065: 'EWOULDBLOCK' : undeclared identifier
c:/t1/hg/objdir-sm/mozilla/media/webrtc/signaling/signaling_sipcc/../../../../../../comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_platform_tcp.c(860) : error C2065: 'ENOTCONN' : undeclared identifier
c:/t1/hg/objdir-sm/mozilla/media/webrtc/signaling/signaling_sipcc/../../../../../../comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_platform_tcp.c(918) : error C2065: 'EWOULDBLOCK' : undeclared identifier
c:/t1/hg/objdir-sm/mozilla/media/webrtc/signaling/signaling_sipcc/../../../../../../comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_platform_tcp.c(1079) : error C2065: 'EWOULDBLOCK' : undeclared identifier

Comment 88

[Philip Chee]

Putting the following in /media/webrtc/signaling/src/sipcc/cpr/include/cpr_errno.h allows my build to continue.

#ifndef EWOULDBLOCK
#define EWOULDBLOCK             WSAEWOULDBLOCK
#endif
#ifndef EINPROGRESS
#define EINPROGRESS             WSAEINPROGRESS
#endif
#ifndef ENOTCONN
#define ENOTCONN                WSAENOTCONN
#endif

For some reason putting this in:
/media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_errno.c
didn't work.

Comment 89

[Eric Rescorla (:ekr)]

Note that csi_platform.h has some #defines for this purpose, but as bsmith points out, they appear to create warnings in some version of VS. We should probably spec out where we need defns like this and where we don't.

Comment 90

[Philip Chee]

I've filed Bug 799076 for VS specific issues.

Comment 91

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

Comment on attachment 662277 [details] [diff] [review]
Rollup media/webrtc/signaling patch

Note that the actual rollup was generated from the result of splitting this patch up, reviewing the portions.  See the other activity here and the final landed patch.