Adding codecs to the RtpParameters returned by an RtpSender.

Adding codecs to the RtpParameters returned by an RtpSender.

Contains every field except for sdpFmtpLine.
Setting a reordered list of codecs is not yet supported.

R=glaznev@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org, tkchin@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/0cd086b70ec939cad25768d5e17a61da23613281

[Taylor Brandstetter, 2016-04-14 00:37:35]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org, skvlad@webrtc.org

[Taylor Brandstetter, 2016-04-14 00:37:36]

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:383: // Does *not* contain codecs,
however.
As RtpParameters grows bigger, I don't know that it makes sense to store it
here. We have two structs (RtpParameters, VideoSendParameters) that contain
duplicate information. Why not put everything in VideoSendParameters, and
generate the RtpParameters from it on the fly?

[skvlad, 2016-04-14 01:03:56]

LGTM overall, but I'm not an owner of any of these files.

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/api/objc/RTCRtpCod...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/api/objc/RTCRtpCod...
webrtc/api/objc/RTCRtpCodecParameters.h:24: @property(nonatomic, nonnull)
NSString *mimeType;
Since this is in a NS_ASSUME_NONNULL block, all properties are nonnull by
default. None of the other files explicitly list the nonnull attribute, so it'd
be better for consistency to remove it.

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/api/objc/RTCRtpCod...
File webrtc/api/objc/RTCRtpCodecParameters.mm (right):

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/api/objc/RTCRtpCod...
webrtc/api/objc/RTCRtpCodecParameters.mm:39: parameters.mime_type = [NSString
stdStringForString:_mimeType];
this probably requires #import "webrtc/base/objc/NSString+StdString.h"

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:899:
rtp_params.codecs.push_back(codec_params);
Could this be moved into a function common for both audio and video? It seems to
only be using fields from the base Codec struct.

[Taylor Brandstetter, 2016-04-14 01:43:45]

I realize you're not an owner, but thought you may have some opinions since
you've been working with these APIs. Thanks for the feedback.

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/api/objc/RTCRtpCod...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/api/objc/RTCRtpCod...
webrtc/api/objc/RTCRtpCodecParameters.h:24: @property(nonatomic, nonnull)
NSString *mimeType;
On 2016/04/14 01:03:56, skvlad wrote:
> Since this is in a NS_ASSUME_NONNULL block, all properties are nonnull by
> default. None of the other files explicitly list the nonnull attribute, so
it'd
> be better for consistency to remove it.

Done.

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/api/objc/RTCRtpCod...
File webrtc/api/objc/RTCRtpCodecParameters.mm (right):

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/api/objc/RTCRtpCod...
webrtc/api/objc/RTCRtpCodecParameters.mm:39: parameters.mime_type = [NSString
stdStringForString:_mimeType];
On 2016/04/14 01:03:56, skvlad wrote:
> this probably requires #import "webrtc/base/objc/NSString+StdString.h"

Done.

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1885473004/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:899:
rtp_params.codecs.push_back(codec_params);
On 2016/04/14 01:03:56, skvlad wrote:
> Could this be moved into a function common for both audio and video? It seems
to
> only be using fields from the base Codec struct.

Yeah, I can move this to codec.h/codec.cc.

[Taylor Brandstetter, 2016-04-14 01:45:21]

deadbeef@webrtc.org changed reviewers:
+ glaznev@webrtc.org, tkchin@webrtc.org

[Taylor Brandstetter, 2016-04-14 01:45:22]

Zeke, can you review webrtc/api/objc/*?
Alex, can you review webrtc/api/java/*?

Thanks!

[pthatcher1, 2016-04-14 15:58:13]

lgtm

https://codereview.webrtc.org/1885473004/diff/80001/webrtc/api/objc/RTCRtpCod...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/80001/webrtc/api/objc/RTCRtpCod...
webrtc/api/objc/RTCRtpCodecParameters.h:29: /** The number of channels (mono=1,
stereo=2). */
Why not include these comments in the .java file also?

https://codereview.webrtc.org/1885473004/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1885473004/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:930:
EXPECT_EQ(kPcmuCodec.channels, rtp_parameters.codecs[1].channels);
Since you implemented the == operator, you could just do

EXPECT_EQ(kIsacCodec, rtp_parameters.codecs[0])
EXPECT_EQ(kPcmuCodec, rtp_parameters.codecs[1])

[Taylor Brandstetter, 2016-04-14 16:30:54]

https://codereview.webrtc.org/1885473004/diff/80001/webrtc/api/objc/RTCRtpCod...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/80001/webrtc/api/objc/RTCRtpCod...
webrtc/api/objc/RTCRtpCodecParameters.h:29: /** The number of channels (mono=1,
stereo=2). */
On 2016/04/14 15:58:12, pthatcher1 wrote:
> Why not include these comments in the .java file also?

I was just following the convention. The C++ .h file and .java file refer to the
WebRTC spec for the meaning of the parameters, whereas the objc .h file doesn't.

https://codereview.webrtc.org/1885473004/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1885473004/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:930:
EXPECT_EQ(kPcmuCodec.channels, rtp_parameters.codecs[1].channels);
On 2016/04/14 15:58:12, pthatcher1 wrote:
> Since you implemented the == operator, you could just do
> 
> EXPECT_EQ(kIsacCodec, rtp_parameters.codecs[0])
> EXPECT_EQ(kPcmuCodec, rtp_parameters.codecs[1])

I didn't implement the == operator for webrtc::RtpCodecParameters to
cricket::AudioCodecs though.

[tkchin_webrtc, 2016-04-14 17:52:51]

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:18: @property(nonatomic, assign) int
payloadType;
ditto enum?

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:24: @property(nonatomic, nonnull)
NSString *mimeType;
Use an enum for this and translate to what the C++ needs?

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.mm (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.mm:11: #import
"RTCRtpCodecParameters+Private.h"
nit: one blank line after interface header import

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
File webrtc/api/objc/RTCRtpParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
webrtc/api/objc/RTCRtpParameters.h:24: @property(nonatomic, copy)
NSArray<RTCRtpCodecParameters*>* codecs;
update * spacing (see above)

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
File webrtc/api/objc/RTCRtpParameters.mm (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
webrtc/api/objc/RTCRtpParameters.mm:11: #import "RTCRtpParameters+Private.h"
ditto add a blank line after interface import

[Taylor Brandstetter, 2016-04-14 18:17:22]

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:18: @property(nonatomic, assign) int
payloadType;
On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> ditto enum?

An enum for every value from 0 to 127?

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:24: @property(nonatomic, nonnull)
NSString *mimeType;
On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> Use an enum for this and translate to what the C++ needs?

There are a pretty lot of possibilities (100+) and more could be added. Granted,
we only support a small subset of them, but it still seems like overkill when an
application could use a string compare if it really needs to.

Also, this is a string in the JS API and I think it makes sense to keep the APIs
as similar as possible.

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.mm (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.mm:11: #import
"RTCRtpCodecParameters+Private.h"
On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> nit: one blank line after interface header import

Done.

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
File webrtc/api/objc/RTCRtpParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
webrtc/api/objc/RTCRtpParameters.h:24: @property(nonatomic, copy)
NSArray<RTCRtpCodecParameters*>* codecs;
On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> update * spacing (see above)

Done. I think git cl format did this and I didn't catch it.

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
File webrtc/api/objc/RTCRtpParameters.mm (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
webrtc/api/objc/RTCRtpParameters.mm:11: #import "RTCRtpParameters+Private.h"
On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> ditto add a blank line after interface import

Done.

[pthatcher1, 2016-04-14 18:27:15]

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:18: @property(nonatomic, assign) int
payloadType;
On 2016/04/14 18:17:22, Taylor Brandstetter wrote:
> On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> > ditto enum?
> 
> An enum for every value from 0 to 127?

And since the values are dynamically assigned, at best the enum would be

PAYLOAD_TYPE_1
PAYLOAD_TYPE_2
PAYLOAD_TYPE_3

With the exception of perhaps PCMU and a few other random PTs that have static
values.

[tkchin_webrtc, 2016-04-14 18:30:09]

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:18: @property(nonatomic, assign) int
payloadType;
On 2016/04/14 18:27:15, pthatcher1 wrote:
> On 2016/04/14 18:17:22, Taylor Brandstetter wrote:
> > On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> > > ditto enum?
> > 
> > An enum for every value from 0 to 127?
> 
> And since the values are dynamically assigned, at best the enum would be
> 
> PAYLOAD_TYPE_1
> PAYLOAD_TYPE_2
> PAYLOAD_TYPE_3
> 
> With the exception of perhaps PCMU and a few other random PTs that have static
> values.

Interesting. How would a client assign this payload type then?

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:24: @property(nonatomic, nonnull)
NSString *mimeType;
On 2016/04/14 18:17:22, Taylor Brandstetter wrote:
> On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> > Use an enum for this and translate to what the C++ needs?
> 
> There are a pretty lot of possibilities (100+) and more could be added.
Granted,
> we only support a small subset of them, but it still seems like overkill when
an
> application could use a string compare if it really needs to.
> 
> Also, this is a string in the JS API and I think it makes sense to keep the
APIs
> as similar as possible.

Ok. In that case please add supported strings as constants in this file?

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
File webrtc/api/objc/RTCRtpParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpPa...
webrtc/api/objc/RTCRtpParameters.h:24: @property(nonatomic, copy)
NSArray<RTCRtpCodecParameters*>* codecs;
On 2016/04/14 18:17:22, Taylor Brandstetter wrote:
> On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> > update * spacing (see above)
> 
> Done. I think git cl format did this and I didn't catch it.

Yeah git cl format wrecks ObjC styling.

[Taylor Brandstetter, 2016-04-14 20:01:48]

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:18: @property(nonatomic, assign) int
payloadType;
On 2016/04/14 18:30:08, tkchin_webrtc wrote:
> On 2016/04/14 18:27:15, pthatcher1 wrote:
> > On 2016/04/14 18:17:22, Taylor Brandstetter wrote:
> > > On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> > > > ditto enum?
> > > 
> > > An enum for every value from 0 to 127?
> > 
> > And since the values are dynamically assigned, at best the enum would be
> > 
> > PAYLOAD_TYPE_1
> > PAYLOAD_TYPE_2
> > PAYLOAD_TYPE_3
> > 
> > With the exception of perhaps PCMU and a few other random PTs that have
static
> > values.
> 
> Interesting. How would a client assign this payload type then? 

If a client wanted to add a new codec without SDP renegotiation (the ORTC way of
doing things) they might construct a codec from scratch and choose an available
payload type from the dynamic range.

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:24: @property(nonatomic, nonnull)
NSString *mimeType;
On 2016/04/14 18:30:08, tkchin_webrtc wrote:
> On 2016/04/14 18:17:22, Taylor Brandstetter wrote:
> > On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> > > Use an enum for this and translate to what the C++ needs?
> > 
> > There are a pretty lot of possibilities (100+) and more could be added.
> Granted,
> > we only support a small subset of them, but it still seems like overkill
when
> an
> > application could use a string compare if it really needs to.
> > 
> > Also, this is a string in the JS API and I think it makes sense to keep the
> APIs
> > as similar as possible.
> 
> Ok. In that case please add supported strings as constants in this file?

Ok. I copied them from mediaconstants.h/cc

[tkchin_webrtc, 2016-04-14 21:11:47]

lgtm for objc % nits

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:18: @property(nonatomic, assign) int
payloadType;
On 2016/04/14 20:01:47, Taylor Brandstetter wrote:
> On 2016/04/14 18:30:08, tkchin_webrtc wrote:
> > On 2016/04/14 18:27:15, pthatcher1 wrote:
> > > On 2016/04/14 18:17:22, Taylor Brandstetter wrote:
> > > > On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> > > > > ditto enum?
> > > > 
> > > > An enum for every value from 0 to 127?
> > > 
> > > And since the values are dynamically assigned, at best the enum would be
> > > 
> > > PAYLOAD_TYPE_1
> > > PAYLOAD_TYPE_2
> > > PAYLOAD_TYPE_3
> > > 
> > > With the exception of perhaps PCMU and a few other random PTs that have
> static
> > > values.
> > 
> > Interesting. How would a client assign this payload type then? 
> 
> If a client wanted to add a new codec without SDP renegotiation (the ORTC way
of
> doing things) they might construct a codec from scratch and choose an
available
> payload type from the dynamic range.

Can we add a pointer to documentation for that, or just make this readonly?
I just wanted to avoid having a setter where it's not clear what the value
should be set to.

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:15: extern NSString * const
kRtxCodecMimeType;
nit: const NSString * const?

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:37: * The codec MIME type. Valid types
are listed in:
nit: add short comment that points at above constants

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.mm (right):

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.mm:15: NSString * const kRtxCodecMimeType
= @"rtx";
nit: point at the constants directly? e.g. const NSString * const
kRtxCodecMimeType = @(cricket::kRtxCodecName);

I *think* this works.

[AlexG, 2016-04-14 21:21:32]

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
File webrtc/api/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2149: RTC_DCHECK(added);
RTC_CHECK()? Failing to add to linked list is critical error?

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2175: jboolean added =
jni->CallBooleanMethod(j_codecs, add, j_codec);
This jmethodID add is for j_encodings object, not for j_codecs. It should not
probably matter, but better to get separate add for j_codecs

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2177: RTC_DCHECK(added);
ditto: RTC_CHECK?

[AlexG, 2016-04-14 21:25:54]

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
File webrtc/api/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2175: jboolean added =
jni->CallBooleanMethod(j_codecs, add, j_codec);
On 2016/04/14 21:21:32, AlexG wrote:
> This jmethodID add is for j_encodings object, not for j_codecs. It should not
> probably matter, but better to get separate add for j_codecs

... or rename this method to add_linked_list or similar to indicate that this
add is applicable for LinkedList only class

[Taylor Brandstetter, 2016-04-14 22:03:31]

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/100001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:18: @property(nonatomic, assign) int
payloadType;
On 2016/04/14 21:11:47, tkchin_webrtc wrote:
> On 2016/04/14 20:01:47, Taylor Brandstetter wrote:
> > On 2016/04/14 18:30:08, tkchin_webrtc wrote:
> > > On 2016/04/14 18:27:15, pthatcher1 wrote:
> > > > On 2016/04/14 18:17:22, Taylor Brandstetter wrote:
> > > > > On 2016/04/14 17:52:51, tkchin_webrtc wrote:
> > > > > > ditto enum?
> > > > > 
> > > > > An enum for every value from 0 to 127?
> > > > 
> > > > And since the values are dynamically assigned, at best the enum would be
> > > > 
> > > > PAYLOAD_TYPE_1
> > > > PAYLOAD_TYPE_2
> > > > PAYLOAD_TYPE_3
> > > > 
> > > > With the exception of perhaps PCMU and a few other random PTs that have
> > static
> > > > values.
> > > 
> > > Interesting. How would a client assign this payload type then? 
> > 
> > If a client wanted to add a new codec without SDP renegotiation (the ORTC
way
> of
> > doing things) they might construct a codec from scratch and choose an
> available
> > payload type from the dynamic range.
> 
> Can we add a pointer to documentation for that, or just make this readonly?
> I just wanted to avoid having a setter where it's not clear what the value
> should be set to.

Done.

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
File webrtc/api/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2149: RTC_DCHECK(added);
On 2016/04/14 21:21:32, AlexG wrote:
> RTC_CHECK()? Failing to add to linked list is critical error?

Done.

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2175: jboolean added =
jni->CallBooleanMethod(j_codecs, add, j_codec);
On 2016/04/14 21:21:32, AlexG wrote:
> This jmethodID add is for j_encodings object, not for j_codecs. It should not
> probably matter, but better to get separate add for j_codecs

Done.

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2177: RTC_DCHECK(added);
On 2016/04/14 21:21:32, AlexG wrote:
> ditto: RTC_CHECK?

Done.

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.h (right):

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:15: extern NSString * const
kRtxCodecMimeType;
On 2016/04/14 21:11:47, tkchin_webrtc wrote:
> nit: const NSString * const?

Done.

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.h:37: * The codec MIME type. Valid types
are listed in:
On 2016/04/14 21:11:47, tkchin_webrtc wrote:
> nit: add short comment that points at above constants

Done.

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
File webrtc/api/objc/RTCRtpCodecParameters.mm (right):

https://codereview.webrtc.org/1885473004/diff/140001/webrtc/api/objc/RTCRtpCo...
webrtc/api/objc/RTCRtpCodecParameters.mm:15: NSString * const kRtxCodecMimeType
= @"rtx";
On 2016/04/14 21:11:47, tkchin_webrtc wrote:
> nit: point at the constants directly? e.g. const NSString * const
> kRtxCodecMimeType = @(cricket::kRtxCodecName);
> 
> I *think* this works.

Done.

[AlexG, 2016-04-14 22:07:51]

lgtm

https://codereview.webrtc.org/1885473004/diff/160001/webrtc/api/java/jni/peer...
File webrtc/api/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1885473004/diff/160001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2146: jboolean added =
jni->CallBooleanMethod(j_encodings, encodings_add,
alignment : 4 space ident

https://codereview.webrtc.org/1885473004/diff/160001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2157: jmethodID codecs_add =
GetMethodID(jni, GetObjectClass(jni, j_codecs),
alignment

https://codereview.webrtc.org/1885473004/diff/160001/webrtc/api/java/jni/peer...
webrtc/api/java/jni/peerconnection_jni.cc:2170: jni->SetObjectField(j_codec,
mime_type_id,
alignment

[Taylor Brandstetter, 2016-04-20 22:53:13]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-04-20 22:53:14]

The patchset sent to the CQ was uploaded after l-g-t-m from skvlad@webrtc.org,
pthatcher@webrtc.org, tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1885473004/#ps160001 (title:
"Responding to review comments.")

[commit-bot: I haz the power, 2016-04-20 22:53:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1885473004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1885473004/160001

[commit-bot: I haz the power, 2016-04-20 22:56:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 22:56:27]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4971)

[commit-bot: I haz the power, 2016-04-20 23:23:28]

Description was changed from

==========
Adding codecs to the RtpParameters returned by an RtpSender.

Contains every field except for sdpFmtpLine.
Setting a reordered list of codecs is not yet supported.
==========

to

==========
Adding codecs to the RtpParameters returned by an RtpSender.

Contains every field except for sdpFmtpLine.
Setting a reordered list of codecs is not yet supported.

R=glaznev@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/0cd086b70ec939cad25768d5e17a61da23613281
Cr-Commit-Position: refs/heads/master@{#12453}
==========

[commit-bot: I haz the power, 2016-04-20 23:23:29]

Patchset 9 (id:??) landed as
https://crrev.com/0cd086b70ec939cad25768d5e17a61da23613281
Cr-Commit-Position: refs/heads/master@{#12453}

[Taylor Brandstetter, 2016-04-20 23:23:30]

Description was changed from

==========
Adding codecs to the RtpParameters returned by an RtpSender.

Contains every field except for sdpFmtpLine.
Setting a reordered list of codecs is not yet supported.

R=glaznev@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/0cd086b70ec939cad25768d5e17a61da23613281
Cr-Commit-Position: refs/heads/master@{#12453}
==========

to

==========
Adding codecs to the RtpParameters returned by an RtpSender.

Contains every field except for sdpFmtpLine.
Setting a reordered list of codecs is not yet supported.

R=glaznev@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org, tkchin@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/0cd086b70ec939cad25768d5e...
==========

[Taylor Brandstetter, 2016-04-20 23:23:32]

Committed patchset #9 (id:160001) manually as
0cd086b70ec939cad25768d5e17a61da23613281 (presubmit successful).