Closed
Bug 792188
Opened 12 years ago
Closed 12 years ago
Import media/webrtc/signaling from alder to m-c
Categories
(Core :: WebRTC: Signaling, defect)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: jesup, Unassigned)
References
Details
(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])
Attachments
(18 files, 16 obsolete files)
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•12 years ago
|
||
This should have all the changesets that are part of the rollup patch for reference
Reporter | ||
Comment 2•12 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•12 years ago
|
Whiteboard: [WebRTC]
Comment 3•12 years ago
|
||
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•12 years ago
|
Whiteboard: [WebRTC] → [WebRTC], [blocking-webrtc+]
Comment 4•12 years ago
|
||
Comment 5•12 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•12 years ago
|
Attachment #665143 -
Flags: feedback?(rjesup) → feedback+
Comment 6•12 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•12 years ago
|
||
Comment 8•12 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•12 years ago
|
||
Comment 10•12 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 11•12 years ago
|
||
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 12•12 years ago
|
||
Comment on attachment 666100 [details] [diff] [review] Patch 3 - Change to MutexAutoLock Review of attachment 666100 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Updated•12 years ago
|
Attachment #666100 -
Flags: feedback?(ekr) → feedback+
Comment 13•12 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•12 years ago
|
||
Comment 15•12 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•12 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•12 years ago
|
||
Comment 18•12 years ago
|
||
Attachment #666622 -
Attachment is obsolete: true
Attachment #666624 -
Flags: review?(ethanhugg)
Comment 19•12 years ago
|
||
Attachment #666624 -
Attachment is obsolete: true
Attachment #666624 -
Flags: review?(ethanhugg)
Attachment #666775 -
Flags: review?(ethanhugg)
Comment 20•12 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•12 years ago
|
||
Comment 22•12 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 23•12 years ago
|
||
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•12 years ago
|
||
Updated•12 years ago
|
Attachment #666812 -
Attachment is obsolete: true
Comment 25•12 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•12 years ago
|
||
Attachment #666775 -
Attachment is obsolete: true
Attachment #666775 -
Flags: review?(ethanhugg)
Attachment #666887 -
Flags: review?(ethanhugg)
Comment 27•12 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.
Attachment #666887 -
Flags: review?(ekr)
Updated•12 years ago
|
Attachment #666887 -
Flags: review?(ethanhugg) → review+
Comment 28•12 years ago
|
||
Comment 29•12 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•12 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•12 years ago
|
||
Attachment #666887 -
Attachment is obsolete: true
Comment 32•12 years ago
|
||
Updated•12 years ago
|
Attachment #666609 -
Attachment is obsolete: true
Attachment #666609 -
Flags: feedback?(ekr)
Updated•12 years ago
|
Attachment #667312 -
Flags: feedback?(ekr)
Comment 33•12 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•12 years ago
|
||
Attachment #667622 -
Flags: review?(ethanhugg)
Comment 35•12 years ago
|
||
Attachment #667640 -
Flags: ui-review?(ekr)
Comment 36•12 years ago
|
||
Comment 37•12 years ago
|
||
Attachment #667622 -
Attachment is obsolete: true
Attachment #667622 -
Flags: review?(ethanhugg)
Comment 38•12 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)
Attachment #667788 -
Flags: review?(rjesup)
Comment 39•12 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]: ----------------------------------------------------------------- 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•12 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 41•12 years ago
|
||
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•12 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
Comment 43•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
Attachment #667788 -
Attachment is obsolete: true
Attachment #667788 -
Flags: review?(rjesup)
Attachment #667788 -
Flags: review?(ethanhugg)
Attachment #667819 -
Flags: review?(ethanhugg)
Comment 46•12 years ago
|
||
Final Peer Connection and ECC rollup patch uploaded.
Comment 47•12 years ago
|
||
Attachment #667128 -
Attachment is obsolete: true
Comment 48•12 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•12 years ago
|
||
Reporter | ||
Comment 50•12 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•12 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+
Comment 52•12 years ago
|
||
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•12 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 54•12 years ago
|
||
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•12 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
Comment 56•12 years ago
|
||
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•12 years ago
|
||
Attachment #667980 -
Flags: review?(ethanhugg)
Attachment #667829 -
Attachment is obsolete: true
Attachment #667829 -
Flags: review?(ethanhugg)
Attachment #667128 -
Attachment is obsolete: false
Comment 58•12 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•12 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•12 years ago
|
||
Updated•12 years ago
|
Attachment #667819 -
Attachment is obsolete: true
Attachment #667819 -
Flags: review?(ethanhugg)
Comment 61•12 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•12 years ago
|
||
Updated•12 years ago
|
Attachment #667663 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #668066 -
Flags: feedback?(ekr)
Comment 63•12 years ago
|
||
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•12 years ago
|
||
Updated•12 years ago
|
Attachment #668066 -
Attachment is obsolete: true
Comment 65•12 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•12 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•12 years ago
|
Attachment #667980 -
Flags: review?(ethanhugg) → review+
Comment 67•12 years ago
|
||
Comment on attachment 667980 [details] [diff] [review] Patch 11 MediaConduitTest Fix Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/e699103b8c77
Comment 68•12 years ago
|
||
Updated•12 years ago
|
Attachment #668245 -
Flags: review?(rjesup)
Comment 69•12 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]: ----------------------------------------------------------------- Adding derf because screwing with the SRTP code makes me paranoid.
Attachment #668245 -
Flags: review?(tterribe)
Reporter | ||
Comment 70•12 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 71•12 years ago
|
||
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•12 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]: ----------------------------------------------------------------- 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+
Comment 73•12 years ago
|
||
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•12 years ago
|
Attachment #668400 -
Flags: feedback?(ekr) → feedback+
Updated•12 years ago
|
Attachment #668400 -
Flags: feedback?(ethanhugg) → feedback+
Comment 74•12 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
Comment 75•12 years ago
|
||
This is a hand-applied version of the previous patch accomodating comments from derf and jesup.
Attachment #668735 -
Flags: review?
Updated•12 years ago
|
Attachment #668735 -
Flags: review? → review?(rjesup)
Comment 76•12 years ago
|
||
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•12 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+
Comment 78•12 years ago
|
||
Updated•12 years ago
|
Attachment #668735 -
Attachment is obsolete: true
Comment 79•12 years ago
|
||
Updated•12 years ago
|
Attachment #668803 -
Flags: feedback?(ekr)
Comment 80•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
Comment 84•12 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•12 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
Comment 86•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Comment 87•12 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•12 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.
Comment 89•12 years ago
|
||
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•12 years ago
|
||
I've filed Bug 799076 for VS specific issues.
Reporter | ||
Comment 91•12 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)
Updated•12 years ago
|
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•