760858 - Need WebRTC Media Transport Abstraction for Peer Connection and mTransport

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Suhas]

WebRTC library needs to be linked with mTransport for testing out media transport scenarios

Comment 1

[Suhas]

Attachment: WebRTC linking with standalone audio test case

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 629513 [details] [diff] [review]
WebRTC linking with standalone audio test case

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

I've noted a bunch of coding nits but generally, I think we need to be clear on what this is supposed to do,
which is verify that WebRTC is working correctly. That means that to the extent possible every
operation needs an error check for the right behavior.

::: media/mtransport/test/webrtc_standalonetest.cpp
@@ +31,5 @@
> +
> +//Very Basic External Transport
> +
> +class _ExternalTransport : public webrtc::Transport 
> +{

Please don't name things with leading underscores. That's reserved in the global namespace.

@@ +53,5 @@
> +    return 0;
> +  }
> +
> +private:
> +  webrtc::VoENetwork* ptrVoENet;

Why do you need this member, since it's never initialized and apparently ignored.

@@ +61,5 @@
> +
> +class WebrtcTest : public ::testing::Test {
> + public:
> +  WebrtcTest() {
> +    std::cerr << "  Creating Voice Engine " << std::endl;

Provide initializers for all variables outside the constructor.

@@ +64,5 @@
> +  WebrtcTest() {
> +    std::cerr << "  Creating Voice Engine " << std::endl;
> +  	mVoiceEngine = webrtc::VoiceEngine::Create();
> +    // create voice engine & interfaces - no cerror check done here
> +    mPtrVoEBase = webrtc::VoEBase::GetInterface(mVoiceEngine);

These tests should have error checks everywhere. That's part of the point of unit tests.

@@ +70,5 @@
> +    mPtrVoEHw = webrtc::VoEHardware::GetInterface(mVoiceEngine);
> +    mPtrVoENetwork = webrtc::VoENetwork::GetInterface(mVoiceEngine);
> +    mPtrVoEFile = webrtc::VoEFile::GetInterface(mVoiceEngine); 
> +    mPtrVoECodec = webrtc::VoECodec::GetInterface(mVoiceEngine);
> +   fileToPlay = "/Users/XTM/HTML5/firefox/firefox-alder-ekr/src/media/webrtc/trunk/test/data/voice_engine/audio_short16.pcm";

This obviously isn't going to be portable to anyone's machine.

@@ +89,5 @@
> +  void SetUp(bool playSpeaker) {
> +   playToSpeaker = playSpeaker;
> +   mChannel = mPtrVoEBase->CreateChannel();
> +   // out network layer
> +   if(false == playToSpeaker) {

Style: replace if (false == bool) with if (!bool)

@@ +91,5 @@
> +   mChannel = mPtrVoEBase->CreateChannel();
> +   // out network layer
> +   if(false == playToSpeaker) {
> +     extXport = new _ExternalTransport(mPtrVoENetwork);
> +     if( -1 == mPtrVoENetwork->RegisterExternalTransport(mChannel, *extXport))

These errors need to be enforced with gtest style ASSERTs, etc., not with explicit tests.

@@ +103,5 @@
> +    webrtc::CodecInst cinst;
> +
> +    mPtrVoEBase->SetLocalReceiver(mChannel,DEFAULT_PORT);
> +    mPtrVoEBase->SetSendDestination(mChannel,DEFAULT_PORT,"127.0.0.1");
> +    mPtrVoEBase->StartPlayout(mChannel);

Why are you using network I/O at all here? That's the point of the external transport.

@@ +121,5 @@
> +    cinst.rate = -1; // default rate
> +    cinst.pacsize = 480; // 30ms
> +    cinst.plfreq = 16000;
> +    mPtrVoECodec->SetSendCodec(mChannel, cinst); 
> +    

Should you really be starting to play *before* you set the codec?

@@ +137,5 @@
> +  webrtc::VoENetwork* mPtrVoENetwork;
> +  webrtc::VoEBase*    mPtrVoEBase;
> +  webrtc::VoEFile*   mPtrVoEFile;
> +  webrtc::VoEHardware* mPtrVoEHw;
> +  webrtc::VoECodec* mPtrVoECodec;

Please make all of these Scoped (http://mxr.mozilla.org/mozilla-central/source/mfbt/Scoped.h) so they auto-delete.

@@ +145,5 @@
> +  bool playToSpeaker;
> +};
> +
> +
> +// TODO([e-mail]): add a test with more values

Why am I the target of this TODO? Shouldn't this be you?

@@ +155,5 @@
> +  StopMedia();
> +}
> +
> +TEST_F(WebrtcTest, TestExternalTransportPlayout) {
> +  SetUp(false);

This test should presumably be testing that packets are appearing on the network interface, right?

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 629513 [details] [diff] [review]
WebRTC linking with standalone audio test case

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

I've noted a bunch of coding nits but generally, I think we need to be clear on what this is supposed to do,
which is verify that WebRTC is working correctly. That means that to the extent possible every
operation needs an error check for the right behavior.

::: media/mtransport/test/webrtc_standalonetest.cpp
@@ +31,5 @@
> +
> +//Very Basic External Transport
> +
> +class _ExternalTransport : public webrtc::Transport 
> +{

Please don't name things with leading underscores. That's reserved in the global namespace.

@@ +53,5 @@
> +    return 0;
> +  }
> +
> +private:
> +  webrtc::VoENetwork* ptrVoENet;

Why do you need this member, since it's never initialized and apparently ignored.

@@ +61,5 @@
> +
> +class WebrtcTest : public ::testing::Test {
> + public:
> +  WebrtcTest() {
> +    std::cerr << "  Creating Voice Engine " << std::endl;

Provide initializers for all variables outside the constructor.

@@ +64,5 @@
> +  WebrtcTest() {
> +    std::cerr << "  Creating Voice Engine " << std::endl;
> +  	mVoiceEngine = webrtc::VoiceEngine::Create();
> +    // create voice engine & interfaces - no cerror check done here
> +    mPtrVoEBase = webrtc::VoEBase::GetInterface(mVoiceEngine);

These tests should have error checks everywhere. That's part of the point of unit tests.

@@ +70,5 @@
> +    mPtrVoEHw = webrtc::VoEHardware::GetInterface(mVoiceEngine);
> +    mPtrVoENetwork = webrtc::VoENetwork::GetInterface(mVoiceEngine);
> +    mPtrVoEFile = webrtc::VoEFile::GetInterface(mVoiceEngine); 
> +    mPtrVoECodec = webrtc::VoECodec::GetInterface(mVoiceEngine);
> +   fileToPlay = "/Users/XTM/HTML5/firefox/firefox-alder-ekr/src/media/webrtc/trunk/test/data/voice_engine/audio_short16.pcm";

This obviously isn't going to be portable to anyone's machine.

@@ +89,5 @@
> +  void SetUp(bool playSpeaker) {
> +   playToSpeaker = playSpeaker;
> +   mChannel = mPtrVoEBase->CreateChannel();
> +   // out network layer
> +   if(false == playToSpeaker) {

Style: replace if (false == bool) with if (!bool)

@@ +91,5 @@
> +   mChannel = mPtrVoEBase->CreateChannel();
> +   // out network layer
> +   if(false == playToSpeaker) {
> +     extXport = new _ExternalTransport(mPtrVoENetwork);
> +     if( -1 == mPtrVoENetwork->RegisterExternalTransport(mChannel, *extXport))

These errors need to be enforced with gtest style ASSERTs, etc., not with explicit tests.

@@ +103,5 @@
> +    webrtc::CodecInst cinst;
> +
> +    mPtrVoEBase->SetLocalReceiver(mChannel,DEFAULT_PORT);
> +    mPtrVoEBase->SetSendDestination(mChannel,DEFAULT_PORT,"127.0.0.1");
> +    mPtrVoEBase->StartPlayout(mChannel);

Why are you using network I/O at all here? That's the point of the external transport.

@@ +121,5 @@
> +    cinst.rate = -1; // default rate
> +    cinst.pacsize = 480; // 30ms
> +    cinst.plfreq = 16000;
> +    mPtrVoECodec->SetSendCodec(mChannel, cinst); 
> +    

Should you really be starting to play *before* you set the codec?

@@ +137,5 @@
> +  webrtc::VoENetwork* mPtrVoENetwork;
> +  webrtc::VoEBase*    mPtrVoEBase;
> +  webrtc::VoEFile*   mPtrVoEFile;
> +  webrtc::VoEHardware* mPtrVoEHw;
> +  webrtc::VoECodec* mPtrVoECodec;

Please make all of these Scoped (http://mxr.mozilla.org/mozilla-central/source/mfbt/Scoped.h) so they auto-delete.

@@ +145,5 @@
> +  bool playToSpeaker;
> +};
> +
> +
> +// TODO([e-mail]): add a test with more values

Why am I the target of this TODO? Shouldn't this be you?

@@ +155,5 @@
> +  StopMedia();
> +}
> +
> +TEST_F(WebrtcTest, TestExternalTransportPlayout) {
> +  SetUp(false);

This test should presumably be testing that packets are appearing on the network interface, right?

Comment 4

[Suhas]

Attachment: Webrtc Stanalone Audio Unit Test - Version 1.1

Comment 5

[Suhas]

Updated new patch with fixing the comments from Eric. 
Also didnot make webrtc pointer scoped since they don't get either new'd or malloc'ed  but only get interface-ref-counted.

This patch includes 3 test-cases
 - play file as mic to the speaker
 - dump recorded packets on the external transport interface
 - play the RTP packets received on the external transport interface to the speaker

Comment 6

[Suhas]

Attachment: Media Transport Abstraction patch for mTransport and Peer Connection

Comment 7

[Suhas]

Uploaded version 1 of the Transport Abstraction patch ...

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 640030 [details]
Media Transport Abstraction patch for mTransport and Peer Connection

Review of attachment 640030 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/transport/AudioSession.cpp
@@ +12,5 @@
> +
> +namespace mozilla {
> +
> +//Factory Implementation
> +mozilla::TemporaryRef<AudioSessionConduit> AudioSessionConduit::Create()

Why not RefPtr.

@@ +14,5 @@
> +
> +//Factory Implementation
> +mozilla::TemporaryRef<AudioSessionConduit> AudioSessionConduit::Create()
> +{
> +  printf("WebrtcAudioConduitImpl:: Create ");  	

Don't use Printf.

@@ +19,5 @@
> +  return  new WebrtcAudioConduit();
> +}
> +
> +
> +void WebrtcAudioConduit::Construct()

This seems like it can fail in a lot of ways. Prefer a minimal constructor and an Init() that returns an error code.

@@ +23,5 @@
> +void WebrtcAudioConduit::Construct()
> +{
> +  int res = 0; 
> +
> +  if(!mVoiceEngine) {

Why would mVoiceEngine not be NULL?

@@ +25,5 @@
> +  int res = 0; 
> +
> +  if(!mVoiceEngine) {
> +   mVoiceEngine = webrtc::VoiceEngine::Create();
> +   if(NULL == mVoiceEngine)

if (!mVoiceEngine)

@@ +32,5 @@
> +        return;
> +    }
> +  }
> +
> +  printf("\nWebrtcAudioConduitImpl:: Engine Created: Init'ng the interfaces "); 

logging.

@@ +58,5 @@
> +//only for testing
> +  mPtrVoEFile  = webrtc::VoEFile::GetInterface(mVoiceEngine);
> +#endif
> +
> +  if(!mPtrVoENetwork || !mPtrVoECodec || !mPtrVoEXmedia)

I would prefer to test each one as it is done.

@@ +65,5 @@
> +														mPtrVoEBase->LastError());
> +  	return;
> +  }
> +
> +  if(-1 == (mChannel = mPtrVoEBase->CreateChannel()))

Break this up please.

@@ +96,5 @@
> +  initDone = true;
> +}
> +
> +//TODO:Crypt: Discuss the engine ownership
> +void WebrtcAudioConduit::Destruct()

Yes, lets.

@@ +103,5 @@
> +  int error = 0;   
> +
> +
> +  //Deal with External Media
> +  error = mPtrVoEBase->StopPlayout(mChannel); 

What happens if these are NULL?
Protect yourself.

@@ +131,5 @@
> +      webrtc::VoiceEngine::Delete(mVoiceEngine);
> +      mVoiceEngine = NULL;  
> +   }
> +
> +  initDone = false;

What's the reasoning here? I can't re-init, right?

@@ +188,5 @@
> +  error = mPtrVoEBase->StartSend(mChannel);
> +  if(-1 == error) 
> +  {
> +  	printf("\n WebrtcAudioConduitImpl: StartSend() Failed %d ", 
> +									mPtrVoEBase->LastError());

Really scary indent.

@@ +200,5 @@
> +int WebrtcAudioConduit::ConfigureRecvMediaCodec(CodecConfig* codecInfo)
> +{
> +
> +
> +  AudioCodecConfig* codecConfig = (AudioCodecConfig*) codecInfo;

Don't use C style casts. This should be static_cast. Also, why do we need a downcast here at all. This is unsafe.

@@ +281,5 @@
> +  // we should be good here
> +  return 0;
> +}
> +
> +void WebrtcAudioConduit::GetAudioFrame(int16_t speechData[],

Indent.

@@ +290,5 @@
> +  printf("\nWebrtcAudioConduitImpl:GetAudioFrame ");
> +  lengthSamples = 0;
> +
> +  if(mEnginePlaying)
> +   	mPtrVoEXmedia->ExternalPlayoutGetData((WebRtc_Word16*) speechData, (int)samplingFreqHz,

Line length/

@@ +318,5 @@
> +{
> +  printf("\nWebrtcAudioConduitImpl:: SendRtpPacket len %d ",len);
> +
> +   if(mTransport)
> +     mTransport->SendRtpPacket(data, len); 

Why are you ignoring errors?

::: media/webrtc/signaling/transport/AudioSession.h
@@ +56,5 @@
> +  virtual int SendRTCPPacket(int channel, const void *data, int len) ;
> +
> +	
> +	
> +  explicit WebrtcAudioConduit(): initDone(false)

Don't need explicit here.

Put the constructor at the top.

@@ +58,5 @@
> +	
> +	
> +  explicit WebrtcAudioConduit(): initDone(false)
> +								,mChannel(-1)
> +        						,mTransport(0)

NULL

@@ +59,5 @@
> +	
> +  explicit WebrtcAudioConduit(): initDone(false)
> +								,mChannel(-1)
> +        						,mTransport(0)
> +	        		    		,mRenderer(0)

NULL

@@ +63,5 @@
> +	        		    		,mRenderer(0)
> +			                    ,sessionId(-1) 
> +								,mVoiceEngine(0)
> +								,mEnginePlaying(false)
> +  {

Do not use printf.

@@ +65,5 @@
> +								,mVoiceEngine(0)
> +								,mEnginePlaying(false)
> +  {
> +  	printf("\n WebrtcAudioConduit : Constructor ");
> +    Construct(); 

Why not just do the construct operations directly.

@@ +68,5 @@
> +  	printf("\n WebrtcAudioConduit : Constructor ");
> +    Construct(); 
> +  }
> +
> +  virtual ~WebrtcAudioConduit() 

Why not just do the Destruct operations directly.

@@ +78,5 @@
> +private:
> +  void Construct();
> +  void Destruct();
> +
> +  bool initDone;

mInitDone?

::: media/webrtc/signaling/transport/CodecConfig.h
@@ +6,5 @@
> +
> +
> +namespace mozilla {
> +
> +struct CodecConfig

Why do these inherit from CodecConfig? That's just a source of confusion, since the downcast isn't type-safe.

@@ +20,5 @@
> +  int mPacSize;
> +  int mChannels;
> +  int mRate;
> +
> +  AudioCodecConfig()

Why even provide this constructor?

@@ +24,5 @@
> +  AudioCodecConfig()
> +  {
> +  }
> +
> +  AudioCodecConfig(int type, std::string name, int freq, int pacSize, int channels, int rate) {

Use initializers, not assignment.

@@ +44,5 @@
> +  std::string mName;
> +  int mWidth;
> +  int mHeight;
> +
> +  VideoCodecConfig()

Why is this defined?

@@ +48,5 @@
> +  VideoCodecConfig()
> +  {
> +  }
> +
> +  VideoCodecConfig(int type, std::string name,int width, int height)

Use initializers.

::: media/webrtc/signaling/transport/MediaTransportAbstraction.h
@@ +20,5 @@
> + */
> +class TransportInterface : public mozilla::RefCounted<TransportInterface>
> +{
> +public:
> +  virtual ~TransportInterface() {};

Pure virtual.

@@ +22,5 @@
> +{
> +public:
> +  virtual ~TransportInterface() {};
> +
> +  //send packet to a peer as setup by the transport

What do these return values mean?

@@ +63,5 @@
> + */
> +class AudioRenderer : public mozilla::RefCounted<AudioRenderer>
> +{
> +public: 
> +  virtual ~AudioRenderer() {};

Pure virtual

@@ +86,5 @@
> + */
> +class MediaSessionConduit :  public mozilla::RefCounted<MediaSessionConduit>
> +{
> +public:
> +  virtual ~MediaSessionConduit() {};

Pure virtual

@@ +111,5 @@
> +class VideoSessionConduit : public MediaSessionConduit
> +{
> +public:
> +  // Factory Object 
> +  static mozilla::TemporaryRef<VideoSessionConduit> Create();

Why is this a temporary ref?

@@ +141,5 @@
> +{
> +public:
> +
> +  // Factory Object 
> +  static mozilla::TemporaryRef<AudioSessionConduit> Create();

Why TemporaryRef

@@ +143,5 @@
> +
> +  // Factory Object 
> +  static mozilla::TemporaryRef<AudioSessionConduit> Create();
> +
> +  virtual ~AudioSessionConduit() {};

Pure virtual.

::: media/webrtc/signaling/transport/VideoSession.h
@@ +71,5 @@
> +        unsigned char*,int, uint32_t , int64_t
> +  	);
> +
> +
> +  explicit WebrtcVideoConduit(): initDone(false)

Indent style.

Comment 9

[Eric Rescorla (:ekr)]

Additional comments:


1. Return value consistency
2. Use NS_INLINE_IMPL_THREADSAFE_...
3. Pure virtual destructors
4. Use logging, not printf
5. Define the scary constructors for the non-struct objects.
6. Do most work in initializers.
7. C++ style casts (Eff C++ #27)
8. Error returns.
9. Indent style (Consider Google CPP style or something like it)

Comment 10

[Suhas]

Uploaded a new patch with most of the comments incorporated , except for 2. Since I am not sure if we want to make these objects nsCOMPtrs and hence bring QueryInterface circuitry into the impln ...

Comment 11

[Suhas]

Attachment: MediaSessionInterface Patch

Comment 12

[Suhas]

weird .. the patch still has indentation issues .. the code looks pretty good on my editor ... hmmmm

Comment 13

[Suhas]

Attachment: MediaSessionInterface Patch

Comment 14

[Suhas]

Attachment: MediaSessionInterface Patch

Comment 15

[Suhas]

Attachment: MediaSessionInterface Patch

Comment 16

[Suhas]

Attachment: MediaSessionInterface Patch

Comment 17

[Ethan Hugg [:ehugg]]

Comment on attachment 641735 [details]
MediaSessionInterface Patch

Review of attachment 641735 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/media_session/AudioSession.cpp
@@ +9,5 @@
> +
> +namespace mozilla {
> +
> +//Stealing Erk's logging for now
> +MLOG_INIT("audiosession");

I'm a bit confused here about why you created your own logging module instead of using the CSFLog stuff that the rest of ECC uses.  I'm concerned that someone will turn on signaling logs and expect this debugging to show up.  If you think using CSFLogDebug is a bad idea, we should discuss.

@@ +131,5 @@
> +
> +MediaSessionErrorCode 
> +  WebrtcAudioConduit::AttachRenderer(mozilla::RefPtr<AudioRenderer> aAudioRenderer)
> +{
> +  MLOG(PR_LOG_DEBUG, AUDIO_SESSION << "AttachRenderer");

We should be using __FUNCTION__ in these types of log messages.  I don't think this code will now be built on any compiler that doesn't support __FUNCTION__, __LINE__ and __FILE__

@@ +178,5 @@
> +  { 
> +  	MLOG(PR_LOG_ERROR,AUDIO_SESSION << "Setting Send Codec Failed " 
> +                                    <<  mPtrVoEBase->LastError());
> +    error = mPtrVoEBase->LastError();
> +    if(error == 8084)

8084 seems like a magic number in this context.  We should either find a .h that has it defined or define it ourselves somewhere.

@@ +220,5 @@
> +
> +  if(mPtrVoECodec->SetRecPayloadType(mChannel,cinst) == -1)
> +  {
> +  	MLOG(PR_LOG_ERROR, AUDIO_SESSION << "Setting Receive Codec Failed " 
> +   											             << mPtrVoEBase->LastError());

Still see crazy indents like this.

@@ +236,5 @@
> +  {
> +  	MLOG(PR_LOG_ERROR , AUDIO_SESSION << "StartReceive Failed " 
> +									                     << mPtrVoEBase->LastError());
> +    error = mPtrVoEBase->LastError();
> +    if(error == 10033 )

Here's another magic number.

::: media/webrtc/signaling/media_session/CodecConfig.h
@@ +1,1 @@
> +

This one has no header

::: media/webrtc/signaling/media_session/MediaEngineWrapper.cpp
@@ +14,5 @@
> +  if(_instance)
> +  {
> +     return _instance;
> +  } else {
> +    _instance = new WebRTCEngineWrapper();

We should think about whether these Instance() methods need locking so two threads don't hit this line at the same time.

::: media/webrtc/signaling/media_session/VideoTypes.h
@@ +1,1 @@
> +#ifndef VIDEO_TYPE_

This one has no header

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 641735 [details]
MediaSessionInterface Patch

Review of attachment 641735 [details]:
-----------------------------------------------------------------

Fix indents throughout.

I suggest that you fix the API issues I noted in MediaSessionErrors and MediaSessionInterface. Also, rename Session -> Conduit in filenames if that is the new term.

At that point you can resubmit the patch and work on the implementation issues while I work on integration with the rest of the system.

::: media/webrtc/signaling/media_session/AudioSession.cpp
@@ +9,5 @@
> +
> +namespace mozilla {
> +
> +//Stealing Erk's logging for now
> +MLOG_INIT("audiosession");

Not saying I endorse the use of MLOG here. If this is nominally part of ECC it should use CSFLog. If it's not, then of course it can use whatever and shouldn't use CSFLog.

@@ +11,5 @@
> +
> +//Stealing Erk's logging for now
> +MLOG_INIT("audiosession");
> +
> +mozilla::RefPtr<AudioSessionConduit> AudioSessionConduit::Create()

A lot of the comments from Video apply to this file as well.

::: media/webrtc/signaling/media_session/AudioSession.h
@@ +37,5 @@
> +  typedef webrtc::VoEBase     Base;
> +  typedef webrtc::VoENetwork  Network;
> +  typedef webrtc::VoECodec    Codec;
> +  typedef webrtc::VoEFile     File;
> +  typedef webrtc::VoEExternalMedia XMedia;

See  comments about using in the Video version.

@@ +73,5 @@
> +
> +	
> +	
> +  WebrtcAudioConduit():
> +  mVoiceEngine(WebRTCEngineWrapper::Instance()->GetVoiceEngine())

Commas at end, not beginning.

@@ +89,5 @@
> +  {
> +  	printf("\n WebrtcAudioConduit : Constructor ");
> +  }
> +
> +  virtual ~WebrtcAudioConduit() 

Why does Desctruct need to be separate?

@@ +94,5 @@
> +  {
> +     Destruct();
> +  }
> +
> +  MediaSessionErrorCode Init();

Make private.

@@ +101,5 @@
> +  WebrtcAudioConduit(WebrtcAudioConduit const&) {}
> +  void operator=(WebrtcAudioConduit const&) { }
> +
> +
> +  webrtc::VoiceEngine* mVoiceEngine; //weak reference

Not weak, just raw. There's no way to check for current validity.

@@ +120,5 @@
> +};
> +
> +} // end namespace
> +
> +#define AUDIO_SESSION "WebrtcAudioConduit "

What is this for?

::: media/webrtc/signaling/media_session/CodecConfig.h
@@ +13,5 @@
> +};
> +
> +struct AudioCodecConfig : public CodecConfig
> +{
> +  int mType;

This file does not seem to have addressed my previous comments.

::: media/webrtc/signaling/media_session/MediaEngineWrapper.cpp
@@ +14,5 @@
> +  if(_instance)
> +  {
> +     return _instance;
> +  } else {
> +    _instance = new WebRTCEngineWrapper();

Concur.

::: media/webrtc/signaling/media_session/MediaEngineWrapper.h
@@ +77,5 @@
> + *  T is the interface type
> + *  E is the Engine Type
> + *  Ex: scoped_m_ptr<webrtc::VoENetwork>
> + */
> +template <class T, class E>

There seems to be duplication here. This isn't used... why is it better than ScopedPtr

::: media/webrtc/signaling/media_session/MediaSessionErrors.h
@@ +8,5 @@
> +
> +
> +namespace mozilla 
> +{
> +enum MediaSessionErrorCode

I'm not excited about all of these being pulled into the global namespace. I think they need prefixes to indicate that they are media errors.

::: media/webrtc/signaling/media_session/MediaSessionInterface.h
@@ +7,5 @@
> +
> +#include "nspr.h"
> +#include "prerror.h"
> +
> +#include "nsCOMPtr.h"

You do not need to include nsCOMPtr.

The relevant includes are in nsIsupportsImpl.h

@@ +10,5 @@
> +
> +#include "nsCOMPtr.h"
> +#include "nsXPCOM.h"
> +#include "mozilla/RefPtr.h"
> +#include "video_engine/include/vie_capture.h"

This needs to go/

@@ +16,5 @@
> +#include "VideoTypes.h"
> +#include "MediaSessionErrors.h"
> +
> +
> +//TODO:crypt:Implemement the interfaces as nsCOMPtr for Ref-Counting 

No, just replace the inheritance from RefCounted<T> with an NS_INLINE_DECL_THREADSAFE_REFCOUNTING in the body.

@@ +82,5 @@
> +   * this callback. 
> +   * Such implementations should be quick in processing the frames and return
> +   * immediately.
> +   */
> +  virtual void RenderVideoFrame(unsigned char* buffer,

const unsigned char *

@@ +94,5 @@
> + /**
> +  * 1. Abstract renderer for audio data
> +  * 2. This class acts as abstract interface between the audio-engine and 
> +  *    audio-engine agnostic renderer implementations.
> +  * 3. Concrete implementation of this interface is responsible for 

implementations.

@@ +115,5 @@
> +   * Note: It is responsibility of the concrete implementation to manange
> +   * memory for the speechData
> +   */
> +  virtual void RenderAudioFrame(int16_t speechData[],
> +			                          uint32_t samplingFreqHz,

Indent <- back to (

@@ +180,5 @@
> +   * Factory function to create and initialize a Video Conduit Session
> +   * return: Concrete VideoSessionConduitObject or nsnull in the case 
> +   *         of failure 
> +   */
> +  static RefPtr<VideoSessionConduit> Create();

mozilla::RefPtr. Or if you're assuming you are in mozilla::, then remove all of these.

@@ +187,5 @@
> +
> +  /**
> +   * Function to attach Renderer end-point of the Media-Video conduit.
> +   * @param aRenderer : Reference to the concrete Video renderer implementation 
> +   * NOTE: Owner ship of the aRenderer is not maintained by the conduit

Ownership. Also,  I don't understand this? The RefPtr implicitly involves holding it open.

@@ +202,5 @@
> +   *                       if 0 timestamp is automatcally generated
> +   * NOTE: Calling function shouldn't release the frame till this function
> +   * completes to the fullest. 
> +   */
> +  virtual MediaSessionErrorCode SendVideoFrame(unsigned char* video_frame,

Fix indent. const unsigned char *.

@@ +207,5 @@
> +			                                         unsigned int video_frame_length,
> +			                                          unsigned short width,
> +			                                          unsigned short height,
> +			                                          VideoType video_type,
> +			                                          uint64_t capture_time) = 0;

What units is the timestamp in?

@@ +210,5 @@
> +			                                          VideoType video_type,
> +			                                          uint64_t capture_time) = 0;
> +
> + //test only - send I420 frame
> + virtual int SendI420VideoFrame(const webrtc::ViEVideoFrameI420& video_frame,

Fix indent.

@@ +238,5 @@
> +public:
> +
> +   /**
> +    * Factory function to create and initialize a Video Conduit Session
> +    * return: Concrete VideoSessionConduitObject or nsnull in the case 

nsnull or NULL? be consistent.

@@ +251,5 @@
> +   * @param aRenderer : Reference to the concrete Video renderer
> +   * implementation 
> +   * NOTE: Owner ship of the aRenderer is not maintained by the conduit
> +   */
> +  virtual MediaSessionErrorCode AttachRenderer(

Fix indent.

@@ +261,5 @@
> +   * @param lengthSamples: Number of 16 bit samples in the buffer 
> +   * @param samplingFreqHz: Frequency/rate of the sampling 
> +   * @param capture_time: Sample captured timestamp
> +   */
> +  virtual MediaSessionErrorCode SendAudioFrame(const int16_t speechData[], 

Fix indent.

::: media/webrtc/signaling/media_session/VideoSession.cpp
@@ +15,5 @@
> +mozilla::RefPtr<VideoSessionConduit> VideoSessionConduit::Create()
> +{
> +  MLOG(PR_LOG_ERROR, VIDEO_SESSION <<"WebrtcVideoConduitImpl:: Create ");  	
> +  WebrtcVideoConduit* obj = new WebrtcVideoConduit();
> +  if(obj)

Return early on error, not success.

@@ +24,5 @@
> +
> +  return nsnull; 
> +}
> +
> +/** Super Class Pure destructor **/

You shouldn't need to do this. It should automatcically be synthesized.

@@ +39,5 @@
> +     MLOG(PR_LOG_ERROR, VIDEO_SESSION <<" Unable to create video engine ");
> +     return kVideoSessionNotInited;
> +  }
> +
> +  //temp logging only

Conditionalize this somehow.

@@ +47,5 @@
> +  MLOG(PR_LOG_DEBUG, VIDEO_SESSION <<" Engine Created: Init'ng the interfaces "); 
> +
> +  if(!mPtrViEBase || (mPtrViEBase->Init() == -1 ) )
> +  {
> +  	MLOG(PR_LOG_ERROR, VIDEO_SESSION <<"Vide Engine Init Failed " 

Vide -> Video. 

Fix indent.

@@ +97,5 @@
> +
> +  MLOG(PR_LOG_ERROR, VIDEO_SESSION <<"Initialization Done");  
> +  
> +  // we should be good here
> +  initDone = true;

Where is this used? It's not possible to have one of these with initDone == false

@@ +102,5 @@
> +  return kNoError;
> +}
> +
> +//TODO:crypt:Improve the cleanup
> +void WebrtcVideoConduit::Destruct() 

Shouldn't this just be in the destructor?

@@ +109,5 @@
> +  int error = 0;    
> +  //Deal with External Capturer   
> +  if(mPtrViECapture)
> +  {
> +    error = mPtrViECapture->DisconnectCaptureDevice(mCapId);   

What are these error values doing for us? You just keep ignoring them and overwriting them

@@ +136,5 @@
> +  }
> +
> +}
> +
> +// VideoSessionConduit Impelmentation

sp.

@@ +142,5 @@
> +MediaSessionErrorCode 
> +  WebrtcVideoConduit::AttachRenderer(mozilla::RefPtr<VideoRenderer> aVideoRenderer)
> +{
> +  MLOG(PR_LOG_ERROR, VIDEO_SESSION <<"AttachRenderer");
> +  if(aVideoRenderer)

I would use this to be .get(). That way it will work when we change the semantics of RefPtr

@@ +147,5 @@
> +  {
> +    mRenderer = aVideoRenderer;
> +    if(!mEnginePlaying)
> +    {
> +      if(mPtrViERender->StartRender(mChannel) == -1)

Is there a race condition if the renderer is attached before there is media?

@@ +149,5 @@
> +    if(!mEnginePlaying)
> +    {
> +      if(mPtrViERender->StartRender(mChannel) == -1)
> +      {
> +          MLOG(PR_LOG_ERROR, VIDEO_SESSION <<"Starting the Renderer Fail");

Fix message so it's grammatcial.

@@ +166,5 @@
> +MediaSessionErrorCode 
> +  WebrtcVideoConduit::AttachTransport(mozilla::RefPtr<TransportInterface> aTransport)
> +{
> +  MLOG(PR_LOG_ERROR, VIDEO_SESSION <<"AttachTransport ");
> +  if(aTransport)

aTransport.get()

@@ +244,5 @@
> +  int error = 0;
> +  webrtc::VideoCodec video_codec;
> +  memset(&video_codec, 0, sizeof(webrtc::VideoCodec));
> +
> +  //Set all the codecs for now .. 

Am I missing something, or are you ignoring codecConfig?

@@ +300,5 @@
> +         				                        VideoType video_type,
> +         				                        uint64_t capture_time)
> +{
> +
> +  if(!initDone) 

This is a can't happen.

@@ +422,5 @@
> +    }
> +
> +   } else 
> +   {
> +      return kVideoTransportRegistrationFail;

Is this right? It seems to be a random positive number. IS that what WebRTC expects?

@@ +441,5 @@
> +    }
> +
> +   } else 
> +   {
> +      return kVideoTransportRegistrationFail;

See above.

::: media/webrtc/signaling/media_session/VideoSession.h
@@ +29,5 @@
> + * Concrete class for Video session. Hooks up  
> + *  - media-source and target to external transport 
> + */
> +class WebrtcVideoConduit  : public VideoSessionConduit			
> +	      		                ,public webrtc::Transport

Comma goes on previous line. Also, fix indent.

@@ +36,5 @@
> +
> +public:
> +
> +  //typedefs for shortcuts
> +  typedef webrtc::ViEBase     Base;

I'm not a huge fan of this. Why not just do "using webrtc::VieBase" and then you can use VieBase

@@ +51,5 @@
> +  virtual MediaSessionErrorCode ReceivedRTCPPacket(const void *data, int len);
> +  virtual MediaSessionErrorCode ConfigureSendMediaCodec(const VideoCodecConfig* codecInfo);
> +  virtual MediaSessionErrorCode ConfigureRecvMediaCodec(const VideoCodecConfig* codecInfo);
> +  virtual MediaSessionErrorCode AttachTransport(mozilla::RefPtr<TransportInterface> aTransport);
> +  virtual MediaSessionErrorCode SendVideoFrame(unsigned char* video_frame,

Fix indent.

@@ +73,5 @@
> +  virtual int SendRTCPPacket(int channel, const void *data, int len) ;
> +
> +	
> +   // ViEExternalRenderer implementation
> +  virtual int FrameSizeChange(

Fix indent.

@@ +77,5 @@
> +  virtual int FrameSizeChange(
> +        unsigned int, unsigned int, unsigned int
> +  	);
> +
> + virtual int DeliverFrame(

Fix indent.

@@ +83,5 @@
> +  	);
> +
> +
> + explicit WebrtcVideoConduit():
> + mVideoEngine(WebRTCEngineWrapper::Instance()->GetVideoEngine())

Commas go at the end of the line, IMO

@@ +120,5 @@
> +  mozilla::RefPtr<TransportInterface> mTransport;
> +  mozilla::RefPtr<VideoRenderer> mRenderer; 
> +
> +  ScopedCustomReleasePtr<webrtc::ViEBase> mPtrViEBase;
> +  ScopedCustomReleasePtr<webrtc::ViECapture> mPtrViECapture;

Where is the defn for this?

@@ +127,5 @@
> +  ScopedCustomReleasePtr<webrtc::ViERender> mPtrViERender;
> +  webrtc::ViEExternalCapture*  mPtrExtCapture;
> +
> +  bool mEnginePlaying;
> +  bool initDone;

Why is this needed?

::: media/webrtc/signaling/media_session/VideoTypes.h
@@ +1,1 @@
> +#ifndef VIDEO_TYPE_

These seem like they need comments. Also, I note no copyright on this. Where did they come from?

::: media/webrtc/signaling/media_session/logging.h
@@ +1,2 @@
> +/* 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,

Please don't do this. If oyu are going to use mlog, then #include the right file, dont' copy it.

Comment 19

[Suhas]

Attachment: Testing Indentation

Comment 20

[Suhas]

Attachment: Dummy Patch

Comment 21

[Suhas]

Attachment: MediaConduit patch for mtransport and PeerConnection.

Comment 22

[Suhas]

Incorporated almost all the comments .. Will work on few implementation details,synchronization of WebRTCEngineWrapper and fix up test-cases

Comment 23

[Ethan Hugg [:ehugg]]

Comment on attachment 642153 [details]
MediaConduit patch for mtransport and PeerConnection.

Review of attachment 642153 [details]:
-----------------------------------------------------------------

Almost there, just a few nits.

::: media/webrtc/signaling/media-conduit/AudioConduit.cpp
@@ +13,5 @@
> +
> +
> +mozilla::RefPtr<AudioSessionConduit> AudioSessionConduit::Create()
> +{
> +  CSFLogDebug(logTag,  "%s Create", __FUNCTION__);

I believe this will print "Create Create"

@@ +22,5 @@
> +  } 
> +  
> +  if(obj->Init() == kMediaConduitNoError)
> +  {
> +      return obj;

Should be indented same as the rest

::: media/webrtc/signaling/media-conduit/AudioConduit.h
@@ +83,5 @@
> +                      mEnginePlaying(false),
> +                      mChannel(-1),
> +                      sessionId(-1)
> +  {
> +  	printf("\n WebrtcAudioConduit : Constructor ");

Printf still here.  Do a grep for printfs

::: media/webrtc/signaling/media-conduit/MediaEngineWrapper.cpp
@@ +14,5 @@
> +  if(_instance)
> +  {
> +     return _instance;
> +  } else {
> +    _instance = new WebRTCEngineWrapper();

Perhaps put a thread-safe TODO here.  The standard pattern would be to got a lock, check for it being NULL again and then create it.  We can discuss whether that's necessary later, and put a TODO for now.

::: media/webrtc/signaling/media-conduit/VideoConduit.cpp
@@ +85,5 @@
> +  CSFLogDebug(logTag, " Engine Created: Init'ng the interfaces "); 
> +
> +  if(!mPtrViEBase || (mPtrViEBase->Init() == -1 ) )
> +  {
> +    CSFLogError(logTag, "Vide Engine Init Failed %d ", 

s/Vide/Video

@@ +280,5 @@
> +
> +  if(mPtrViEBase->StartReceive(mChannel) == -1)
> +  {
> +    error = mPtrViEBase->LastError();
> +    if(error == 12008)

One last magic error return code.

Comment 24

[Suhas]

Attachment: MediaConduit Patch for mtransport and peer connecton

Comment 25

[Ethan Hugg [:ehugg]]

Comment on attachment 642168 [details]
MediaConduit Patch for mtransport and peer connecton

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

Comment 26

[Eric Rescorla (:ekr)]

Can you please commit the test harness for this?

Comment 27

[Suhas]

Attachment: Media Conduit Abstraction Test Harness

Comment 28

[Suhas]

Attachment: Add missing unit test file

Comment 29

[Ethan Hugg [:ehugg]]

Comment on attachment 642674 [details]
Add missing unit test file

Review of attachment 642674 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/test/resource_mgr.h
@@ +5,5 @@
> + *  that can be found in the LICENSE file in the root of the source
> + *  tree. An additional intellectual property rights grant can be found
> + *  in the file PATENTS.  All contributing project authors may
> + *  be found in the AUTHORS file in the root of the source tree.
> + */

Much of this file seems to be lifted from media/webrtc/trunk/test/testsupport/fileutils.cc, or perhaps they have a mutual parent.  We need to change it or figure out how to properly attribute it.

Comment 30

[Suhas]

(In reply to Ethan Hugg [:ehugg] from comment #29)
> Comment on attachment 642674 [details]
> Add missing unit test file
> 
> Review of attachment 642674 [details]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/test/resource_mgr.h
> @@ +5,5 @@
> > + *  that can be found in the LICENSE file in the root of the source
> > + *  tree. An additional intellectual property rights grant can be found
> > + *  in the file PATENTS.  All contributing project authors may
> > + *  be found in the AUTHORS file in the root of the source tree.
> > + */
> 
> Much of this file seems to be lifted from
> media/webrtc/trunk/test/testsupport/fileutils.cc, or perhaps they have a
> mutual parent.  We need to change it or figure out how to properly attribute
> it.

Yes Ethan . this is taken directly from WebRTC test and removed unnecessary things .. I have used WebRTC copyright instead of Mozilla one ... should that d the work ?

Comment 31

[Ethan Hugg [:ehugg]]

Attachment: Crypts fix to unblock EKR

Comment 32

[Suhas]

(In reply to Ethan Hugg [:ehugg] from comment #31)
> Created attachment 644420 [details]
> Crypts fix to unblock EKR

Ekr a new patch has been upload with most of the comments from earlier and new design implemented .. I am on a poor wi-fi connection and couldnot get latest alder downloaded .. I will upload a new one with few more cosmetic changes later tonite .. the one uploaded one should be good for you to work with

Comment 33

[Eric Rescorla (:ekr)]

Please upload the unit tests as well.

Comment 34

[Suhas]

Also sample test-file is pastebin'ed at http://pastebin.mozilla.org/1709620 ... I will upload a patch with final test alogn with the above patch

Comment 35

[Ethan Hugg [:ehugg]]

Attachment: Crypts in-progress test patch

Comment 36

[Ethan Hugg [:ehugg]]

Comment on attachment 644426 [details]
Crypts in-progress test patch

Review of attachment 644426 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/test/Makefile.in
@@ -70,5 @@
> -
> -ifeq (gtk2,$(MOZ_WIDGET_TOOLKIT))
> -LIBS += \
> -  $(XLIBS) \
> -  $(MOZ_GTK2_LIBS) \

this is the inadvertent rollback of the Fedora link fix.  Just a reminder not to take this stuff out on next update.

Comment 37

[Suhas]

Attachment: New Design and Improved Error Handling

Comment 38

[Suhas]

Attachment: Basic Test-cases for MediaConduits

Comment 39

[Eric Rescorla (:ekr)]

Comment on attachment 644691 [details] [diff] [review]
New Design and Improved Error Handling

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

I have reviewed the audio portions and the interface. 

Please fix the things I've noted, check for related issues in the video, and fix the formatting throughout.

I am finding the logic around the states very confusing. What does mPlaying, etc. mean? It appears
that you can be playing in either send or receive? Huh?

::: media/webrtc/signaling/media-conduit/AudioConduit.cpp
@@ +28,2 @@
>      return obj;
>    }else

Funny formatting.

do either:
}
else {

or 
} else {

@@ +66,5 @@
>      mPtrVoEBase->StopSend(mChannel);
>      mPtrVoEBase->StopReceive(mChannel);
>      mPtrVoEBase->DeleteChannel(mChannel);
>      mPtrVoEBase->Terminate();
> +    webrtc::VoiceEngine::Delete(mVoiceEngine);

What happens if mVoiceEngine is NULL?

@@ +77,5 @@
>  MediaConduitErrorCode WebrtcAudioConduit::Init()
>  {
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
>  
>    if(!mVoiceEngine)

The error checking here seems problematic. If mVoiceEngine is nonzero, you're going to get a crash in the initializers, no? Why don't you just do all the real construction here?

@@ +129,5 @@
>    return kMediaConduitNoError;
>  }
>  
>  
>  // AudioSessionConduit Impelmentation

Spell.

@@ +145,4 @@
>    }
> +
> +  //Set the renderer
> +  mRenderer = aAudioRenderer.get();

Don't use .get() here. You can assign directly.

@@ +162,4 @@
>    }
> +
> +  // set the transport
> +  mTransport = aTransport.get();

Same as above.

@@ +186,4 @@
>    }
>  
> +  //are we transmitting already
> +  if(mEngineTransmitting)

Check to see if the codec is the same. In that case you can just return immediately, right?

@@ +195,5 @@
> +      if(error == VE_NOT_SENDING)
> +      {
> +        CSFLogDebug(logTag, "%s StopSend() Success ", __FUNCTION__);
> +        mEngineTransmitting = false;
> +      }else

Formatting.

@@ +209,3 @@
>  
> +  memset(&cinst, 0, sizeof(webrtc::CodecInst));
> +  memcpy(cinst.plname, codecConfig->mName.c_str(), CODEC_PLNAME_SIZE);

This is a memory overread. Consider what happens if mName.size() == 10 and CODEC_PLNAME_SIZE = 100000

@@ +237,5 @@
>      if(error == VE_ALREADY_SENDING)
> +    {
> +      return kMediaConduitSendingAlready;
> +    }
> +      return kMediaConduitUnknownError;

Indentation.

@@ +262,3 @@
>    }
>  
> +  // are we receiving already

Consider the same comments as above.

@@ +350,5 @@
> +  
> +  //validate params
> +  if(!audio_data || (lengthSamples <= 0) ||
> +                    (IsSamplingFreqSupported(samplingFreqHz) == false) ||
> +                    ((lengthSamples % (samplingFreqHz / 100) != 0)) ||

What does this test do?

@@ +351,5 @@
> +  //validate params
> +  if(!audio_data || (lengthSamples <= 0) ||
> +                    (IsSamplingFreqSupported(samplingFreqHz) == false) ||
> +                    ((lengthSamples % (samplingFreqHz / 100) != 0)) ||
> +                    (capture_delay < 0) )

How can this happen if capture_delay is unsigned?

@@ +369,1 @@
>    if(!mEnginePlaying)

Why can't I insert the samples if I am not playing?

These things should be able to go one-way.

@@ +393,2 @@
>       }
> +       return kMediaConduitUnknownError;

Indent.

@@ +409,5 @@
> +  unsigned int numSamples = 0;
> +  
> +  //validate params
> +  if(!speechData || (numSamples =
> +                        GetNum10msSamplesForFrequency(samplingFreqHz) == 0) ||

Add parentheses here rather than relying on precedence. Better yet, break up this statement.

@@ +410,5 @@
> +  
> +  //validate params
> +  if(!speechData || (numSamples =
> +                        GetNum10msSamplesForFrequency(samplingFreqHz) == 0) ||
> +                    (capture_delay < 0) )

can't happen because it's unsigned.

@@ +427,3 @@
>  
> +  //Attempt to start the engine, if it is not started already
> +  if(!mEnginePlaying)

This seems too confusing. Enforce correct calling order, don't compensate for it.

@@ +445,5 @@
>    {
>      if(mPtrVoEXmedia->ExternalPlayoutGetData((WebRtc_Word16*) speechData, 
>                                                (int)samplingFreqHz,
>                                                capture_delay, 
>                                                (int&)lengthSamples) == -1)

What the heck is going on here? You can't cast from (unsigned int) to (int). Moreover, you shouldn't be using C-style casts in C++ code. Please reexamine all of this.

::: media/webrtc/signaling/media-conduit/AudioConduit.h
@@ +65,1 @@
>    virtual MediaConduitErrorCode GetAudioFrame(int16_t speechData[],

Please document all of these arguments.

Is lengthSamples an in or an out? Both?

@@ +74,5 @@
>  
>  	
>  	
>    WebrtcAudioConduit():
> +                      mVoiceEngine(webrtc::VoiceEngine::Create()),

What happens if this fails?

@@ +98,5 @@
>    void operator=(WebrtcAudioConduit const&) { }
> + 
> +  bool IsSamplingFreqSupported(int freq)
> +  {
> +    if(freq == 16000 || freq == 32000 || freq == 44000 || freq == 48000)

I would replace this with a switch.

@@ +102,5 @@
> +    if(freq == 16000 || freq == 32000 || freq == 44000 || freq == 48000)
> +    {
> +      return true;
> +    }
> +      return false;

Indentation.

@@ +105,5 @@
> +    }
> +      return false;
> +  }
> +
> +  unsigned int GetNum10msSamplesForFrequency(int samplingFreqHz)

Why don't you just have IsSamplingFreqSupported() call this and return true if the result is nonzero?

::: media/webrtc/signaling/media-conduit/MediaConduitInterface.h
@@ +76,4 @@
>     * @param buffer: pointer to decoded video frame
>     * @param buffer_size: size of the decoded frame
>     * @param time_stamp: Decoder timestamp, typically 90KHz as per RTP
>     * @render_time: Wall-clock time at the decoder for synchronizartion 

Un what units?

@@ +126,2 @@
>  
>     NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AudioRenderer)

Why is there still an AudioRenderer class?

@@ +165,5 @@
>  
>    /**
>     * Function to attach Transport end-point of the Media conduit.
>     * @param aTransport: Reference to the concrete teansport implementation 
> +   * Note: Multiple invocations of this call , replaces exisintg transport with

Spelling.

@@ +196,5 @@
>  
>    /**
>     * Function to attach Renderer end-point of the Media-Video conduit.
>     * @param aRenderer : Reference to the concrete Video renderer implementation 
> +   * Note: Multiple invocations of this API shall remove an exsiting renderer 

Spelling.

@@ +226,5 @@
>     * Function to configure send codec for the video session
>     * @param sendSessionConfig: CodecConfiguration
> +   * @result: On Success, the video engine is configured with passed in codec for send
> +   *          On failure, video engine transmit functionality is disabled.
> +   * NOTE: This API can be invoked multitple time. Invoking this API may involve restarting

Spelling.

@@ +264,5 @@
>    virtual ~AudioSessionConduit() = 0; 
>  
>    /**
>     * Function to attach Renderer end-point of the Media-Video conduit.
>     * @param aRenderer : Reference to the concrete Video renderer

This isn't a reference, really. It's a RefPtr.

Comment 40

[Suhas]

Attachment: Incorporated Feedback from Eric

Comment 41

[Suhas]

Uploaded new patch with comments incorporated for both Audio and Video Conduits.

Comment 42

[Suhas]

Attachment: Updated test-case to new API

Comment 43

[Suhas]

Attachment: Test Harness update-Fixed Makefile missing links

Comment 44

[Eric Rescorla (:ekr)]

Comment on attachment 644712 [details] [diff] [review]
Incorporated Feedback from Eric

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

::: media/webrtc/signaling/media-conduit/AudioConduit.cpp
@@ +200,5 @@
> +                                               codecConfig->mPacSize,
> +                                               codecConfig->mChannels,
> +                                               codecConfig->mRate);
> +  } else {
> +    memcpy(mCurSendCodecConfig, codecConfig, sizeof(AudioCodecConfig));

AFAIK, you cannot memcpy() one std::string onto another, which is a sife effect of what you are doing here.

@@ +225,5 @@
>  
> +  mEngineTransmitting = false;
> +
> +  memset(&cinst, 0, sizeof(webrtc::CodecInst));
> +  plNameSafeLength = !(CODEC_PLNAME_SIZE < codecConfig->mName.length()) 

This doesn't look right. The name should always fit into the buffer.

Why don't you simply check that the length is < CODEC_PLNAME_SIZE and if not, throw an error. If it is, then copy the actual data and make sure it's null terminated. As it is, this code appears to truncate the string, though it's confusing because of the check above.

@@ +382,5 @@
>  {
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +  
> +  // Non null audio buffer pointer, invalid sampling frequency,
> +  // and invlid lengthSamples result in falure. 

Spelling.

@@ +387,5 @@
> +  // Support sampling frequencies - 16, 32, 44, 48 KHz and supported
> +  // sample size for a 10 ms frame is 160, 320, 440 and 480 respectively.
> +  if(!audio_data || (lengthSamples <= 0) ||
> +                    (IsSamplingFreqSupported(samplingFreqHz) == false) ||
> +                    ((lengthSamples % (samplingFreqHz / 100) != 0)) )

What does this last test do. Please comment.

@@ +457,5 @@
> +  lengthSamples = 0;  //output paramter
> +
> +  memset(speechData, 0, numSamples);
> +  
> +  if(mPtrVoEXmedia->ExternalPlayoutGetData((WebRtc_Word16*) speechData, 

No C style casts please.

@@ +458,5 @@
> +
> +  memset(speechData, 0, numSamples);
> +  
> +  if(mPtrVoEXmedia->ExternalPlayoutGetData((WebRtc_Word16*) speechData, 
> +                                            samplingFreqHz,

You still have a signed/unsigned conversion error here. samplingFreqHz is unsigned coming in.

Every time you change types and there is a chance of range issues, you need to check.

@@ +459,5 @@
> +  memset(speechData, 0, numSamples);
> +  
> +  if(mPtrVoEXmedia->ExternalPlayoutGetData((WebRtc_Word16*) speechData, 
> +                                            samplingFreqHz,
> +                                            capture_delay, 

Another signed/unsigned conversion error, which is doubly relevant since you are onverting a uint64_t to an int. For that matter, why is this a uint64_t? Under what circumstances could the estimated delay possibly be greater than a million seconds?

::: media/webrtc/signaling/media-conduit/MediaConduitInterface.h
@@ +256,5 @@
> +   * / playoutof length 10 milliseconds.
> +   * 
> +   * @param speechData [in]: Pointer to a array to which a 10ms frame of audio will be copied
> +   * @param samplingFreqHz [in]: Frequency of the sampling for playback in Hertz (16000, 32000,..)
> +   * @param capture_delay [in]: Estimated Time between reading of the samples to rendering/playback

In ms?

::: media/webrtc/signaling/media-conduit/VideoConduit.cpp
@@ +474,5 @@
> +    CSFLogError(logTag, "%s Engine not transmitting ", __FUNCTION__);
> +    return kMediaConduitSessionNotInited;
> +  }
> +  
> +  //insert the frame to video engine in I420 format only

check that video_type == 420.

::: media/webrtc/signaling/media-conduit/VideoConduit.h
@@ +143,3 @@
>                        mTransport(NULL),
>                        mRenderer(NULL),
>                        mPtrViEBase(ViEBase::GetInterface(mVideoEngine)),

Wha thappens if you call these with NULL?

Comment 45

[Suhas]

Attachment: Incorporated Feedback from Eric

Comment 46

[Suhas]

Attachment: updated to latest design changes

Comment 47

[Suhas]

Updated the code to incorporate feedback ..

Comment 48

[Eric Rescorla (:ekr)]

Comment on attachment 644819 [details] [diff] [review]
Incorporated Feedback from Eric

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

This isn't a complete review, but at minimum you need to fix the memcpy issue below.

More generally I just realized this API has an error in it. You need to be able to receive multiple codecs, not just one, but your code actually blows away the entire state every time SetRecvCodec is called.

The easiest thing to do is probably to allow SetRecvCodec() to take a std::vector<AudioCodecConfig *> and impose them all at once. Please change this, and fix the things I have noted and resubmit.
I also noticed that you missed at least one change from the previous review: checking that packets are in I420 format when they are passed to you. Please double-check that you fixed everything in the previous review.

::: media/webrtc/signaling/media-conduit/AudioConduit.cpp
@@ +199,5 @@
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
>  
> +  int error = 0;
> +  webrtc::CodecInst cinst;
> +  int plNameSafeLength = 0;

Unused variable.

@@ +255,5 @@
> +
> +  //zero fill the codec structure
> +  memset(&cinst, 0, sizeof(webrtc::CodecInst));
> +  // max size of codecConfig->mName() can be only 31 by here.
> +  memcpy(cinst.plname, codecConfig->mName.c_str(), sizeof(codecConfig->mName));

You're copying sizeof(std::string) here. That's not going to work.

@@ +298,5 @@
> +                                               codecConfig->mPacSize,
> +                                               codecConfig->mChannels,
> +                                               codecConfig->mRate);
> +  } else {
> +    mCurSendCodecConfig->mType     = codecConfig->mType;

You know you can just asisgn this, right?

@@ +316,5 @@
>  {
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +  int error = 0;
> +  webrtc::CodecInst cinst;
> +  int plNameSafeLength = 0;

Unused variable.

::: media/webrtc/signaling/media-conduit/VideoConduit.h
@@ +161,5 @@
>  
>  private:
>  
>    //Copy and Assigment
>    WebrtcVideoConduit(WebrtcVideoConduit const&) {}

Why are you even defining these?

@@ +162,5 @@
>  private:
>  
>    //Copy and Assigment
>    WebrtcVideoConduit(WebrtcVideoConduit const&) {}
>    void operator=(WebrtcVideoConduit const&) { }

Or this. Conventional approach is simply not to provide at all.

Comment 49

[Eric Rescorla (:ekr)]

Note that you only need to be able to send one codec at once. It's just receive that needs changing.

Comment 50

[Suhas]

(In reply to Eric Rescorla from comment #48)
> Comment on attachment 644819 [details] [diff] [review]
> Incorporated Feedback from Eric
> 
> Review of attachment 644819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This isn't a complete review, but at minimum you need to fix the memcpy
> issue below.
> 
> More generally I just realized this API has an error in it. You need to be
> able to receive multiple codecs, not just one, but your code actually blows
> away the entire state every time SetRecvCodec is called.
> 
> The easiest thing to do is probably to allow SetRecvCodec() to take a
> std::vector<AudioCodecConfig *> and impose them all at once. Please change
> this, and fix the things I have noted and resubmit.
> I also noticed that you missed at least one change from the previous review:
> checking that packets are in I420 format when they are passed to you. Please
> double-check that you fixed everything in the previous review.
> 
> ::: media/webrtc/signaling/media-conduit/AudioConduit.cpp
> @@ +199,5 @@
> >    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> >  


I was myself thinking of providing the API on setting the receive codecs. I shall add it
> > +  int error = 0;
> > +  webrtc::CodecInst cinst;
> > +  int plNameSafeLength = 0;
> 
> Unused variable.
> 
> @@ +255,5 @@
> > +
> > +  //zero fill the codec structure
> > +  memset(&cinst, 0, sizeof(webrtc::CodecInst));
> > +  // max size of codecConfig->mName() can be only 31 by here.
> > +  memcpy(cinst.plname, codecConfig->mName.c_str(), sizeof(codecConfig->mName));
> 
> You're copying sizeof(std::string) here. That's not going to work.
> 
> @@ +298,5 @@
> > +                                               codecConfig->mPacSize,
> > +                                               codecConfig->mChannels,
> > +                                               codecConfig->mRate);
> > +  } else {
> > +    mCurSendCodecConfig->mType     = codecConfig->mType;
> 
> You know you can just asisgn this, right?
> 
> @@ +316,5 @@
> >  {
> >    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +  int error = 0;
> > +  webrtc::CodecInst cinst;
> > +  int plNameSafeLength = 0;
> 
> Unused variable.
> 
> ::: media/webrtc/signaling/media-conduit/VideoConduit.h
> @@ +161,5 @@
> >  
> >  private:
> >  
> >    //Copy and Assigment
> >    WebrtcVideoConduit(WebrtcVideoConduit const&) {}
> 
> Why are you even defining these?
> 
> @@ +162,5 @@
> >  private:
> >  
> >    //Copy and Assigment
> >    WebrtcVideoConduit(WebrtcVideoConduit const&) {}
> >    void operator=(WebrtcVideoConduit const&) { }
> 
> Or this. Conventional approach is simply not to provide at all.

Comment 51

[Suhas]

Attachment: MediaConduit Version 3 Design Changes and Few nits

Comment 52

[Suhas]

Attachment: updated test-case for multiple codecs

Comment 53

[Suhas]

As far as i checked back , i should have incorporated all the comments .. If i a still missing one or two .. happy to fix them ..

Comment 54

[Suhas]

Attachment: MediaConduit Version 3 Design Changes and Few nits

Comment 55

[Suhas]

Attachment: updated test-case for multiple codecs

Comment 56

[Suhas]

Added new attachment with more unit test-cases for Codec Send and Recv APIs.

Comment 57

[Ethan Hugg [:ehugg]]

Comment on attachment 645203 [details] [diff] [review]
MediaConduit Version 3 Design Changes and Few nits

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

::: media/webrtc/signaling/signaling.gyp
@@ +65,5 @@
>        #
>        # SOURCES
>        #
>        'sources': [
>          # Media Conduit 

If you remove MediaEngineWrapper.cpp from the .gyp you should probably do an 'hg remove' on it as well.

Comment 58

[Suhas]

(In reply to Ethan Hugg [:ehugg] from comment #57)
> Comment on attachment 645203 [details] [diff] [review]
> MediaConduit Version 3 Design Changes and Few nits
> 
> Review of attachment 645203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/signaling.gyp
> @@ +65,5 @@
> >        #
> >        # SOURCES
> >        #
> >        'sources': [
> >          # Media Conduit 
> 
> If you remove MediaEngineWrapper.cpp from the .gyp you should probably do an
> 'hg remove' on it as well.


Thanks Ethan .. Shall do in the final patch after i collect all the feedback ..

Comment 59

[Suhas]

(In reply to Ethan Hugg [:ehugg] from comment #57)
> Comment on attachment 645203 [details] [diff] [review]
> MediaConduit Version 3 Design Changes and Few nits
> 
> Review of attachment 645203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/signaling.gyp
> @@ +65,5 @@
> >        #
> >        # SOURCES
> >        #
> >        'sources': [
> >          # Media Conduit 
> 
> If you remove MediaEngineWrapper.cpp from the .gyp you should probably do an
> 'hg remove' on it as well.


Thanks Ethan .. Shall do in the final patch after i collect all the feedback ..

Comment 60

[Eric Rescorla (:ekr)]

Comment on attachment 645203 [details] [diff] [review]
MediaConduit Version 3 Design Changes and Few nits

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

Please fix these minor issues and land.

::: media/webrtc/signaling/media-conduit/AudioConduit.cpp
@@ +258,5 @@
> +
> +  //zero fill the codec structure
> +  memset(&cinst, 0, sizeof(webrtc::CodecInst));
> +  // max size of codecConfig->mName() can be only 31 by here.
> +  memcpy(cinst.plname, codecConfig->mName.c_str(), codecConfig->mName.length());

This code is kind of scary, since you are copying an unterminated string and relying on the plname being zeroed by the memset above.

Add + 1 to the length in the memcpy. I would also replicate the length check above to make sure
they don't get separated.

@@ +293,3 @@
>    }
>  
> +  //copy the applied codec

Simplify this code by just doing a delete if mCurSendCodecConfig != 0 and then you can just call the constructor for both paths.

@@ +377,5 @@
> +
> +    mEngineReceiving = false;
> +    webrtc::CodecInst cinst;
> +     
> +    memset(&cinst, 0, sizeof(webrtc::CodecInst));

Refactor out this code so you only do the translation from our config to WebRTC once.

@@ +454,5 @@
> +  // 3. Appropriate Sample Length for 10 ms audio-frame. This represents
> +  //    block size the VoiceEngine feeds into encoder for passed in audio-frame 
> +  //    Ex: for 16000 sampling rate , valid block-length is 160
> +  //    Similarly for 32000 sampling rate, valid block length is 320
> +  //    We do the check by the varify modular operator below to be zero

verify?

@@ +724,5 @@
> +      curCodecConfig->mChannels == codecInfo->mChannels &&
> +      curCodecConfig->mRate == codecInfo->mRate)
> +  {
> +    return true;
> +  } else {

No need to ahve an else here, since the other side returns.

::: media/webrtc/signaling/media-conduit/MediaConduitInterface.h
@@ +203,5 @@
>    /**
>     * Function to configure receive codec for the video session
>     * @param sendSessionConfig: CodecConfiguration
> +   * NOTE: This API can be invoked multiple time. Invoking this API may involve restarting
> +   *        reception sub-system on the engine

s/reception/sending/

@@ +209,2 @@
>     */
> +  virtual MediaConduitErrorCode ConfigureRecvMediaCodec(

Codecs (plural) here and below.

@@ +243,5 @@
> +                                    samples of 16-bits each for a 10m audio frame.
> +   * @param samplingFreqHz [in]: Frequency/rate of the sampling in Hz ( 16000, 32000 ...)
> +   * @param capture_delay [in]:  Approx Delay from recording until it is delivered to VoiceEngine
> +                                 in milliseconds.
> +   * NOTE: ConfigureSendMediaCodec() SHOULD be called before this function can be invoked

SHOULD or MUST? What happens if you don't?

::: media/webrtc/signaling/media-conduit/VideoConduit.cpp
@@ +296,5 @@
>  
> +  //reset the flag
> +  mEngineTransmitting = false;
> +  
> +  // we shud be good here to set the new codec.

should

Comment 61

[Suhas]

Attachment: MediaConduit Version 3 Design Changes and Few nits

Comment 62

[Suhas]

Attachment: updated test-case for multiple codecs

Comment 63

[Ethan Hugg [:ehugg]]

Comment on attachment 645457 [details] [diff] [review]
MediaConduit Version 3 Design Changes and Few nits


Pushed to Alder branch - http://hg.mozilla.org/projects/alder/rev/6113f89f82e0

Comment 64

[Ethan Hugg [:ehugg]]

Comment on attachment 645459 [details] [diff] [review]
updated test-case for multiple codecs

Pushed to Alder branch - http://hg.mozilla.org/projects/alder/rev/4017d6d48e77

Comment 65

[Suhas]

Attachment: Added Error Checks On Audio playout to overcome sound card errors as asserts

Comment 66

[Suhas]

Attachment: AudioPlayout Error Fix

Comment 67

[Suhas]

Attachment: AudioPlayout Error Fix

Comment 68

[Ethan Hugg [:ehugg]]

Comment on attachment 645867 [details] [diff] [review]
AudioPlayout Error Fix

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

::: media/webrtc/signaling/test/webrtc_standalone_test.cpp
@@ +181,5 @@
>      Init();
> +    if(!initDone)
> +    {
> +
> +      printf("Init Failed. Error is  \n %s", lastError.c_str());

I doubt you want the \n in the middle of this string.  You could probably simplify these buy using c++ streams instead.  

"cout << "Init failed. Error is " << lastError << endl;"

@@ +257,5 @@
>  
> +    } else if(choice == 1)
> +    {
> +      FILE* id = fopen(fileToPlay.c_str(),"rb");
> +      ASSERT_TRUE(id != NULL);

ASSERT_NE() exists for this.

@@ +270,5 @@
>            finish = true;
>            break;
>          }
> +        mPtrVoEXmedia->ExternalRecordingInsertData(audio,160,16000,10);
> +        usleep(10*1000);

You might consider changing this to a PR_Sleep(PR_MillisecondsToInterval()) so it's portable.

Comment 69

[Suhas]

Attachment: AudioPlayout Error Fix

Comment 70

[Ethan Hugg [:ehugg]]

Comment on attachment 645911 [details] [diff] [review]
AudioPlayout Error Fix

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

Did you mean to put me on the feedback list twice?

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +407,5 @@
>      audioTester.Init(mAudioSession,mAudioSession2, fileToPlay,fileToRecord);
>      printf("\n ********************************************************\n");
>      printf("\n           Generating Samples for 6 seconds ");
>      printf("\n *********************************************************\n");
> +    PR_Sleep(2);

PR_Sleep is in tics.  I think maybe you want PR_Sleep(PR_SecondsToInterval())

::: media/webrtc/signaling/test/webrtc_standalone_test.cpp
@@ +145,5 @@
>  
> +    if(!initDone)
> +    {
> +
> +      printf("Init Failed. Error is  %s", lastError.c_str());

So, is your plan to stick with printf?  Most of your statements start with an endl, some have one at the end, this one has neither.

Comment 71

[Suhas]

Attachment: AudioPlayout Error Fix

Comment 72

[Suhas]

Attachment: AudioPlayout Error Fix

Comment 73

[Ethan Hugg [:ehugg]]

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