If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla18

Status

()

Core
WebRTC: Signaling
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jesup, Unassigned)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])

Attachments

(18 attachments, 16 obsolete attachments)

2.14 MB, patch
ted
: review+
Details | Diff | Splinter Review
272.02 KB, text/plain
Details
9.91 KB, patch
jesup
: feedback+
Details | Diff | Splinter Review
153.34 KB, patch
Details | Diff | Splinter Review
23.36 KB, patch
Details | Diff | Splinter Review
3.30 KB, patch
Details | Diff | Splinter Review
13.48 KB, patch
jesup
: feedback+
Details | Diff | Splinter Review
129.14 KB, patch
Details | Diff | Splinter Review
109.50 KB, patch
Details | Diff | Splinter Review
4.11 KB, patch
ekr
: review+
Details | Diff | Splinter Review
3.05 KB, patch
ehugg
: review+
Details | Diff | Splinter Review
55.80 KB, patch
Details | Diff | Splinter Review
24.41 KB, patch
Details | Diff | Splinter Review
43.88 KB, patch
jesup
: review+
derf
: review+
Details | Diff | Splinter Review
58.07 KB, patch
ehugg
: feedback+
Details | Diff | Splinter Review
44.02 KB, patch
Details | Diff | Splinter Review
2.50 KB, patch
Details | Diff | Splinter Review
2.59 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 662277 [details] [diff] [review]
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...
Attachment #662277 - Flags: review?(ted.mielczarek)
Attachment #662277 - Flags: review?(rjesup)
Attachment #662277 - Flags: review?(ekr)
(Reporter)

Comment 1

5 years ago
Created attachment 662380 [details]
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
(Reporter)

Comment 2

5 years ago
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

Updated

5 years ago
Whiteboard: [WebRTC]
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.
Attachment #662277 - Flags: review?(ted.mielczarek) → review+

Updated

5 years ago
Whiteboard: [WebRTC] → [WebRTC], [blocking-webrtc+]

Comment 4

5 years ago
Created attachment 665143 [details] [diff] [review]
Patch 1 Cleanup for import of signaling code

Comment 5

5 years ago
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.
Attachment #665143 - Flags: feedback?(rjesup)
(Reporter)

Updated

5 years ago
Attachment #665143 - Flags: feedback?(rjesup) → feedback+

Comment 6

5 years ago
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

5 years ago
Created attachment 666056 [details] [diff] [review]
Patch 2 - removal of WebrtcMediaProvider

Comment 8

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


I was able to remove everything in the media/webrtc directory
Attachment #666056 - Flags: feedback?(ekr)

Comment 9

5 years ago
Created attachment 666100 [details] [diff] [review]
Patch 3 - Change to MutexAutoLock

Comment 10

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


Full removal of AutoLockNSPR.  Requires Patch 2 applied.
Attachment #666100 - Flags: feedback?(ekr)
Comment on attachment 666056 [details] [diff] [review]
Patch 2 - removal of WebrtcMediaProvider

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

lgtm
Attachment #666056 - Flags: feedback?(ekr) → feedback+
Comment on attachment 666100 [details] [diff] [review]
Patch 3 - Change to MutexAutoLock

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

lgtm

Updated

5 years ago
Attachment #666100 - Flags: feedback?(ekr) → feedback+

Comment 13

5 years ago
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

5 years ago
Created attachment 666609 [details] [diff] [review]
Patch 4 - Un-duplicate code in ui.c and ccapi.c

Comment 15

5 years ago
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__
Attachment #666609 - Flags: feedback?(ekr)

Comment 16

5 years ago
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

5 years ago
Created attachment 666622 [details] [diff] [review]
Patch 5

Comment 18

5 years ago
Created attachment 666624 [details] [diff] [review]
Patch 5 MediaConduit Rollup

Updated

5 years ago
Attachment #666622 - Attachment is obsolete: true

Updated

5 years ago
Attachment #666624 - Flags: review?(ethanhugg)

Comment 19

5 years ago
Created attachment 666775 [details] [diff] [review]
Patch 5 MediaConduit Rollup

Updated

5 years ago
Attachment #666624 - Attachment is obsolete: true
Attachment #666624 - Flags: review?(ethanhugg)

Updated

5 years ago
Attachment #666775 - Flags: review?(ethanhugg)

Comment 20

5 years ago
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

5 years ago
Created attachment 666812 [details] [diff] [review]
Patch 6 - Move Wrapper back to using its own autolock

Comment 22

5 years ago
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.
Attachment #666812 - Flags: feedback?(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
Attachment #666812 - Flags: feedback?(ekr) → feedback+

Comment 24

5 years ago
Created attachment 666847 [details] [diff] [review]
Patch 6 - Move Wrapper back to using its own autolock

Updated

5 years ago
Attachment #666812 - Attachment is obsolete: true

Comment 25

5 years ago
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

5 years ago
Created attachment 666887 [details] [diff] [review]
Patch 5 MediaConduit Rollup, UnitTests Music generation

Updated

5 years ago
Attachment #666775 - Attachment is obsolete: true
Attachment #666775 - Flags: review?(ethanhugg)

Updated

5 years ago
Attachment #666887 - Flags: review?(ethanhugg)

Comment 27

5 years ago
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.

Updated

5 years ago
Attachment #666887 - Flags: review?(ekr)

Updated

5 years ago
Attachment #666887 - Flags: review?(ethanhugg) → review+

Comment 28

5 years ago
Created attachment 667064 [details] [diff] [review]
Patch 7 Rollup fix of cpr, src-common, misc and core/includes

Comment 29

5 years ago
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.
Attachment #667064 - Flags: feedback?(rjesup)
(Reporter)

Comment 30

5 years ago
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)
Attachment #666887 - Flags: review?(ekr) → review+

Comment 31

5 years ago
Created attachment 667128 [details] [diff] [review]
Patch 5 MediaConduit Rollup, Final

Updated

5 years ago
Attachment #666887 - Attachment is obsolete: true

Comment 32

5 years ago
Created attachment 667312 [details] [diff] [review]
Patch 4 - Un-duplicate code in ui.c and ccapi.c

Updated

5 years ago
Attachment #666609 - Attachment is obsolete: true
Attachment #666609 - Flags: feedback?(ekr)

Updated

5 years ago
Attachment #667312 - Flags: feedback?(ekr)

Comment 33

5 years ago
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

5 years ago
Created attachment 667622 [details] [diff] [review]
Patch 9 PeerConnection and Ecc Rollup V1

Updated

5 years ago
Attachment #667622 - Flags: review?(ethanhugg)
Created attachment 667640 [details] [diff] [review]
Patch 8 - GSM rollup of SIPCC changes
Attachment #667640 - Flags: ui-review?(ekr)

Comment 36

5 years ago
Created attachment 667663 [details] [diff] [review]
Patch 10 rollup fixes to core/sdp and core/ccapp

Comment 37

5 years ago
Created attachment 667788 [details] [diff] [review]
Patch 9 PeerConnection and Ecc Rollup V2

Updated

5 years ago
Attachment #667622 - Attachment is obsolete: true
Attachment #667622 - Flags: review?(ethanhugg)

Comment 38

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

Incorporated more changes on discussing with Ethan.
Attachment #667788 - Flags: review?(ethanhugg)

Updated

5 years ago
Attachment #667788 - Flags: review?(rjesup)
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?
Attachment #667312 - Flags: feedback?(ekr) → feedback+

Comment 40

5 years ago
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 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.
Attachment #667788 - Flags: review+

Comment 42

5 years ago
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
(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 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

5 years ago
Created attachment 667819 [details] [diff] [review]
Patch 9 PeerConnection and Ecc Rollup V2

Updated

5 years ago
Attachment #667788 - Attachment is obsolete: true
Attachment #667788 - Flags: review?(rjesup)
Attachment #667788 - Flags: review?(ethanhugg)

Updated

5 years ago
Attachment #667819 - Flags: review?(ethanhugg)

Comment 46

5 years ago
Final Peer Connection and ECC rollup patch uploaded.

Comment 47

5 years ago
Created attachment 667829 [details] [diff] [review]
Patch 5 MediaConduit Rollup, Final

Updated

5 years ago
Attachment #667128 - Attachment is obsolete: true

Comment 48

5 years ago
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
Attachment #667829 - Flags: review?(ethanhugg)
(Reporter)

Comment 49

5 years ago
Created attachment 667872 [details] [diff] [review]
MediaPipeline changes for YUV buffer handling, put ImageContainer back to normal
(Reporter)

Comment 50

5 years ago
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
Attachment #667872 - Flags: review?(ekr)
(Reporter)

Comment 51

5 years ago
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 :-)
Attachment #667064 - Flags: feedback?(rjesup) → feedback+
Created attachment 667930 [details] [diff] [review]
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.
Attachment #667640 - Attachment is obsolete: true
Attachment #667640 - Flags: ui-review?(ekr)
Attachment #667930 - Flags: feedback?(ekr)

Comment 53

5 years ago
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 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
Attachment #667872 - Flags: review?(ekr) → review+

Comment 55

5 years ago
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
Created attachment 667978 [details] [diff] [review]
Patch 8 - GSM rollup of SIPCC changes, revised

More comments to explain ICE and DTLS negotiation.
Attachment #667930 - Attachment is obsolete: true
Attachment #667930 - Flags: feedback?(ekr)
Attachment #667978 - Flags: feedback?(ethanhugg)
Attachment #667978 - Flags: feedback?(ekr)

Comment 57

5 years ago
Created attachment 667980 [details] [diff] [review]
Patch 11 MediaConduitTest Fix

Updated

5 years ago
Attachment #667980 - Flags: review?(ethanhugg)

Updated

5 years ago
Attachment #667829 - Attachment is obsolete: true
Attachment #667829 - Flags: review?(ethanhugg)

Updated

5 years ago
Attachment #667128 - Attachment is obsolete: false

Comment 58

5 years ago
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

5 years ago
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

5 years ago
Created attachment 668018 [details] [diff] [review]
Patch 9 PeerConnection and Ecc Rollup V2

Updated

5 years ago
Attachment #667819 - Attachment is obsolete: true
Attachment #667819 - Flags: review?(ethanhugg)

Comment 61

5 years ago
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

5 years ago
Created attachment 668066 [details] [diff] [review]
Patch 10 rollup fixes to vcmSIPCCBinding, core/sdp and core/ccapp

Updated

5 years ago
Attachment #667663 - Attachment is obsolete: true

Updated

5 years ago
Attachment #668066 - Flags: feedback?(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
Attachment #668066 - Flags: feedback?(ekr) → feedback+

Comment 64

5 years ago
Created attachment 668076 [details] [diff] [review]
Patch 10 rollup fixes to vcmSIPCCBinding, core/sdp and core/ccapp

Updated

5 years ago
Attachment #668066 - Attachment is obsolete: true

Comment 65

5 years ago
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

5 years ago
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?

Updated

5 years ago
Attachment #667980 - Flags: review?(ethanhugg) → review+

Comment 67

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


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/e699103b8c77
Created attachment 668245 [details] [diff] [review]
Patch 12: Change MediaPipeline/SrtpFlow to match Jesup's comments

Updated

5 years ago
Attachment #668245 - Flags: review?(rjesup)
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.
Attachment #668245 - Flags: review?(tterribe)
(Reporter)

Comment 70

5 years ago
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
Attachment #668245 - Flags: review?(rjesup) → review+
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 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(ekr@rtfm.com): 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.
Attachment #668245 - Flags: review?(tterribe) → review+
Created attachment 668400 [details] [diff] [review]
Patch 8 - GSM rollup of SIPCC changes, revised

Adding a validation check on setting ice candidates broke the trickle ice test, fixed now.
Attachment #667978 - Attachment is obsolete: true
Attachment #667978 - Flags: feedback?(ethanhugg)
Attachment #667978 - Flags: feedback?(ekr)
Attachment #668400 - Flags: feedback?(ethanhugg)
Attachment #668400 - Flags: feedback?(ekr)

Updated

5 years ago
Attachment #668400 - Flags: feedback?(ekr) → feedback+

Updated

5 years ago
Attachment #668400 - Flags: feedback?(ethanhugg) → feedback+

Comment 74

5 years ago
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
Created attachment 668735 [details] [diff] [review]
Revised mediapipeline. Merged and with jesup/derf comments

This is a hand-applied version of the previous patch accomodating comments from derf and jesup.
Attachment #668735 - Flags: review?

Updated

5 years ago
Attachment #668735 - Flags: review? → review?(rjesup)
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.
(Reporter)

Comment 77

5 years ago
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.
Attachment #668735 - Flags: review?(rjesup) → review+
Created attachment 668788 [details] [diff] [review]
derf/jesup comments on mediapipeline

Updated

5 years ago
Attachment #668735 - Attachment is obsolete: true

Comment 79

5 years ago
Created attachment 668803 [details] [diff] [review]
Patch 13 - Change how peerconnection handle is created

Updated

5 years ago
Attachment #668803 - Flags: feedback?(ekr)
Comment on attachment 668803 [details] [diff] [review]
Patch 13 - Change how peerconnection handle is created

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

lgtm
Attachment #668803 - Flags: feedback?(ekr) → feedback+

Comment 81

5 years ago
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

5 years ago
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

5 years ago
Created attachment 668809 [details] [diff] [review]
Patch 13 - Change how peerconnection handle is created

Comment 84

5 years ago
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
(Reporter)

Comment 85

5 years ago
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

Updated

5 years ago
Depends on: 798926
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 87

5 years ago
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

5 years ago
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.

Updated

5 years ago
Depends on: 799076
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

5 years ago
I've filed Bug 799076 for VS specific issues.
(Reporter)

Comment 91

5 years ago
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.
Attachment #662277 - Flags: review?(rjesup)
Attachment #662277 - Flags: review?(ekr)
Blocks: 793959

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]

Updated

5 years ago
Duplicate of this bug: 769801
Blocks: 1001539
You need to log in before you can comment on or make changes to this bug.