Currently we offer a fixed codec list. We need to autodiscover.

Jingle's SDP code is in talk/app/webrtc/webrtcsdp.cc, in BuildMediaDescription(), and uses engine->codecs() to get the lists.

Comment on attachment 652469 [details] Temporary patch to disable audio codecs we're not using Review of attachment 652469 [details]: ----------------------------------------------------------------- looks fine to me, I will be adding opus to this tomorrow so it would be good if you pushed this.

Comment on attachment 725015 [details] Make offered codecs depend on the available codecs in Audio/Video Conduit Changed implementation to incorporate feedback. Now the codecs list are generated as static methods on respective conduits.

Comment on attachment 725015 [details] Make offered codecs depend on the available codecs in Audio/Video Conduit Review of attachment 725015 [details]: ----------------------------------------------------------------- r+ assuming comments addressed. ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +81,5 @@ > + { > + //a return value of zero indicates success > + if(!codecInterface->GetCodec(idx, audio_codec)) > + { > + payloadName = audio_codec.plname; So, I got an interesting list of names from this, five "L16", two each of "PCMU" and "PCMA", four "CN" and a "telephone-event", is this the right member to be keying this off of? Which one would be G722 for example. @@ +82,5 @@ > + //a return value of zero indicates success > + if(!codecInterface->GetCodec(idx, audio_codec)) > + { > + payloadName = audio_codec.plname; > + CSFLogError(logTag, "%s\n", payloadName.c_str()); I don't think this needs a \n at the end and is not worthy of an Error, perhaps LogDebug. ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +66,5 @@ > + //a return value of zero indicates success > + if(!codecInterface->GetCodec(idx, video_codec)) > + { > + payloadName = video_codec.plName; > + CSFLogError(logTag, "%s\n", payloadName.c_str()); Again, Debug and no \n ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp @@ +175,5 @@ > //codecMask |= VCM_CODEC_RESOURCE_LINEAR; > //codecMask |= VCM_CODEC_RESOURCE_G722; > //codecMask |= VCM_CODEC_RESOURCE_iLBC; > //codecMask |= VCM_CODEC_RESOURCE_iSAC; > + codecMask = populateCodecList(TYPE_AUDIO); I think you could get rid of these commented out lines now and just do "int codecMask = populateCodecList(TYPE_AUDIO);" @@ +299,5 @@ > + } else if(!codec.compare("opus")) { > + return VCM_CODEC_RESOURCE_OPUS; > + } else if (!codec.compare("VP8")) { > + return VCM_CODEC_RESOURCE_VP8; > + } else { I recommend adding else ifs for the codecs that normally come back that we are purposely suppressing here with a quick comment and an = 0;. That way it would be easy for someone to turn one of the back on. @@ +320,5 @@ > + { > + case TYPE_AUDIO: > + codecs = AudioSessionConduit::Codecs(); > + if(!codecs) > + { In the next case, you put the brace at the end of the line. Perhaps stay consistent with how you want it to look.

Comment on attachment 725015 [details] Make offered codecs depend on the available codecs in Audio/Video Conduit Review of attachment 725015 [details]: ----------------------------------------------------------------- r+ with nits and if the codec list supported includes all of them (and verify that only those enabled in webrtc_config.gypi actually get returned) ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +81,5 @@ > + { > + //a return value of zero indicates success > + if(!codecInterface->GetCodec(idx, audio_codec)) > + { > + payloadName = audio_codec.plname; > which one would be G722? None of them. I suspect the different versions you got differed by things like sampling trate. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp @@ +167,5 @@ > > NS_ENSURE_TRUE(mCCM.get(), NS_ERROR_FAILURE); > > // Add the local audio codecs > // FIX - Get this list from MediaEngine instead remove this comment (last line) @@ +168,5 @@ > NS_ENSURE_TRUE(mCCM.get(), NS_ERROR_FAILURE); > > // Add the local audio codecs > // FIX - Get this list from MediaEngine instead > int codecMask = 0; Initialization is now redundant @@ +180,5 @@ > mCCM->setAudioCodecs(codecMask); > > //Add the local video codecs > // FIX - Get this list from MediaEngine instead > // Turning them all on for now Remove this comment @@ +191,1 @@ > //codecMask |= VCM_CODEC_RESOURCE_I420; ditto @@ +300,5 @@ > + return VCM_CODEC_RESOURCE_OPUS; > + } else if (!codec.compare("VP8")) { > + return VCM_CODEC_RESOURCE_VP8; > + } else { > + // return 0 which implies the codec mask I would add all the codecs here. Codecs are enabled or disabled through webrtc_config.gypi; this code should simply reflect which codecs are enabled with no code changes @@ +320,5 @@ > + { > + case TYPE_AUDIO: > + codecs = AudioSessionConduit::Codecs(); > + if(!codecs) > + { Ditto ethan - stay in the style of the file

Patch to support codecs retrieval from the conduits.

Comment on attachment 725015 [details] Make offered codecs depend on the available codecs in Audio/Video Conduit I will be uploading the per conduit level codec retrieval patch instead of static one-time approach followed in here.

Comment on attachment 735551 [details] [diff] [review] Part2: PeerConnectionMedia changes Updated PeerConnectionMedia to store generic MediaSessionConduit references instead of AudioSessionConduit. Removed setting Audio/Video hardcoded masks during PeerConnectionInitialization

Comment on attachment 735552 [details] [diff] [review] Sipcc changes to supporting dynamic codec retrieval Review of attachment 735552 [details] [diff] [review]: ----------------------------------------------------------------- GSM/VCM changes to retrieve codec list dynamically.

Comment on attachment 735550 [details] [diff] [review] Part1: MediaConduit Changes to report Codecs supported Review of attachment 735550 [details] [diff] [review]: ----------------------------------------------------------------- r- for refetch of the list each time. And please change the name. Convert to an r+ with my suggestion for avoiding refetches if it makes sense. ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +257,5 @@ > + payloadName = codec.plname; > + mCodecList.push_back(payloadName); > + } > + } > + return mCodecList; If we're fetching the names every time, why are we storing a local copy? More to the point, why are we fetching the names every time unless the supported codecs in Voice/Video engine can change dynamically, which I believe they cannot. So maybe: if (mCodecList.empty()) { fetch list } return mCodecList; Note: in the unlikely case of no codecs supported, it will attempt to re-fetch the list anyways (and get no codecs again). That's ok. ::: media/webrtc/signaling/src/media-conduit/AudioConduit.h @@ +87,5 @@ > > /** > + * Function to retrieve the Codecs supported. > + * return: Reference to list of codecs supported by the underlying > + * Audio Engine. The returned value represents payload name string "The returned array contains codec names" perhaps @@ +91,5 @@ > + * Audio Engine. The returned value represents payload name string > + * of the codec. > + * Note: This function can be invoked multiple times. > + */ > + virtual const std::vector<std::string>& Codecs(); SupportedCodecs() please (or COdecsSupported(), I don't care.) Codecs() is too generic and could be confusing ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h @@ +139,5 @@ > + /** > + * Function to retrieve the Codecs supported. > + * return: Reference to list of codecs supported by the underlying > + * Audio/Video Media Engine. The returned value represents > + * payload name string of the codec. Ditto comment on comment

Comment on attachment 735551 [details] [diff] [review] Part2: PeerConnectionMedia changes Review of attachment 735551 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the obvious typo fix ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h @@ +310,5 @@ > } > > // Add a conduit > void AddConduit(int aIndex, bool aReceive, > + const mozilla::RefPtr<mozilla::MediaSegment> &aConduit) { MediaSegment??? MediaSessionConduit I think you mean I see, a trap to make sure I'm reading the diffs ;-)

Comment on attachment 735552 [details] [diff] [review] Sipcc changes to supporting dynamic codec retrieval Review of attachment 735552 [details] [diff] [review]: ----------------------------------------------------------------- r- This is overall the wrong approach. Codecs aren't tied to conduits; they're globally configured at build time (and certainly don't change from one conduit to another). You'll note that the jingle code I referred to (once your drill down) is just fetching matching audio/video entries from an existing global list. So all the changes to instantiate the conduits in order to be able to ask them what their codecs are causes the code to be much harder to follow (and adds cross-thread calls) to fetch the same list, again and again. The list should be fetched once when the engine is opened, and then just re-used when generating SDP. Some of the changes here may be useful for other reasons (in particular the direction changes and registering all the conduits with pcmedia) such as resolving the dependent bug about recvonly/sendonly. Other parts might simply get moved and invoked at a different point (generating the masks perhaps). ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +1308,5 @@ > + CSFLogError(logTag,"%s", "FOUND VP8"); > + codecMask |= VCM_CODEC_RESOURCE_VP8; > + break; > + } > + } This should check all known codecs, not just the 3 (4 with PCMA/U) that we support, so that if we enable (say) iSAC in webrtc_config.gypi, it will Just Work. @@ +1333,5 @@ > + // Find the PC. > + sipcc::PeerConnectionWrapper pc(peerconnection); > + ENSURE_PC(pc, VCM_ERROR); > + > + // Create Tx Conduit/Retrieve if already created. "create foo if already created"?? Maybe "not" is missing ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h @@ +309,5 @@ > } > > // Add a conduit > void AddConduit(int aIndex, bool aReceive, > + const mozilla::RefPtr<mozilla::MediaSessionConduit> &aConduit) { How was this ever correct? Aha, I see, it fixed the typo in the previous patch!

Based on the IRC discussion, we will hold per conduit instance codec retrieval and configuration till there is a need for it. We can re-open a bug whenever we need this functionality.