806151 - Peer Connection - Populate Codecs Dynamically from the Media Engine

Status: RESOLVED WORKSFORME

(Core :: WebRTC: Signaling, defect, P2)

Description

[Suhas]

Peer Connection needs a way to dynamically query the voice and video engine for codec list.

Comment 1

[Suhas]

Attachment: Populate Codecs Dynamically - PeerConnection

Comment 2

[Suhas]

Comment on attachment 675909 [details] [diff] [review]
Populate Codecs Dynamically - PeerConnection

Scoped versions of media engine classes are added to enable querying attributes off the engine.
The same has been hooked up to peer-connection to retrieve codec list during setup and removed hard-coded codec bit mask.

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 675909 [details] [diff] [review]
Populate Codecs Dynamically - PeerConnection

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

::: media/webrtc/signaling/src/media-conduit/MediaEngineWrapper.cpp
@@ +49,5 @@
> +                 mPtrVoECodec(VoECodec::GetInterface(mEngine));
> +
> +  for(int idx=0; idx < mPtrVoECodec->NumOfCodecs(); idx++)
> +  {
> +    if(0 == mPtrVoECodec->GetCodec(idx, audio_codec))

I think the Moz style might be if (!mPtrVoeCodec...

::: media/webrtc/signaling/src/media-conduit/MediaEngineWrapper.h
@@ +38,5 @@
> +
> +	/**
> +   * For a given media-engine retrieve the names of
> +   * codecs suported.
> +   */

This is newly added code that looks like it contains tabs.  Let's not add any more tabs.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +64,1 @@
>    codecMask = 0;

This line can be removed now

@@ +151,5 @@
> +        for(std::vector<std::string>::size_type i=0;i < codecs.size();i++)
> +        {
> +          codecMask |= GetCodecBitMask(codecs[i]);
> +        }
> +        return codecMask;

I would be tempted to make these breaks instead since you're returning right below.  Also a default: with an error case, perhaps MOZ_ASSERT.

Comment 4

[Suhas]

Attachment: Populate Codecs Dynamically - PeerConnection

Comment 5

[Suhas]

Attachment: Populate Codecs Dynamically - PeerConnection

Comment 6

[Suhas]

Attachment: Populate Codecs Dynamically - PeerConnection

Comment 7

[Suhas]

Attachment: Populate Codecs Dynamically - PeerConnection

Comment 8

[Suhas]

Attachment: Populate Codecs Dynamically - PeerConnection

Comment 9

[Suhas]

Comment on attachment 676817 [details] [diff] [review]
Populate Codecs Dynamically - PeerConnection

Fixed comments from ehugg.

Comment 10

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

Comment on attachment 676817 [details] [diff] [review]
Populate Codecs Dynamically - PeerConnection

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

WIth the *current* implementation, it's tempting to make this generic (make it a template), as audio and video are virtually the same.  However, they may diverge in the future, and if they diverge than the template may make the instantiation more annoying - but still, it's worth considering.

A more pertinent issue: What's the overhead of creating and destroying these engine instances all the time just to get a single code list, for example?  Especially as we're opening the engine elsewhere as well; should this just be added to those and handled via some shared resource attached to one of the PeerConnection backends?  GetUserMedia also is probably opening it, though we may not want to share with that.

r+ as I believe this will do the job, but I'd like to see those questions answered/discussed.

::: media/webrtc/signaling/src/media-conduit/MediaEngineWrapper.cpp
@@ +15,5 @@
> +
> +MediaEngineInterface* MediaEngineFactory::CreateAudioEngine()
> +{
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +#if defined(MOZ_WEBRTC)

Under what circumstance does this get built with MOZ_WEBRTC not set?  And if it can, then a bunch more stuff needs to be #if'd

::: media/webrtc/signaling/src/media-conduit/MediaEngineWrapper.h
@@ +41,5 @@
> +  virtual void Codecs(std::vector<std::string>* codecList) = 0;
> +};
> +
> +/**
> + * WebRTC Voice Engine Absrtaction.

Abstraction

@@ +51,5 @@
> +{
> +public:
> +  ScopedWebRTCVoiceEngine(): mEngine(nullptr)
> +  {
> +    mEngine = webrtc::VoiceEngine::Create();

mEngine(nullptr) is useless here as we'll always init it from ::Create()

@@ +63,5 @@
> +  }
> +
> +  virtual void Codecs(std::vector<std::string>* codecList);
> + private:
> +  //Engine Instance

the comment isn't actually useful

@@ +68,5 @@
> +  webrtc::VoiceEngine* mEngine;
> +};
> +
> +/**
> + * WebRTC Video Engine Absrtaction.

ditto spelling

@@ +77,5 @@
> +class ScopedWebRTCVideoEngine : public MediaEngineInterface {
> +public:
> +  ScopedWebRTCVideoEngine(): mEngine(nullptr)
> +  {
> +    mEngine = webrtc::VideoEngine::Create();

mEngine(nullptr) is useless here as we'll always init it from ::Create()

Comment 11

[Suhas]

(In reply to Randell Jesup [:jesup] from comment #10)
> Comment on attachment 676817 [details] [diff] [review]
> Populate Codecs Dynamically - PeerConnection
> 
> Review of attachment 676817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> WIth the *current* implementation, it's tempting to make this generic (make
> it a template), as audio and video are virtually the same.  However, they
> may diverge in the future, and if they diverge than the template may make
> the instantiation more annoying - but still, it's worth considering.
> 

> A more pertinent issue: What's the overhead of creating and destroying these
> engine instances all the time just to get a single code list, for example? 
> Especially as we're opening the engine elsewhere as well; should this just
> be added to those and handled via some shared resource attached to one of
> the PeerConnection backends?  GetUserMedia also is probably opening it,
> though we may not want to share with that. 
> r+ as I believe this will do the job, but I'd like to see those questions
> answered/discussed.
> 
> ::: media/webrtc/signaling/src/media-conduit/MediaEngineWrapper.cpp
> @@ +15,5 @@
> > +
> > +MediaEngineInterface* MediaEngineFactory::CreateAudioEngine()
> > +{
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +#if defined(MOZ_WEBRTC)
> 
> Under what circumstance does this get built with MOZ_WEBRTC not set?  And if
> it can, then a bunch more stuff needs to be #if'd
> 
> ::: media/webrtc/signaling/src/media-conduit/MediaEngineWrapper.h
> @@ +41,5 @@
> > +  virtual void Codecs(std::vector<std::string>* codecList) = 0;
> > +};
> > +
> > +/**
> > + * WebRTC Voice Engine Absrtaction.
> 
> Abstraction
> 
> @@ +51,5 @@
> > +{
> > +public:
> > +  ScopedWebRTCVoiceEngine(): mEngine(nullptr)
> > +  {
> > +    mEngine = webrtc::VoiceEngine::Create();
> 
> mEngine(nullptr) is useless here as we'll always init it from ::Create()
> 
> @@ +63,5 @@
> > +  }
> > +
> > +  virtual void Codecs(std::vector<std::string>* codecList);
> > + private:
> > +  //Engine Instance
> 
> the comment isn't actually useful
> 
> @@ +68,5 @@
> > +  webrtc::VoiceEngine* mEngine;
> > +};
> > +
> > +/**
> > + * WebRTC Video Engine Absrtaction.
> 
> ditto spelling
> 
> @@ +77,5 @@
> > +class ScopedWebRTCVideoEngine : public MediaEngineInterface {
> > +public:
> > +  ScopedWebRTCVideoEngine(): mEngine(nullptr)
> > +  {
> > +    mEngine = webrtc::VideoEngine::Create();
> 
> mEngine(nullptr) is useless here as we'll always init it from ::Create()

These are couple of things that were on my mind when creating these classes
1. At present GUM and Media-Conduit create their own instances of Voice and Video Engine instances than going through a factory instance like the one above .. Should those backends go through some thing like this to handle engine instances ?

2. Idea behind Codecs() list operation was , in future we might need to add more functions on these wrappers to enable more functionality than is presented today.

3. The way PeerConnectionObject deals with voice/video instances is via Media-Conduit which is not created until we receive an offer/answer that is triggered via creation of the media pipeline. The requirement  for setting the codec list (audio and video) happens during PeerConnection::Init() by when there is no engine handle to ask for list of supported codecs.

4. If we plan to hook up some APIs over to Media-Conduit to ask for getting similar information, we need to go over the point 3 above and place creation of media-conduit when PeerConnection Object is created which might not be right design since we end up creating send and receive media-pipeline w/o even parsing offer and the answer.

5. There is definitely overhead associated with the current design since every time a ScopedWebRTCVoice/VideoEngine is created, we create & destroy engine instances. Every-time we called Codecs() we acquire and release Interface pointers. But with more functionality added to this interface and eventually moving the engine-related operations to a wrapper classes from different webrtc consumers (media-conduit + Gum) should be fine.


I am open for ideas and suggestions in this matter and how do we want to take this further.

Comment 12

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

Well, instantiating VoiceEngine/VoiceEngineImpl (and ditto Video) instantiates a ton of superclasses; just looking at VoiceEngineImpl I see ~14 public superclasses.

Normally I'd be leery of this, though glancing through the classes most do nothing at creation time but store a pointer to the shared data; a few do a CreateCriticalSection(), the hardware class sets up a CPU monitor (but that mean spawning a thread - may be slow-ish, especially on Windows, and a bunch of critical sections/vars), etc.

So this may not actually be an issue, but it makes me concerned.

The alternative is to have ownership of the VoiceEngine/VideoEngines be in a wrapper class that is created and attached to the PeerConnection (and then used by the codeclist and conduits), or in a single instance per browser.  For example, I'm not sure if it's good if we have N CPU monitor threads running, since it seems to create one per VoiceEngine (and probably VideoEngine).

So, the interesting step would be to do an strace (or other trace) starting at the Voice/Video engine creation call, and through the destruction of it to return the codec list (and even better, a profile).

This is an important architectural element and design decision.  (CC-ing various people; Anant has thought about this and may have some comments.)

Comment 13

[Eric Rescorla (:ekr)]

Jesup, 

I share your concerns. Is there a reason why this can't be taken care of with a single precomputation?

Comment 14

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

Yes, exactly - the codec list is a compile-time option, so returning a fixed list to match that build-config list is totally reasonable.  Then we wouldn't have to instantiate the entire system twice before we *really* do it.  Just make a static function enumerator that returns the build list of codecs (for now, at least).

Comment 15

[Adam Roach [:abr]]

I'm not sure we want to close this bug yet.

Unless I missed some relevant conversation somewhere, the current course of action that was settled on above wasn't "leave the hard coded list of codecs as-is". It was "don't calculate the list more than necessary," right?

I don't think we're quite where we want to be yet. Jesup? Ekr?

Comment 16

[Eric Rescorla (:ekr)]

Comment on attachment 676817 [details] [diff] [review]
Populate Codecs Dynamically - PeerConnection

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

This will need an eventual rewrite once the h.264 code is more nailed down.