1045967 - Allow WebRTC to be built as a standalone library.

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Randall Barker [:rbarker]]

It should be possible to build the WebRTC component so that it can be used in standalone application.

Comment 1

[Randall Barker [:rbarker]]

Attachment: part 1 - Library needed to build standalone WebRTC

Comment 2

[Randall Barker [:rbarker]]

Attachment: part 2 - Changes need to build WebRTC standalone

Comment 3

[Randall Barker [:rbarker]]

Attachment: part 1 - Library needed to build standalone WebRTC v2

Comment 4

[Randall Barker [:rbarker]]

Attachment: part 2 - Changes need to build WebRTC standalone v2

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8467391 [details] [diff] [review]
part 1 - Library needed to build standalone WebRTC  v2

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

Alright, first idea after reviewing part of this: let's split this up in parts.

a) Parts that are pure or near-pure copies of existing code to deal with INTERNAL_API issues and are really for "interfacing" existing code.
b) Parts that are legitimate new code to deal with the "standalone" part.

I think I'll want to look closely at which of (a) are really needed, and if we can get rid of some.

::: media/standalone/include/MediaASocketHandler.h
@@ +1,4 @@
> +#ifndef media_ASocketHandler_h
> +#define media_ASocketHandler_h
> +
> +#include "MediaErrorCodes.h" // nsresult

nsresult is defined in nsError.h

@@ +3,5 @@
> +
> +#include "MediaErrorCodes.h" // nsresult
> +#include "MediaRefCount.h"
> +#include "prio.h"  // PRFileDesc
> +#include "stdint.h" // UINT16_MAX

style nit: system headers preferably use <>

::: media/standalone/include/MediaAssertions.h
@@ +1,4 @@
> +#ifndef media_Assertions_h
> +#define media_Assertions_h
> +
> +#include "assert.h"

same

@@ +3,5 @@
> +
> +#include "assert.h"
> +
> +#if defined(DEBUG)
> +# define MEDIA_ASSERT(value, ...) assert(value)

Why does this exist? Why does it not forward to MOZ_ASSERT? Other parts of this code are using the latter.

::: media/standalone/include/MediaAutoPtr.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

We don't allow tabs, so most code sets tab-width to something really big to make them obvious. Setting them to 2 hides them.

@@ +10,5 @@
> +
> +namespace media {
> +
> +template <class T>
> +class AutoPtr {

Sad that we have to do our own implementation, but I see nsAutoPtr is, eh, "contaminated". Question: given that all your nsAutoPtr users seem to not require any of the black magic in the original implementation, and can in fact survive with this minimal implementation, would it be feasible to convert them to UniquePtr (which has deprecated nsAutoPtr)?

@@ +42,5 @@
> +    mRawPtr = 0;
> +    return temp;
> +  }
> +
> +#if 0

Get rid of this.

@@ +100,5 @@
> +    mRawPtr = 0;
> +    return temp;
> +  }
> +
> +#if 0

same

::: media/standalone/include/MediaCondVar.h
@@ +3,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/. */
> +
> +#ifndef media_CondVar_h

The original one already seems to have "#ifdef MOZILLA_INTERNAL_API" guards, so I'm surprised we need this?

::: media/standalone/include/MediaConvert.h
@@ +4,5 @@
> +#include "mozilla/RefPtr.h"
> +
> +namespace media {
> +template<typename T>
> +mozilla::TemporaryRef<T> Convert(mozilla::TemporaryRef<T> aPtr)

Why is this needed? I don't see any users? What does it even do?

::: media/standalone/include/MediaCountedPtr.h
@@ +5,5 @@
> +#include "mozilla/RefCountType.h"
> +
> +namespace media {
> +
> +template <class T> class CountedPtr

Why do we need this? RefPtr has INTERNAL_API guards, and you're using it above.

::: media/standalone/include/MediaReentrantMonitor.h
@@ +13,5 @@
> +#include "prmon.h"
> +
> +namespace media {
> +
> +class ReentrantMonitor

Same remark wrt INTERNAL_API guards.

::: media/standalone/include/MediaSharedBuffer.h
@@ +20,5 @@
> +  static mozilla::TemporaryRef<SharedBuffer> Create(size_t aSize)
> +  {
> +    // FIXME? This seems wrong. How does this memory get freed?
> +    void* m = malloc(sizeof(SharedBuffer) + aSize);
> +    mozilla::RefPtr<SharedBuffer> p = new (m) SharedBuffer();

Placement new and abusing the fact that we can delete malloced regions as we have our own mallocs?

Comment 6

[Randall Barker [:rbarker]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> Comment on attachment 8467391 [details] [diff] [review]
> part 1 - Library needed to build standalone WebRTC  v2
> 
> Review of attachment 8467391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Alright, first idea after reviewing part of this: let's split this up in
> parts.
> 
> a) Parts that are pure or near-pure copies of existing code to deal with
> INTERNAL_API issues and are really for "interfacing" existing code.
> b) Parts that are legitimate new code to deal with the "standalone" part.
> 
> I think I'll want to look closely at which of (a) are really needed, and if
> we can get rid of some.
> 
> ::: media/standalone/include/MediaASocketHandler.h
> @@ +1,4 @@
> > +#ifndef media_ASocketHandler_h
> > +#define media_ASocketHandler_h
> > +
> > +#include "MediaErrorCodes.h" // nsresult
> 
> nsresult is defined in nsError.h
> 

Since nsError.h is in xpcom/base I was hesitant to use it. nsError.h does currently look clean. I re-defined nsresult in MediaErrorCodes.h

> @@ +3,5 @@
> > +
> > +#include "MediaErrorCodes.h" // nsresult
> > +#include "MediaRefCount.h"
> > +#include "prio.h"  // PRFileDesc
> > +#include "stdint.h" // UINT16_MAX
> 
> style nit: system headers preferably use <>
> 
> ::: media/standalone/include/MediaAssertions.h
> @@ +1,4 @@
> > +#ifndef media_Assertions_h
> > +#define media_Assertions_h
> > +
> > +#include "assert.h"
> 
> same
> 
> @@ +3,5 @@
> > +
> > +#include "assert.h"
> > +
> > +#if defined(DEBUG)
> > +# define MEDIA_ASSERT(value, ...) assert(value)
> 
> Why does this exist? Why does it not forward to MOZ_ASSERT? Other parts of
> this code are using the latter.
> 

This may be vestigial code left from my previous attempts to get this to work. I'll try converting to use MOZ_ASSERT.

> ::: media/standalone/include/MediaAutoPtr.h
> @@ +1,1 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> We don't allow tabs, so most code sets tab-width to something really big to
> make them obvious. Setting them to 2 hides them.
> 

That was cruft inherited from whatever file I copied.

> @@ +10,5 @@
> > +
> > +namespace media {
> > +
> > +template <class T>
> > +class AutoPtr {
> 
> Sad that we have to do our own implementation, but I see nsAutoPtr is, eh,
> "contaminated". Question: given that all your nsAutoPtr users seem to not
> require any of the black magic in the original implementation, and can in
> fact survive with this minimal implementation, would it be feasible to
> convert them to UniquePtr (which has deprecated nsAutoPtr)?
> 

I'll give that a try. I would like to git rid of as much support code as possible.

> @@ +42,5 @@
> > +    mRawPtr = 0;
> > +    return temp;
> > +  }
> > +
> > +#if 0
> 
> Get rid of this.
> 
> @@ +100,5 @@
> > +    mRawPtr = 0;
> > +    return temp;
> > +  }
> > +
> > +#if 0
> 
> same
> 
> ::: media/standalone/include/MediaCondVar.h
> @@ +3,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/. */
> > +
> > +#ifndef media_CondVar_h
> 
> The original one already seems to have "#ifdef MOZILLA_INTERNAL_API" guards,
> so I'm surprised we need this?
> 

The problem is that it includes xpcom/glue/BlockingResourceBase.h

> ::: media/standalone/include/MediaConvert.h
> @@ +4,5 @@
> > +#include "mozilla/RefPtr.h"
> > +
> > +namespace media {
> > +template<typename T>
> > +mozilla::TemporaryRef<T> Convert(mozilla::TemporaryRef<T> aPtr)
> 
> Why is this needed? I don't see any users? What does it even do?
> 

More vestigial code. I'll remove it.

> ::: media/standalone/include/MediaCountedPtr.h
> @@ +5,5 @@
> > +#include "mozilla/RefCountType.h"
> > +
> > +namespace media {
> > +
> > +template <class T> class CountedPtr
> 
> Why do we need this? RefPtr has INTERNAL_API guards, and you're using it
> above.
> 

The counted pointer is need as a replacement for ipc/chromium/src/base/linked_ptr.h, I assumed we were using a linked pointer instead of a ref counted pointer for a reason so I created a drop in replacement that didn't require the chromium code.

> ::: media/standalone/include/MediaReentrantMonitor.h
> @@ +13,5 @@
> > +#include "prmon.h"
> > +
> > +namespace media {
> > +
> > +class ReentrantMonitor
> 
> Same remark wrt INTERNAL_API guards.
> 

Same reason as above, Replacing XPCOM functionality.

> ::: media/standalone/include/MediaSharedBuffer.h
> @@ +20,5 @@
> > +  static mozilla::TemporaryRef<SharedBuffer> Create(size_t aSize)
> > +  {
> > +    // FIXME? This seems wrong. How does this memory get freed?
> > +    void* m = malloc(sizeof(SharedBuffer) + aSize);
> > +    mozilla::RefPtr<SharedBuffer> p = new (m) SharedBuffer();
> 
> Placement new and abusing the fact that we can delete malloced regions as we
> have our own mallocs?

I don't think it is being used currently as I think it is used only by audio streaming. I figured it was "okay" since I lifted this from some where else, but I find it hard to believe there isn't a better way to do this.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

>I find it hard to believe there isn't a better way to do this.

It occurred to me that in the standalone build we might not have our own mallocs, in which case this is actually not legal...........

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8467391 [details] [diff] [review]
part 1 - Library needed to build standalone WebRTC  v2

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

::: media/standalone/include/MediaErrorCodes.h
@@ +47,5 @@
> +#else
> +#define MEDIA_NOTREACHED(_msg) do {} while(0)
> +#endif
> +
> +#define NS_IMETHOD nsresult

I'm pretty sure this actually needs virtual.

@@ +62,5 @@
> +#define NS_NOTREACHED(_msg) MEDIA_NOTREACHED(_msg)
> +
> +typedef uint32_t nsresult;
> +const nsresult NS_OK = 0;
> +const nsresult NS_BASE_STREAM_WOULD_BLOCK = 0x81000001; // Just made this up.

nsError.h doesn't look too contaminated. Let's not do this.

::: media/standalone/include/MediaMonitor.h
@@ +26,5 @@
> +{
> +public:
> +    Monitor(const char* aName) :
> +        mMutex(aName),
> +        mCondVar(mMutex, "[Monitor.mCondVar]")

Can't we reuse aName for something sensible here?

::: media/standalone/include/MediaMutex.h
@@ +39,5 @@
> +     **/
> +    Mutex(const char* name)
> +    {
> +        mLock = PR_NewLock();
> +// FIXME

What's the issue here?

@@ +76,5 @@
> +     * AssertCurrentThreadOwns
> +     * @see prlock.h
> +     **/
> +    void AssertCurrentThreadOwns () const
> +    {

Easy to implement via NSPR only: PR_ASSERT_CURRENT_THREAD_OWNS_LOCK(mLock);

@@ +84,5 @@
> +     * AssertNotCurrentThreadOwns
> +     * @see prlock.h
> +     **/
> +    void AssertNotCurrentThreadOwns () const
> +    {

Same.

::: media/standalone/include/MediaRefCount.h
@@ +24,5 @@
> +  }                                                           \
> +  MozRefCountType Release() {                                 \
> +    const MozRefCountType count = --mRefCount;                \
> +    if (count == 0) {                                         \
> +      mRefCount = 1; /* stabilize? */                         \

What's this needed for, given that we delete this?

::: media/standalone/include/MediaSegmentExternal.h
@@ +1,2 @@
> +#ifndef media_Segment_External_h
> +#define media_Segment_External_h

This seems to be both MediaSegments and AudioSampleFormat.h. The latter looks uncontaminated. At the very least I think we'd have to undo this "join" of the two files, and with minor tweaking maybe the latter can go away.

::: media/standalone/include/MediaTimer.h
@@ +35,5 @@
> +
> +  static void Initialize();
> +  static void Shutdown();
> +
> +//  nsresult Init(nsIObserver *aObserver, uint32_t aDelay, uint32_t aType);

Remove or comment why it's commented out.

::: media/standalone/include/MediaTypes.h
@@ +7,5 @@
> +
> +typedef int64_t MediaTime;
> +typedef MediaTime StreamTime;
> +typedef int32_t TrackRate;
> +//const TrackRate TRACK_RATE_MAX = 1 << MEDIA_TIME_FRAC_BITS;

Same remark.

::: media/standalone/include/PeerConnectionImplEnums.h
@@ +1,1 @@
> +/* THIS FILE IS AUTOGENERATED - DO NOT EDIT */

Should this be included? The original isn't in our source tree either.

::: media/standalone/moz.build
@@ +3,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/.
> +
> +SOURCES += [

Not UNIFIED_SOURCES?

@@ +4,5 @@
> +# 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/.
> +
> +SOURCES += [
> +    './src/MediaDNS.cpp',

Why the ./?

::: media/standalone/src/MediaDNS.cpp
@@ +9,5 @@
> +
> +#include <string>
> +
> +namespace {
> +mozilla::RefPtr<media::DNS> sDNS;

Why is this a RefPtr and not an AutoPtr/ScopedPtr/UnqiuePtr? See also below.

@@ +123,5 @@
> +  }
> +
> +  mozilla::RefPtr<DNS> result = sDNS;
> +
> +  return result.forget();

I'm not sure what .forget() is doing here since it's returning a ref to a reference counted pointer where there's a global holding a ref anyway. There's no transfer-of-ownership here. (This construction comes back a few times, maybe it's related to getting that TemporaryRef result?)

::: media/standalone/src/MediaSegmentExternal.cpp
@@ +2,5 @@
> +
> +namespace media {
> +
> +AudioSegment::ChunkIterator::ChunkIterator(AudioSegment&)
> +{

This is enough? I'd put ASSERT's here so it blows up if anything ever ends up using it, instead of failing silently.

::: media/standalone/src/MediaSocketTransportServiceImpl.cpp
@@ +17,5 @@
> +  , mIdleListSize(SOCKET_LIMIT_MIN)
> +  , mActiveCount(0)
> +  , mIdleCount(0)
> +{
> +  mActiveList = new SocketContext[mActiveListSize];

Why aren't these nsTArrays or something similar? Contamination? std::vector then?

@@ +193,5 @@
> +
> +  Thread *thread = NS_GetCurrentThread();
> +
> +  // make sure the pseudo random number generator is seeded on this thread
> +  srand(static_cast<unsigned>(PR_Now()));

Why is this important? Who's the user?

@@ +233,5 @@
> +
> +nsresult
> +SocketTransportServiceImpl::DoPollIteration(bool wait)
> +{
> +  int32_t i, count;

You don't require i to survive the loops, so move this definition to the for's.

@@ +270,5 @@
> +      if (n > 0 && desc.out_flags != 0) {
> +        s.mElapsedTime = 0;
> +        s.mHandler->OnSocketReady(desc.fd, desc.out_flags);
> +      }
> +#if 0

Why?

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8467391 [details] [diff] [review]
part 1 - Library needed to build standalone WebRTC  v2

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

::: media/standalone/include/MediaDNS.h
@@ +13,5 @@
> +
> +class DNSRecord {
> +MEDIA_REF_COUNT_INLINE
> +public:
> +  DNSRecord(PRAddrInfo* info) : mInfo(info), mEnumPtr(nullptr) {}

#include "NullPtr.h"

::: media/standalone/include/MediaThread.h
@@ +39,5 @@
> +  nsresult InitializeCurrentThread();
> +  nsresult Join();
> +  nsresult Shutdown();
> +  nsresult HasPendingEvents(bool* retval);
> +  nsresult ProcessNextEvent(bool mayWait, bool* retval = nullptr);

#include "NullPtr.h"

::: media/standalone/src/MediaThread.cpp
@@ +74,5 @@
> +  }
> +
> +  bool IsPending()
> +  {
> +    return mSyncTask != nullptr;

#include "NullPtr.h"

@@ +180,5 @@
> +Thread::WorkFunc(void* data)
> +{
> +  Thread* self = static_cast<Thread*>(data);
> +  State* mState = self ? self->mState : nullptr;
> +  if (!mState) { return; }

Can you hit this? You crash if allocating the var OOMed, and you don't null it in the destructor.

@@ +187,5 @@
> +  while (!mState->mShutdown) { self->ProcessNextEvent(true); }
> +  ThreadManager::RemoveThread(self);
> +}
> +
> +Thread::Thread() : mState(nullptr)

Dead code ;-)

@@ +322,5 @@
> +
> +      if (mayWait) {
> +         mon.Wait();
> +      }
> +      else {

Our coding style has them on the same line (this happens multiple times in this code).

::: media/standalone/src/MediaTimer.cpp
@@ +37,5 @@
> +  ~TimerEntry()
> +  {
> +  }
> +
> +//  MozRefCountType AddRef() { return ++mRefCount; }

Remove this.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8467394 [details] [diff] [review]
part 2 - Changes need to build WebRTC standalone v2

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

This will need a WebRTC peer to review. Or even a few :P

::: media/mtransport/nomozinternal/moz.build
@@ +1,1 @@
> +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-

I guess we want to rename that dir, as you explained that the MOZ_INTERNAL_API thing is irrelevant anyway. some variation of /standalone/?

::: media/mtransport/nr_socket_prsock.cpp
@@ +124,5 @@
>  
>  // Implement the nsISupports ref counting
>  namespace mozilla {
>  
> +#if !defined(MOZILLA_MEDIA_STANDALONE)

Why do you need this gone?

@@ +288,5 @@
>  //XXX [e-mail]: copy from PRNetAddrToNetAddr,
>  // should be removed after fix the link error in signaling_unittests
>  static int praddr_to_netaddr(const PRNetAddr *prAddr, net::NetAddr *addr)
>  {
> +#if defined(MOZILLA_MEDIA_STANDALONE)

same question

::: media/mtransport/nricectx.cpp
@@ +48,5 @@
>  
>  #include "logging.h"
>  #include "nspr.h"
>  #include "nss.h"
> +#include "sechash.h"

Why do you need this? I see no relevant code change.

::: media/mtransport/nriceresolver.cpp
@@ +45,5 @@
>  #include "nspr.h"
>  #include "prnetdb.h"
>  
>  #include "mozilla/Assertions.h"
> +#include "mozilla/RefPtr.h"

Move this to the #ifdef STANDALONE?

@@ +75,5 @@
>  #include "mtransport/runnable_utils.h"
>  
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +const char *
> +nsAutoCString(const char* value)

I think this probably belongs in the library part of the patches.

::: media/mtransport/nriceresolver.h
@@ +47,5 @@
>  #include <map>
>  #include <string>
>  #include "nspr.h"
>  #include "prnetdb.h"
> +#include "mozilla/RefPtr.h"

Same remark.

::: media/mtransport/rlogringbuffer.cpp
@@ +26,5 @@
>  #include "r_log.h"
>  }
>  
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +typedef media::MutexAutoLock OffTheBooksMutexAutoLock;

Move to library part?

::: media/mtransport/rlogringbuffer.h
@@ +60,5 @@
>  #include <vector>
>  
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +#include "MediaMutex.h"
> +typedef media::Mutex OffTheBooksMutex;

Same.

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +342,5 @@
>      nr_ice_media_stream *stream=cb_arg;
>      nr_ice_cand_pair *pair;
>      int timer_val;
>  
> +// FIXME MOZILLA_MEDIA_STANDALONE

You'll need followup bugs here.

::: media/mtransport/transportflow.h
@@ +57,5 @@
>  namespace mozilla {
>  
> +class TransportFlow :
> +#if !defined(MOZILLA_MEDIA_STANDALONE)
> +                      public nsISupports,

Somewhat offtopic: I wonder if this kind of thing couldn't make a few more "standard" utility classes reusable.

::: media/mtransport/transportlayerdtls.h
@@ +28,2 @@
>  #include "m_cpp_utils.h"
> +#include "nss/hasht.h"

Why?

::: media/webrtc/moz.build
@@ +223,4 @@
>          GYP_DIRS['signalingtest'].non_unified_sources += signaling_non_unified_sources
>          GYP_DIRS['signalingtest'].non_unified_sources += signaling_non_unified_sources_2
>  
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':

Why is FirefoxOS excluded here?

::: media/webrtc/signaling/include/SharedPtr.h
@@ +12,4 @@
>  #include "base/linked_ptr.h"
>  #include "nsAutoPtr.h"
> +#endif
> +#include "mozilla/RefPtr.h"

Move to inside the #ifdef

@@ +14,5 @@
> +#endif
> +#include "mozilla/RefPtr.h"
> +
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +#define LINKED_PTR media::CountedPtr

For what it's worth, I would expect the Chrome classes to not have XPCOM contamination, so not sure why this and scoped_ptr were needed.

::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp
@@ +198,5 @@
>  #ifdef MOZILLA_INTERNAL_API
>    GetWebRtcLogPrefs(&trace_mask, &log_file, &aec_log_dir, &multi_log);
>  #endif
>    CheckOverrides(&trace_mask, &log_file, &multi_log);
>    ConfigWebRtcLog(trace_mask, log_file, aec_log_dir, multi_log);

Maybe you'll have to add a way to configure logging in your standalone version? WebRTC logging is probably gonna be pretty useful.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "CSFLog.h"
>  #include "nspr.h"
> +#include <string.h>

Why isn't it in the #ifdef?

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.h
@@ +19,4 @@
>  class nsIThread;
>  class nsIEventTarget;
>  class nsIPrefBranch;
> +#include "nsCOMPtr.h"

Should be unneeded.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +48,5 @@
>  #endif
>  #include "mozilla/gfx/Point.h"
>  #include "mozilla/gfx/Types.h"
>  
> +#include "prinrval.h"

Same remark.

@@ +1362,3 @@
>      nsRefPtr<SharedBuffer> samples = SharedBuffer::Create(AUDIO_SAMPLE_BUFFER_MAX);
> +#else
> +    mozilla::RefPtr<SharedBuffer> samples = media::CreateSharedBuffer(AUDIO_SAMPLE_BUFFER_MAX);

I'm surprised, didn't you have a macro that aliases nsRefPtr to RefPtr?

@@ +1389,5 @@
>      MOZ_MTLOG(ML_DEBUG, "Audio conduit returned buffer of length "
>                << samples_length);
>  
>      AudioSegment segment;
> +#if !defined(MOZILLA_MEDIA_STANDALONE)

Didn't want that audio anyway? This surely can't just go, can it?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +135,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::dom;
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +using namespace sipcc;

Sure you need this one?

@@ +1853,5 @@
>  
>    // Forget the reference so that we can transfer it to
>    // SelfDestruct().
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +  mMedia.forget().drop()->SelfDestruct();

Can we add a "take" to whatever mMedia is?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +59,5 @@
>  class AFakePCObserver;
>  #endif
>  }
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +#include "PeerConnectionImplEnums.h"

Move the part below to a separate header.

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
@@ +20,5 @@
>  #include "ccapi_call_listener.h"
>  #include "config_api.h"
>  }
>  
> +#include <string>

Why.

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.h
@@ +23,4 @@
>  #include "mozilla/Mutex.h"
> +#endif
> +
> +#include <string>

Why.

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallServerInfo.cpp
@@ +11,5 @@
>  {
>  #include "ccapi_device_info.h"
>  }
>  
> +#include <string>

etc

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCFeatureInfo.cpp
@@ +15,5 @@
>  #include "ccapi_feature_info.h"
>  }
>  
> +#include <string>
> +

etc

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8467394 [details] [diff] [review]
part 2 - Changes need to build WebRTC standalone v2

I don't think we need a full review from a WebRTC peer yet, but if you have some free time (hahaha!), having a quick look at what's coming might be useful.

Comment 12

[Randall Barker [:rbarker]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> Comment on attachment 8467394 [details] [diff] [review] 
> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +124,5 @@
> >  
> >  // Implement the nsISupports ref counting
> >  namespace mozilla {
> >  
> > +#if !defined(MOZILLA_MEDIA_STANDALONE)
> 
> Why do you need this gone?
> 

It uses mozilla::TimeStamp which includes nscore.h. Since the code calling it (WebrtcGlobalInformation.cpp) is not compiled into standalone, it was easier to ifdef it out than create a replacement for TimeStamp that wouldn't even be used.

> @@ +288,5 @@
> >  //XXX [e-mail]: copy from PRNetAddrToNetAddr,
> >  // should be removed after fix the link error in signaling_unittests
> >  static int praddr_to_netaddr(const PRNetAddr *prAddr, net::NetAddr *addr)
> >  {
> > +#if defined(MOZILLA_MEDIA_STANDALONE)
> 
> same question
> 

net::NetAddr is part of xpcom so for external build, net::NetAddr is the same type as PRNetAddr so no conversion is needed.

> ::: media/mtransport/nricectx.cpp
> @@ +48,5 @@
> >  
> >  #include "logging.h"
> >  #include "nspr.h"
> >  #include "nss.h"
> > +#include "sechash.h"
> 
> Why do you need this? I see no relevant code change.
> 

Without it, the standalone build gets:

/Volumes/fennec/gecko-desktop/media/mtransport/nricectx.cpp:185:9: error: unknown type name 'SECHashObject'
  const SECHashObject *ho = HASH_GetHashObject(HASH_AlgMD5);
        ^
/Volumes/fennec/gecko-desktop/media/mtransport/nricectx.cpp:185:48: error: use of undeclared identifier 'HASH_AlgMD5'
  const SECHashObject *ho = HASH_GetHashObject(HASH_AlgMD5);

My guess is that it is included by some file in xpcom that is not included for standalone builds. I could put guards around it but I figured it was safe.

> ::: media/mtransport/nriceresolver.cpp
> @@ +45,5 @@
> >  #include "nspr.h"
> >  #include "prnetdb.h"
> >  
> >  #include "mozilla/Assertions.h"
> > +#include "mozilla/RefPtr.h"
> 
> Move this to the #ifdef STANDALONE?
> 

Why? There are RefPtrs used in the file.

> @@ +75,5 @@
> >  #include "mtransport/runnable_utils.h"
> >  
> > +#if defined(MOZILLA_MEDIA_STANDALONE)
> > +const char *
> > +nsAutoCString(const char* value)
> 
> I think this probably belongs in the library part of the patches.
> 

Yes.

> ::: media/mtransport/nriceresolver.h
> @@ +47,5 @@
> >  #include <map>
> >  #include <string>
> >  #include "nspr.h"
> >  #include "prnetdb.h"
> > +#include "mozilla/RefPtr.h"
> 
> Same remark.
> 

Actually, this is cruft, I just removed it.

> ::: media/mtransport/rlogringbuffer.cpp
> @@ +26,5 @@
> >  #include "r_log.h"
> >  }
> >  
> > +#if defined(MOZILLA_MEDIA_STANDALONE)
> > +typedef media::MutexAutoLock OffTheBooksMutexAutoLock;
> 
> Move to library part?
> 

Yep.

> ::: media/mtransport/rlogringbuffer.h
> @@ +60,5 @@
> >  #include <vector>
> >  
> > +#if defined(MOZILLA_MEDIA_STANDALONE)
> > +#include "MediaMutex.h"
> > +typedef media::Mutex OffTheBooksMutex;
> 
> Same.
> 

Yep.

> ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
> @@ +342,5 @@
> >      nr_ice_media_stream *stream=cb_arg;
> >      nr_ice_cand_pair *pair;
> >      int timer_val;
> >  
> > +// FIXME MOZILLA_MEDIA_STANDALONE
> 
> You'll need followup bugs here.
> 

I'll need to do some testing but this may not be necessary any longer, I had some timer bugs that may have been causing this. 

> ::: media/mtransport/transportflow.h
> @@ +57,5 @@
> >  namespace mozilla {
> >  
> > +class TransportFlow :
> > +#if !defined(MOZILLA_MEDIA_STANDALONE)
> > +                      public nsISupports,
> 
> Somewhat offtopic: I wonder if this kind of thing couldn't make a few more
> "standard" utility classes reusable.
> 
> ::: media/mtransport/transportlayerdtls.h
> @@ +28,2 @@
> >  #include "m_cpp_utils.h"
> > +#include "nss/hasht.h"
> 
> Why?
> 

/Volumes/fennec/gecko-desktop/media/mtransport/transportlayerdtls.h:68:42: error: use of undeclared identifier 'HASH_LENGTH_MAX'
  const static size_t kMaxDigestLength = HASH_LENGTH_MAX;
                                         ^
Which is defined in nss/hasht.h

> ::: media/webrtc/moz.build
> @@ +223,4 @@
> >          GYP_DIRS['signalingtest'].non_unified_sources += signaling_non_unified_sources
> >          GYP_DIRS['signalingtest'].non_unified_sources += signaling_non_unified_sources_2
> >  
> > +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':
> 
> Why is FirefoxOS excluded here?
> 

Same reason the test versions of the libraries aren't built for FxOS? I didn't see any utility in building the standalone libs for FxOS, It can be easily changed.

> ::: media/webrtc/signaling/include/SharedPtr.h
> @@ +12,4 @@
> >  #include "base/linked_ptr.h"
> >  #include "nsAutoPtr.h"
> > +#endif
> > +#include "mozilla/RefPtr.h"
> 
> Move to inside the #ifdef
> 

Done.

> @@ +14,5 @@
> > +#endif
> > +#include "mozilla/RefPtr.h"
> > +
> > +#if defined(MOZILLA_MEDIA_STANDALONE)
> > +#define LINKED_PTR media::CountedPtr
> 
> For what it's worth, I would expect the Chrome classes to not have XPCOM
> contamination, so not sure why this and scoped_ptr were needed.
> 

It is, basictypes.h contains nscore.h which is then included by *everything*.

> ::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp
> @@ +198,5 @@
> >  #ifdef MOZILLA_INTERNAL_API
> >    GetWebRtcLogPrefs(&trace_mask, &log_file, &aec_log_dir, &multi_log);
> >  #endif
> >    CheckOverrides(&trace_mask, &log_file, &multi_log);
> >    ConfigWebRtcLog(trace_mask, log_file, aec_log_dir, multi_log);
> 
> Maybe you'll have to add a way to configure logging in your standalone
> version? WebRTC logging is probably gonna be pretty useful.
> 

Logging to the console seems to work, I've used it to debug problems. You just can't use preferences. 

> ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
> @@ +3,5 @@
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #include "CSFLog.h"
> >  #include "nspr.h"
> > +#include <string.h>
> 
> Why isn't it in the #ifdef?
> 

More cruft, removed.

> ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.h
> @@ +19,4 @@
> >  class nsIThread;
> >  class nsIEventTarget;
> >  class nsIPrefBranch;
> > +#include "nsCOMPtr.h"
> 
> Should be unneeded.
> 

Yep, more cruft.

> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +48,5 @@
> >  #endif
> >  #include "mozilla/gfx/Point.h"
> >  #include "mozilla/gfx/Types.h"
> >  
> > +#include "prinrval.h"
> 
> Same remark.
> 

Removed.

> @@ +1362,3 @@
> >      nsRefPtr<SharedBuffer> samples = SharedBuffer::Create(AUDIO_SAMPLE_BUFFER_MAX);
> > +#else
> > +    mozilla::RefPtr<SharedBuffer> samples = media::CreateSharedBuffer(AUDIO_SAMPLE_BUFFER_MAX);
> 
> I'm surprised, didn't you have a macro that aliases nsRefPtr to RefPtr?
> 

Yeah, the ifdef isn't need, I think it is left over from an earlier port attempt.

> @@ +1389,5 @@
> >      MOZ_MTLOG(ML_DEBUG, "Audio conduit returned buffer of length "
> >                << samples_length);
> >  
> >      AudioSegment segment;
> > +#if !defined(MOZILLA_MEDIA_STANDALONE)
> 
> Didn't want that audio anyway? This surely can't just go, can it?
> 

We aren't supporting audio in standalone yet (nor data channels). When we add audio support, this will need to be fixed.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +135,5 @@
> >  
> >  using namespace mozilla;
> >  using namespace mozilla::dom;
> > +#if defined(MOZILLA_MEDIA_STANDALONE)
> > +using namespace sipcc;
> 
> Sure you need this one?
> 

I guess not. 

> @@ +1853,5 @@
> >  
> >    // Forget the reference so that we can transfer it to
> >    // SelfDestruct().
> > +#if defined(MOZILLA_MEDIA_STANDALONE)
> > +  mMedia.forget().drop()->SelfDestruct();
> 
> Can we add a "take" to whatever mMedia is?
> 

I tried not to make any changes to code outside of media. mMedia is a mozilla::RefPtr, I'm guessing they made it different for a reason?

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> @@ +59,5 @@
> >  class AFakePCObserver;
> >  #endif
> >  }
> > +#if defined(MOZILLA_MEDIA_STANDALONE)
> > +#include "PeerConnectionImplEnums.h"
> 
> Move the part below to a separate header.
> 

Done.

> ::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
> @@ +20,5 @@
> >  #include "ccapi_call_listener.h"
> >  #include "config_api.h"
> >  }
> >  
> > +#include <string>
> 
> Why.
> 

std::string is used in the file, but it doesn't look like they are needed anymore to compile standalone. Removed them all.

> ::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.h
> @@ +23,4 @@
> >  #include "mozilla/Mutex.h"
> > +#endif
> > +
> > +#include <string>
> 
> Why.
> 
> ::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCallServerInfo.cpp
> @@ +11,5 @@
> >  {
> >  #include "ccapi_device_info.h"
> >  }
> >  
> > +#include <string>
> 
> etc
> 
> ::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCFeatureInfo.cpp
> @@ +15,5 @@
> >  #include "ccapi_feature_info.h"
> >  }
> >  
> > +#include <string>
> > +
> 
> etc

Comment 13

[Randall Barker [:rbarker]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> Comment on attachment 8467394 [details] [diff] [review]
> part 2 - Changes need to build WebRTC standalone v2
> 
> Review of attachment 8467394 [details] [diff] [review]:
> ----------------------------------------------------------------- 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
> @@ +342,5 @@
> >      nr_ice_media_stream *stream=cb_arg;
> >      nr_ice_cand_pair *pair;
> >      int timer_val;
> >  
> > +// FIXME MOZILLA_MEDIA_STANDALONE
> 
> You'll need followup bugs here.
> 

I validated that I can still hit these asserts but it is internment. I'll uncomment them in the patch and filed: Bug 1055801

Comment 14

[Randall Barker [:rbarker]]

Attachment: part 1 - Library needed to build standalone WebRTC v3

Comment 15

[Randall Barker [:rbarker]]

Attachment: part 2 - Changes need to build WebRTC standalone v3

Comment 16

[Randall Barker [:rbarker]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)
> Comment on attachment 8467391 [details] [diff] [review]
> part 1 - Library needed to build standalone WebRTC  v2
> 
> Review of attachment 8467391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/standalone/include/MediaErrorCodes.h
> @@ +47,5 @@
> > +#else
> > +#define MEDIA_NOTREACHED(_msg) do {} while(0)
> > +#endif
> > +
> > +#define NS_IMETHOD nsresult
> 
> I'm pretty sure this actually needs virtual.
> 

It doesn't need to be, but it doesn't hurt, I'll add it.

> @@ +62,5 @@
> > +#define NS_NOTREACHED(_msg) MEDIA_NOTREACHED(_msg)
> > +
> > +typedef uint32_t nsresult;
> > +const nsresult NS_OK = 0;
> > +const nsresult NS_BASE_STREAM_WOULD_BLOCK = 0x81000001; // Just made this up.
> 
> nsError.h doesn't look too contaminated. Let's not do this.
> 

I would rather keep all xpcom header out of the standalone build.

> ::: media/standalone/include/MediaMonitor.h
> @@ +26,5 @@
> > +{
> > +public:
> > +    Monitor(const char* aName) :
> > +        mMutex(aName),
> > +        mCondVar(mMutex, "[Monitor.mCondVar]")
> 
> Can't we reuse aName for something sensible here?
> 

I'm not sure what you mean. This is taken from the xpcom implementation which does the same thing. See: xpcom/glue/Monitor.h

> ::: media/standalone/include/MediaMutex.h
> @@ +39,5 @@
> > +     **/
> > +    Mutex(const char* name)
> > +    {
> > +        mLock = PR_NewLock();
> > +// FIXME
> 
> What's the issue here?
> 

It was a problem, fixed.

> @@ +76,5 @@
> > +     * AssertCurrentThreadOwns
> > +     * @see prlock.h
> > +     **/
> > +    void AssertCurrentThreadOwns () const
> > +    {
> 
> Easy to implement via NSPR only: PR_ASSERT_CURRENT_THREAD_OWNS_LOCK(mLock);
> 

They are empty in the xpcom implementation I copied it from.

> @@ +84,5 @@
> > +     * AssertNotCurrentThreadOwns
> > +     * @see prlock.h
> > +     **/
> > +    void AssertNotCurrentThreadOwns () const
> > +    {
> 
> Same.
> 
> ::: media/standalone/include/MediaRefCount.h
> @@ +24,5 @@
> > +  }                                                           \
> > +  MozRefCountType Release() {                                 \
> > +    const MozRefCountType count = --mRefCount;                \
> > +    if (count == 0) {                                         \
> > +      mRefCount = 1; /* stabilize? */                         \
> 
> What's this needed for, given that we delete this?
> 

I have no idea what it is needed for. I ask around and looked at blame and never got a satisfactory answer so I left it.

> ::: media/standalone/include/MediaSegmentExternal.h
> @@ +1,2 @@
> > +#ifndef media_Segment_External_h
> > +#define media_Segment_External_h
> 
> This seems to be both MediaSegments and AudioSampleFormat.h. The latter
> looks uncontaminated. At the very least I think we'd have to undo this
> "join" of the two files, and with minor tweaking maybe the latter can go
> away.
> 

done.

> ::: media/standalone/include/MediaTimer.h
> @@ +35,5 @@
> > +
> > +  static void Initialize();
> > +  static void Shutdown();
> > +
> > +//  nsresult Init(nsIObserver *aObserver, uint32_t aDelay, uint32_t aType);
> 
> Remove or comment why it's commented out.
> 

Removed

> ::: media/standalone/include/MediaTypes.h
> @@ +7,5 @@
> > +
> > +typedef int64_t MediaTime;
> > +typedef MediaTime StreamTime;
> > +typedef int32_t TrackRate;
> > +//const TrackRate TRACK_RATE_MAX = 1 << MEDIA_TIME_FRAC_BITS;
> 
> Same remark.
> 

Removed.

> ::: media/standalone/include/PeerConnectionImplEnums.h
> @@ +1,1 @@
> > +/* THIS FILE IS AUTOGENERATED - DO NOT EDIT */
> 
> Should this be included? The original isn't in our source tree either.
> 

Unfortunately the generated file is contaminated and can't be included. I'm open to other solutions but this was all I could come up with.

> ::: media/standalone/moz.build
> @@ +3,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/.
> > +
> > +SOURCES += [
> 
> Not UNIFIED_SOURCES?
> 

Fixed.

> @@ +4,5 @@
> > +# 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/.
> > +
> > +SOURCES += [
> > +    './src/MediaDNS.cpp',
> 
> Why the ./?
> 

Removed.

> ::: media/standalone/src/MediaDNS.cpp
> @@ +9,5 @@
> > +
> > +#include <string>
> > +
> > +namespace {
> > +mozilla::RefPtr<media::DNS> sDNS;
> 
> Why is this a RefPtr and not an AutoPtr/ScopedPtr/UnqiuePtr? See also below.
> 

I order for it to be API compatible with the code it is replacing, it needs to be returned as a ref counted object.

> @@ +123,5 @@
> > +  }
> > +
> > +  mozilla::RefPtr<DNS> result = sDNS;
> > +
> > +  return result.forget();
> 
> I'm not sure what .forget() is doing here since it's returning a ref to a
> reference counted pointer where there's a global holding a ref anyway.
> There's no transfer-of-ownership here. (This construction comes back a few
> times, maybe it's related to getting that TemporaryRef result?)
> 

The forget turns it into a TemporaryRef which reduces ref count churn.

> ::: media/standalone/src/MediaSegmentExternal.cpp
> @@ +2,5 @@
> > +
> > +namespace media {
> > +
> > +AudioSegment::ChunkIterator::ChunkIterator(AudioSegment&)
> > +{
> 
> This is enough? I'd put ASSERT's here so it blows up if anything ever ends
> up using it, instead of failing silently.
> 
> ::: media/standalone/src/MediaSocketTransportServiceImpl.cpp
> @@ +17,5 @@
> > +  , mIdleListSize(SOCKET_LIMIT_MIN)
> > +  , mActiveCount(0)
> > +  , mIdleCount(0)
> > +{
> > +  mActiveList = new SocketContext[mActiveListSize];
> 
> Why aren't these nsTArrays or something similar? Contamination? std::vector
> then?
>

The code this came from used new allocated arrays.
 
> @@ +193,5 @@
> > +
> > +  Thread *thread = NS_GetCurrentThread();
> > +
> > +  // make sure the pseudo random number generator is seeded on this thread
> > +  srand(static_cast<unsigned>(PR_Now()));
> 
> Why is this important? Who's the user?
> 

I have no idea, it was in the original code I copied. Maybe not needed but since it is in the original code I figured leaving it wouldn't hurt.

> @@ +233,5 @@
> > +
> > +nsresult
> > +SocketTransportServiceImpl::DoPollIteration(bool wait)
> > +{
> > +  int32_t i, count;
> 
> You don't require i to survive the loops, so move this definition to the
> for's.
> 

This also comes from the code I copied. I don't think it is worth diverging from it unless necessary.

> @@ +270,5 @@
> > +      if (n > 0 && desc.out_flags != 0) {
> > +        s.mElapsedTime = 0;
> > +        s.mHandler->OnSocketReady(desc.fd, desc.out_flags);
> > +      }
> > +#if 0
> 
> Why?

I removed it because it relied on functionality I had to remove to get things working.

Comment 17

[Randall Barker [:rbarker]]

Attachment: part 1 - Library needed to build standalone WebRTC v4

Comment 18

[Randall Barker [:rbarker]]

Attachment: part 2 - Changes need to build WebRTC standalone v4

Comment 19

[Randall Barker [:rbarker]]

Attachment: part 1 - Library needed to build standalone WebRTC v5

Comment 20

[Randall Barker [:rbarker]]

Attachment: part 2 - Changes need to build WebRTC standalone v5

Comment 21

[Randall Barker [:rbarker]]

Comment on attachment 8488944 [details] [diff] [review]
part 1 - Library needed to build standalone WebRTC  v5

Updated to tip of m-c tree.

Comment 22

[Randall Barker [:rbarker]]

Comment on attachment 8488945 [details] [diff] [review]
part 2 - Changes need to build WebRTC standalone v5

Updated to tip of m-c tree.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8488944 [details] [diff] [review]
part 1 - Library needed to build standalone WebRTC  v5

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

I can live with this. We probably need to file a followup for that auto-generated file, though.

::: media/standalone/include/MediaASocketHandler.h
@@ +1,4 @@
> +#ifndef media_ASocketHandler_h
> +#define media_ASocketHandler_h
> +
> +#include "MediaErrorCodes.h" // nsresult

IIRC you said after the initial review that the original definition in nsError.h looked clean. So either that was wrong or this can be cleaned up.

::: media/standalone/include/MediaAudioSampleFormat.h
@@ +21,5 @@
> +{
> +  return (MediaTime(aMS) << MEDIA_TIME_FRAC_BITS)/1000;
> +}
> +
> +

spacing

@@ +27,5 @@
> +AudioSampleToFloat(float aValue)
> +{
> +  return aValue;
> +}
> +inline float

...or the lack thereof (some more occurrences throughout the file)

::: media/standalone/include/MediaDNS.h
@@ +2,5 @@
> +#define media_DNS_h
> +
> +#include "MediaErrorCodes.h"
> +#include "MediaRefCount.h"
> +#include "mozilla/NullPtr.h"

FWIW you use 0 in a few of the previous files as nullptr. If we're willing to include mozilla/NullPtr.h, it's probably safe to just use proper "nullptr" consistently.

::: media/standalone/include/MediaErrorCodes.h
@@ +1,2 @@
> +#ifndef media_Error_Codes_h
> +#define media_Error_Codes_h

We went back and forth over whether to use nsError.h. You agreed it was "clean", but didn't want to use it because it's in xpcom/. I'd strongly prefer we use it if it is clean, i.e. I care about that more than the file location.

Note that some parts of the original are auto-generated. On the other hand we're not likely to add dependencies on new modules so in theory that shouldn't bite us...

::: media/standalone/include/MediaMutex.h
@@ +73,5 @@
> +     * AssertCurrentThreadOwns
> +     * @see prlock.h
> +     **/
> +    void AssertCurrentThreadOwns () const
> +    {

The original implementation is only empty when not debugging. Mutexes might be used in standalone-only code so I would do the same here. (Gecko development will obviously catch shared-code bugs)

::: media/standalone/include/MediaScopedKey.h
@@ +44,5 @@
> +                                          CERTValidity,
> +                                          CERT_DestroyValidity)
> +}
> +
> +#endif // ifndef medai_Scoped_Key_h

typo

::: media/standalone/include/MediaTypes.h
@@ +7,5 @@
> +
> +typedef int64_t MediaTime;
> +typedef MediaTime StreamTime;
> +typedef int32_t TrackRate;
> +//const TrackRate TRACK_RATE_MAX = 1 << MEDIA_TIME_FRAC_BITS;

leftover

::: media/standalone/include/PeerConnectionImplEnums.h
@@ +1,1 @@
> +/* THIS FILE IS AUTOGENERATED - DO NOT EDIT */

Need to check or file a follow-up bug to automate this, perhaps by modifying the generator.

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8488945 [details] [diff] [review]
part 2 - Changes need to build WebRTC standalone v5

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

Looks good to me. Time to split it up among reviewer/peer boundaries. You'll want abc/ekr/jesup for this.

::: media/mtransport/databuffer.h
@@ +17,1 @@
>  #include <nsISupportsImpl.h>

I'd consider making a wrapping header (that has the ifdef) because this one is going to come back all the time.

::: media/mtransport/nomozinternal/moz.build
@@ +1,1 @@
> +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-

Do you still use this, or is this a leftover? I noticed the (already existing) /standalone is changed as well.

::: media/mtransport/nriceresolver.h
@@ +48,5 @@
>  #include <string>
>  #include "nspr.h"
>  #include "prnetdb.h"
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +//#include "mozilla/RefPtr.h"

cruft

::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp
@@ +198,5 @@
>  #ifdef MOZILLA_INTERNAL_API
>    GetWebRtcLogPrefs(&trace_mask, &log_file, &aec_log_dir, &multi_log);
>  #endif
>    CheckOverrides(&trace_mask, &log_file, &multi_log);
>    ConfigWebRtcLog(trace_mask, log_file, aec_log_dir, multi_log);

FWIW you said we don't have access to preferences but we might want to add our own things. Good logging may be crucial especially if we need this on a diversity of platforms. But that can be a follow-up.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +14,5 @@
>  #include "AudioConduit.h"
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +#include "MediaThread.h"
> +#include "MediaErrorCodes.h"
> +#include "string.h" // memcpy and memset

<>

::: media/webrtc/signaling/src/media/CSFAudioControlWrapper.cpp
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "CSFLog.h"
>  #include "CSFAudioControlWrapper.h"
> +#include <string>

cruft?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +83,5 @@
>    static void Callback(nsITimer* timer, void *arg) {
>      Fake_AudioGenerator* gen = static_cast<Fake_AudioGenerator*>(arg);
>  
> +#if defined(MOZILLA_MEDIA_STANDALONE)
> +    mozilla::RefPtr<media::SharedBuffer> samples = media::CreateSharedBuffer(1600 * sizeof(int16_t));

Based on the previous review comments this (the ifdef) is cruft.

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
@@ +14,5 @@
>  #include "CSFAudioTermination.h"
>  #include "CSFAudioControl.h"
>  // fsm.h picks this include up as a c header, causing trouble later
>  #include "mozilla/ArrayUtils.h"
> +#include "mozilla/mozalloc.h"

I don't see anything in this file that would require that. Is some header missing it instead?

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Attachment: Miscellaneous parts for standalone WebRTC. r=

Comment 26

[Gian-Carlo Pascutto [:gcp]]

Attachment: Non-XPCOM dependent Mutexes, CondVars, etc. r=

Comment 27

[Gian-Carlo Pascutto [:gcp]]

Attachment: Non-XPCOM dependent smart pointers. r=

Comment 28

[Gian-Carlo Pascutto [:gcp]]

Attachment: Non-XPCOM dependent Timers. r=

Comment 29

[Gian-Carlo Pascutto [:gcp]]

Attachment: Non-XPCOM Threads, Runnables & Thread Manager. r=

Comment 30

[Gian-Carlo Pascutto [:gcp]]

Attachment: Non-XPCOM dependent networking & PeerConnection. r=

Comment 31

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8491482 [details] [diff] [review]
Non-XPCOM dependent networking & PeerConnection. r=

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

Feel free to forward if there are more appropriate reviewers.

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 8488945 [details] [diff] [review]
part 2 - Changes need to build WebRTC standalone v5

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

::: media/mtransport/nomozinternal/moz.build
@@ +29,5 @@
> +    '/media/mtransport/third_party/nrappkit/src/util/libekr',
> +    '/media/standalone/include',
> +]
> +
> +if CONFIG['OS_TARGET'] in ['Darwin', 'DragonFly', 'FreeBSD', 'NetBSD', 'OpenBSD']:

All this DEFINES, LOCAL_INCLUDES modification, etc. should be shared in a common mozbuild file in media/mtransport.

Comment 33

[Nathan Froyd [:froydnj]]

Comment on attachment 8491478 [details] [diff] [review]
Non-XPCOM dependent smart pointers. r=

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

I feel like it must be possible to refactor the necessary bits of XPCOM headers to support the usecases here.

- The AutoPtr stuff ought to be straightforward, even if AutoPtr ought to be burned with fire.
- Similarly for the CounterPtr stuff.
- The reference counting stuff is a little harder, since a lot of the reference counting macros are intertwined with QueryInterface stuff...but something ought to be able to happen here.

I am willing to hear counterarguments.

Comment 34

[Nathan Froyd [:froydnj]]

Comment on attachment 8491476 [details] [diff] [review]
Non-XPCOM dependent Mutexes, CondVars, etc. r=

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

I suppose the reason that all this needs to be separated out is because the BlockingResourceBase-ness of all of our locking constructs wind up dragging in the deadlock detector?  BlockingResourceBase looks like it has some headers that could be fruitfully excised, things could be restructured a bit, and then I don't think we have a lot of other cruft in the locking constructs themselves.  So I don't think we really need to duplicate all this stuff.

Are there other issues I'm missing here?

Comment 35

[Nathan Froyd [:froydnj]]

Comment on attachment 8491481 [details] [diff] [review]
Non-XPCOM Threads, Runnables & Thread Manager. r=

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

I can't say "I don't like this" enough times.  Why can't we carefully bring in even the nsI* headers for runnables and threads, at the very least?

I'll try to do a more thorough review of this on Monday.

::: media/standalone/include/MediaRunnable.h
@@ +1,1 @@
> +#ifndef media_Runnable_h

All of these new files need license headers and modelines.

@@ +6,5 @@
> +
> +namespace media {
> +
> +class Runnable : public RefCountInterface {
> +MEDIA_REF_COUNT_INLINE

Indent this, please.

::: media/standalone/include/MediaSyncRunnable.h
@@ +31,5 @@
> + *
> + */
> +class SyncRunnable : public Runnable
> +{
> +NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SyncRunnable)

Indent this, please.

::: media/standalone/src/MediaThreadManager.cpp
@@ +76,5 @@
> +ThreadManager::RemoveThread(Thread* thread)
> +{
> +  if (sState && thread && (thread == GetCurrentThread())) {
> +    PR_SetThreadPrivate(sState->currentThreadIndex, nullptr);
> +//    thread->Release(); // Called by PR_NewThreadPrivateIndex

This comment should be rewritten.

Comment 36

[Nathan Froyd [:froydnj]]

gcp gave me the short version of the rationale for this bug in IRC:

- We need to build WebRTC standalone;
- The XPCOM headers pull in too much stuff;
- We're going to write our own compatible versions that pull in less stuff;
- Yes, we know this is a maintenance headache;
- But we need this stuff yesterday.

He also said that Randall can provide all sorts of reasons why trying to untangle the XPCOM bits is simply not feasible...or at least might not be feasible in the time frames provided.  Randall, can you provide such rationale, and/or a little more background here?  There's not much in the bug.

Comment 37

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

Comment on attachment 8491482 [details] [diff] [review]
Non-XPCOM dependent networking & PeerConnection. r=

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

So, in addition to r-'ing it (for NS_NewThread() API overload/change, at least), I'm also r? to brian smith for two things: the srand() and that this is mocking the normal SocketTransportService, which is also where most NSS code (such as DTLS key generation, encryption, etc) gets run.

Also r? to mcmanus since this mocking SocketTransportService (Patrick, please forward to jduell or whomever if you prefer).

::: media/standalone/include/MediaDNS.h
@@ +18,5 @@
> +  DNSRecord(PRAddrInfo* info) : mInfo(info), mEnumPtr(nullptr) {}
> +  ~DNSRecord()
> +  {
> +    if (mInfo) {
> +      PR_FreeAddrInfo(mInfo);

You can get rid of the destructor by using mozilla/Scoped.h, and adding something like:

template<typename T>
struct ScopedCustomAddrInfoTraits0 : public ScopedFreePtrTraits<T>
{
  static void release(T* ptr)
  {
    if (ptr) {
      PR_FreeAddrInfo(ptr);
    }
  }
};

SCOPED_TEMPLATE(ScopedCustomAddrInfoPtr, ScopedCustomAddrInfoTraits0)

Then:

  ScopedCustomAddrInfo<PRAddrInfo*> mInfo;

Up to you if it's worth it.  The effect is pretty much the same in this case.  It would protect against leaks from things like reassigning mInfo.

@@ +47,5 @@
> +                                DNSListener *listener,
> +                                EventTarget *target,
> +                                Cancelable **result) = 0;
> +  enum {
> +    RESOLVE_BYPASS_CACHE = 1U,

Why the U's?  enum is int (barring c++11's enum typing)

::: media/standalone/include/MediaPeerConnectionObserver.h
@@ +4,5 @@
> +class Fake_MediaStream;
> +class Fake_SourceMediaStream;
> +class Fake_MediaStreamListener;
> +class Fake_MediaStreamDirectListener;
> +class Fake_DOMMediaStream;

add blank line before namespace for readability

@@ +15,5 @@
> +typedef Fake_DOMMediaStream DOMLocalMediaStream;
> +}
> +typedef Fake_DOMMediaStream nsIDOMMediaStream;
> +class nsIDOMDataChannel;
> +namespace mozilla {

ditto

::: media/standalone/src/MediaDNS.cpp
@@ +40,5 @@
> +  virtual nsresult Cancel(nsresult reason) { return NS_OK; }
> +
> +protected:
> +  enum State { Lookup, Respond };
> +  State mState;

Comment about how state works and the lifetime.  Overall, comments are lacking

@@ +55,5 @@
> +  if (mState == Lookup) {
> +
> +    PRAddrInfo* info = PR_GetAddrInfoByName(mHostname.c_str(), PR_AF_INET, PR_AI_ADDRCONFIG);
> +    if (info) {
> +      mRecord = new DNSRecord(info);

takes ownership.  Given that, it should at least be commented or:

* passed as an & param and have "new DNSRecord" null it
* make a ScopedCustomAddrInfoPtr and .forget it
* combine ScopedCustomAddrInfoPtr with & and have the new .forget() it

You get the idea.  Right now there's nothing here to make you realize the ptr was given up.

@@ +58,5 @@
> +    if (info) {
> +      mRecord = new DNSRecord(info);
> +    }
> +    mState = Respond;
> +    mTarget->Dispatch(this, MEDIA_DISPATCH_NORMAL);

I presume this is a synonym for NS_DISPATCH_NORMAL?

@@ +77,5 @@
> +                                EventTarget *target,
> +                                Cancelable **result);
> +protected:
> +  virtual ~DNSImpl();
> +  mozilla::RefPtr<Thread> mThread;

I presume equivalent more-or-less to nsIThread?  I also get a little concerned about using such a "generic" classname; odds of collision go up.  

More to the point, normally we return an nsIThread from NS_NewThread - as an out argument.  

Like:
nsCOMPtr<nsIThread> mThread;
if (NS_SUCCEEDED(NS_NewThread(getter_AddRefs(mThread))) { ...

@@ +82,5 @@
> +};
> +
> +DNSImpl::DNSImpl()
> +{
> +  mThread = NS_NewThread(nullptr);

Ummmm..  Unless you're doing something VERY weird and this is something totally not the normal NS_NewThread(), this is totally invalid.  And if it is something else, call it anything but NS_NewThread!

@@ +87,5 @@
> +}
> +
> +DNSImpl::~DNSImpl()
> +{
> +  mThread->Shutdown();

I presume something ensures this happens at or before the right point in shutdown, right??

::: media/standalone/src/MediaSocketTransportServiceImpl.cpp
@@ +49,5 @@
> +  }
> +
> +  delete []mActiveList;
> +  delete []mIdleList;
> +  delete []mPollList;

These should not be raw ptrs; and then you can get rid of this.

@@ +192,5 @@
> +
> +  Thread *thread = NS_GetCurrentThread();
> +
> +  // make sure the pseudo random number generator is seeded on this thread
> +  srand(static_cast<unsigned>(PR_Now()));

ooooooh.  No.  I really strongly suspect this is very, very wrong, but I'm not an NSS/etc expert.

@@ +254,5 @@
> +    }
> +  }
> +
> +  // Wait only 25ms if no pending events.
> +  int32_t n = PR_Poll(mPollList, mActiveCount, wait ? PR_MillisecondsToInterval(25) : PR_INTERVAL_NO_WAIT);

What's the application?  This means low ability to sleep with an app built on this running, I imagine.

@@ +257,5 @@
> +  // Wait only 25ms if no pending events.
> +  int32_t n = PR_Poll(mPollList, mActiveCount, wait ? PR_MillisecondsToInterval(25) : PR_INTERVAL_NO_WAIT);
> +
> +  if (n < 0) {
> +  } else {

??

@@ +258,5 @@
> +  int32_t n = PR_Poll(mPollList, mActiveCount, wait ? PR_MillisecondsToInterval(25) : PR_INTERVAL_NO_WAIT);
> +
> +  if (n < 0) {
> +  } else {
> +    for (i=0; i<int32_t(mActiveCount); ++i) {

Why are we forcing this loop's iterator to be signed, when the count is unsigned?

@@ +271,5 @@
> +    //
> +    // check for "dead" sockets and remove them (need to do this in
> +    // reverse order obviously).
> +    //
> +    for (i=mActiveCount-1; i>=0; --i) {

In this look I see it's used for a reason, though there are other ways to code it

::: media/standalone/src/MediaSocketTransportServiceImpl.h
@@ +39,5 @@
> +    uint16_t mElapsedTime;
> +  };
> +
> +  SocketContext *mActiveList;
> +  SocketContext *mIdleList;

These should not be raw ptrs. UniquePtrs perhaps

@@ +54,5 @@
> +  void RemoveFromPollList(SocketContext *);
> +  void MoveToIdleList(SocketContext *sock);
> +  void MoveToPollList(SocketContext *sock);
> +
> +  PRPollDesc *mPollList;

Should not be a raw ptr

Comment 38

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

Comment on attachment 8491475 [details] [diff] [review]
Miscellaneous parts for standalone WebRTC. r=

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

MPL2 needed in LOTS of places.  Stopped bothering to flag

::: media/standalone/include/MediaSegmentExternal.h
@@ +87,5 @@
> +};
> +
> +struct VideoChunk {
> +
> +};

Really?

::: media/standalone/include/MediaSharedBuffer.h
@@ +18,5 @@
> +
> +  static mozilla::TemporaryRef<SharedBuffer> Create(size_t aSize)
> +  {
> +    // FIXME? This seems wrong. How does this memory get freed?
> +    void* m = malloc(sizeof(SharedBuffer) + aSize);

Note that unlike the original code, this is fallible....

@@ +19,5 @@
> +  static mozilla::TemporaryRef<SharedBuffer> Create(size_t aSize)
> +  {
> +    // FIXME? This seems wrong. How does this memory get freed?
> +    void* m = malloc(sizeof(SharedBuffer) + aSize);
> +    mozilla::RefPtr<SharedBuffer> p = new (m) SharedBuffer();

This is "placement new".  Though I totally shudder at how it's used here (and in the original shared buffer code - which at least checks the size for isValid)

::: media/standalone/include/MediaTypes.h
@@ +7,5 @@
> +
> +typedef int64_t MediaTime;
> +typedef MediaTime StreamTime;
> +typedef int32_t TrackRate;
> +//const TrackRate TRACK_RATE_MAX = 1 << MEDIA_TIME_FRAC_BITS;

??  Remove, or uncomment, or explain

::: media/standalone/src/MediaExternal.cpp
@@ +5,5 @@
> +#include "MediaThread.h"
> +#include "MediaThreadManager.h"
> +#include "MediaTimer.h"
> +
> +namespace media {

New top-level namespace?  Not mozilla::media?

Logs?

::: media/standalone/src/MediaSegmentExternal.cpp
@@ +1,1 @@
> +#include "MediaSegmentExternal.h"

MPL2

also:  heh?  why?  I'm sure there's a reason....

::: media/standalone/src/MediaStubs.cpp
@@ +1,1 @@
> +#include "Latency.h"

MPL2

Also, perhaps a comment as to *why* these are stubbed?

Comment 39

[Patrick McManus [:mcmanus]]

Comment on attachment 8491482 [details] [diff] [review]
Non-XPCOM dependent networking & PeerConnection. r=

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

Without nathan's comments I wouldn't have any idea what you were trying to do in this bug. As it is, about all I can figure out is that you need to fork gecko and you want someone to look at the fork.. though of course now that it's forked you're going to have to maintain it yourself, which seems kinda questionable from a security standpoint if nothing else.

you're also forking some pretty old, but stable, code. So it has some idioms that wouldn't pass muster in a current review (reference management for instance). Randell points out some. I'm largely going to ignore them. But from time to time they do get cleaned up in pieces (a wholesale attempt to fix them bounced a year back, so I just do it in small chunks when I get a chance now) and a fork will miss out on that kind of thing - and it will also miss out on any scheduling changes we'd like to do and that's my bigger concern.

In general one of my goals is to bring rtc networking closer to necko networking - not farther apart. I want this so I can help with priority management of the network link. So I really don't care for this general direction at all. rjseup - how are we going to resolve that after your immediate need is met?

The other pragmatic thing that concerns me is the introduction of a second socket pool.. you can see with the ProbeMaxCount() logic that some windows installs tend to barf when they've got too many sockets going. We've never fully diagnosed it, but believe it has to do with winsock filters that weren't tested like that. (Shades of the old embedded select() issues). The existing code path tries to find the ceiling and put a cap on things, and though its not perfect it does help.. a fork further decentralizes that pool and may harm that logic.

If you go for a re-review here please ask :sworkman to look at the dns bits.

::: media/standalone/src/MediaSocketTransportServiceImpl.cpp
@@ +254,5 @@
> +    }
> +  }
> +
> +  // Wait only 25ms if no pending events.
> +  int32_t n = PR_Poll(mPollList, mActiveCount, wait ? PR_MillisecondsToInterval(25) : PR_INTERVAL_NO_WAIT);

Randell is right - that's a problem.

Here's the necko rules:

1] If there is stuff in the run queue already, then NO_WAIT is used. (you're doing a quick poll to see if there is more to do, but you need to go run those over events.)

2] Only ff the OS does not support pollable events, then 25ms is used. We don't have any tier one platforms with this problem. (a pollable event is some kind of local pipe we can add to the poll() list to wake ourselves up on for non-network activity the socket thread is interested in.. e.g. "add a new socket".) So this is a pipe() or the windows equivalent in practice.

3] If there are no sockets registered (other than the pollable event one), block infintely assuming the pollable event wakes you up.

4] Use the minimum time polltimeout for each socket in the list. For all normal necko cases I am familiar with this is set to 0xffff which is defined to be infinite.. but addons or some b2g app or whatnot could use a smaller value that I'm not aware of.

Comment 40

[(no longer active)]

I am not sure about the details of the work happening here, but *please* avoid creating your own refcounting system at all costs.  We have had a terrible track record with the MFBT RefCounted infrastructure that lacked all of the debugging checks that the XPCOM ones have (the terrible track record being letting security sensitive bugs go in which would very easily be diagnosed with the debug assertions had people used the XPCOM refcounting infrastructure.)  See bug 1070064 comment 1 for context.

If making this code build standalone means you can't just use nsRefPtr and friends directly, please file a bug with the specific issues and requirements so that we can figure out a solution to make the standalone build use case work without sacrificing correctness.  Thanks!

Comment 41

[Nathan Froyd [:froydnj]]

Comment on attachment 8491476 [details] [diff] [review]
Non-XPCOM dependent Mutexes, CondVars, etc. r=

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

Canceling this review for reasons previously raised.

Comment 42

[Nathan Froyd [:froydnj]]

Comment on attachment 8491479 [details] [diff] [review]
Non-XPCOM dependent Timers. r=

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

Canceling this review; I think the maintenance concerns that have already been raised and the security concerns Patrick pointed out are sufficient to say that we need to figure out a different way of doing this.

Comment 43

[Nathan Froyd [:froydnj]]

Comment on attachment 8491481 [details] [diff] [review]
Non-XPCOM Threads, Runnables & Thread Manager. r=

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

Everything said for timers goes double for threads here.

Comment 44

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8491482 [details] [diff] [review]
Non-XPCOM dependent networking & PeerConnection. r=

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

This is outside the scope of things I work on.

Comment 45

[Randall Barker [:rbarker]]

(In reply to Nathan Froyd (:froydnj) from comment #36)
> gcp gave me the short version of the rationale for this bug in IRC:
> 
> - We need to build WebRTC standalone;
> - The XPCOM headers pull in too much stuff;
> - We're going to write our own compatible versions that pull in less stuff;
> - Yes, we know this is a maintenance headache;
> - But we need this stuff yesterday.
> 
> He also said that Randall can provide all sorts of reasons why trying to
> untangle the XPCOM bits is simply not feasible...or at least might not be
> feasible in the time frames provided.  Randall, can you provide such
> rationale, and/or a little more background here?  There's not much in the
> bug.

The purpose of this work was to allow a Roku Device to receive WebRTC calls. This would enable tab streaming from fennec to a Roku device. A Roku has a hard requirement for package size. Bundling XUL is not an option. I tried two different approaches before mocking out the required functionality. First I tried using XUL with the standalone WebRTC library created for unit testing. While I could get this to "run" on desktop, the resulting binary was too large. I tried link against just the parts of XUL I needed to resolve symbols using the *.a.desc files, however due to all the interdependencies, I ended up essentially having XUL statically linked into the application and even then I was never able to get it to run. I would have preferred to have just linked in the symbols necessary to compile and run from the existing code base. I do not currently see a way to achieve that.

Comment 46

[Randall Barker [:rbarker]]

We need to get this moving forward. Any ideas on alternatives that also meet our requirements, mainly binary size? I tried again to use the XPCOM code unchanged and quickly got caught up on all the dependencies.

Comment 47

[Nathan Froyd [:froydnj]]

(In reply to Randall Barker [:rbarker] from comment #46)
> We need to get this moving forward. Any ideas on alternatives that also meet
> our requirements, mainly binary size? I tried again to use the XPCOM code
> unchanged and quickly got caught up on all the dependencies.

My only idea is to do something like, given a class nsFoo that implements nsIFoo, is to move the guts of the class into nsFooBase that doesn't inherit from nsIFoo.  Then make nsFoo inherit from nsFooBase and nsIFoo.  That way, you ought to be able to confine most of the XPCOM-ish, pull-in-all-of-libxul stuff to nsFoo, and the standalone WebRTC library can use nsFooBase.  Names subject to bikeshedding, of course.

I suppose AddRef/Release is problematic, since we want nsFoo to have the XPCOM leak-checking bits, but (presumably) you don't want leak-checking bits in nsFooBase.  Maybe template nsFooBase and implement AddRef/Release in terms of the template parameter?  Or perhaps nsFooBase shouldn't have anything to do with AddRef/Release, and subclasses should provide all the refcounting behavior.  Not sure here.

I don't know whether the same strategy would work for netwerk/ or not.  But that's the best idea I have from a code maintenance perspective.

What sort of hard size constraint are you looking at?  Maybe we ought to be trying to stub out large parts of libxul, instead, or introducing a "minimized" build config that simply doesn't include things like...layout/, or intl/, or even dom/.  Again, I'm not sure how feasible that is, just tossing out ideas.

Comment 48

[Gian-Carlo Pascutto [:gcp]]

(In reply to Nathan Froyd (:froydnj) from comment #47)
>...and the standalone WebRTC library can use nsFooBase.  

s/the standalone WebRTC library/all WebRTC related code in mozilla-central/

Comment 49

[Randall Barker [:rbarker]]

Stand alone WebRTC is no longer being developed.