Objective C API to read and set RtpParameters

Objective C API to read and set RtpParameters

This change adds the Objective C API functions to get and set RtpSender's
RtpParameters, which allows setting bitrate limits for audio and video and
turning off RtpSenders to pre-initialize the encoder.

This CL adds only the smallest set of methods required to support bitrate
limiting - there is no way to create an RtpSender, for example, or to set
its track. The only supported functionality is this:
 	RTCPeerConnection.senders - a read-only property returning the array
	  of all RTCRtpSenders for the connection.
        RTCRtpSender.parameters - a read-only property returning the current
    	  parameters
	RTCRtpSender.setParameters: - a method to change the parameters.
	RTCRtpSender.track - a read-only property returning the
	  RTCMediaStreamTrack corresponding to the sender. It is necessary
	  to be able to identify RTCRtpSenders for video and audio. The
	  track object is of the base RTCMediaStreamTrack type, not of the
          specific subclass for audio and video - just like it is in the
	  Java API.

BUG=

Committed: https://crrev.com/79b4b8720d151a773a42212d52949e8604c258c5
Cr-Commit-Position: refs/heads/master@{#12297}

[skvlad, 2016-04-05 01:54:24]

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

[skvlad, 2016-04-05 01:54:25]

This is the ObjC API for reading and writing RtpParameters; the CL does not
include the changes to the demo app necessary to test them.

[Taylor Brandstetter, 2016-04-05 17:48:03]

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
File webrtc/api/objc/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:319: for (auto nativeSender :
nativeSenders) {
nit: I think this creates a copy of the scoped_refptrs; usually we use "const
auto&" to avoid this, though it shouldn't really matter.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
File webrtc/api/objc/RTCRtpEncodingParameters.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.h:18: @property(nonatomic, assign) BOOL
active;
nit: could put "active" first, since it's first elsewhere.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender+...
File webrtc/api/objc/RTCRtpSender+Private.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender+...
webrtc/api/objc/RTCRtpSender+Private.h:19: /** Initialize an RTCMediaStream from
a native MediaStreamInterface. */
MediaStream => RtpSender

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender.mm
File webrtc/api/objc/RTCRtpSender.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.mm:29: _nativeRtpSender->track());
I think it would be better if the track wasn't cached, and it was just retrieved
every time "track" is called. That way, the Obj-C RtpSender doesn't need to keep
track of when the track of the native RtpSender changes (due to, for example,
PeerConnection.addTrack).

I know the Java binding caches the track, but that's due to its unique ownership
semantics (the RtpSender is supposed to "dispose" of its track). I plan to
improve how that works at some point.

[tkchin_webrtc, 2016-04-05 18:48:16]

Largely style nits. Ping me offline if you have other style questions.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
File webrtc/api/objc/RTCPeerConnection.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.h:120: @property(nonatomic, readonly)
NSArray<RTCRtpSender*> *senders;
ObjC pointer style is space *

@property(nonatomic, readonly) NSArray<RTCRtpSender *> *senders;

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
File webrtc/api/objc/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:315: - (NSArray<RTCRtpSender*>*)senders {
ditto Sender *> *)

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:316:
std::vector<rtc::scoped_refptr<webrtc::RtpSenderInterface>> nativeSenders(
Clearly rusty on c++ -> compiler doesn't barf on >> anymore?

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:318: NSMutableArray* senders =
[[NSMutableArray alloc] init];
pointer style: NSMutableArray *senders =

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:319: for (auto nativeSender :
nativeSenders) {
Consider adding a category in webrtc/base/objc similar to NSString+StdString to
deal with these common conversions (vector to NSArray, NSArray to vector).

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
File webrtc/api/objc/RTCRtpEncodingParameters+Private.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters+Private.h:12: #import
"webrtc/api/objc/RTCRtpEncodingParameters.h"
nit: blank line after system imports

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
File webrtc/api/objc/RTCRtpEncodingParameters.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.h:17: @property(nonatomic, copy,
nullable) NSNumber* maxBitrateBps;
not common to use NSNumber outside of collection classes - just use whatever the
underlying C++ type is.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.h:18: @property(nonatomic, assign) BOOL
active;
ObjC style (at least in our codebase) is to prefix bools with is/has/should
BOOL isActive;

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
File webrtc/api/objc/RTCRtpEncodingParameters.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.mm:18: const int kBitrateUnlimited =
-1;
no namespace, so either static linkage or prefix:
RTCRtpEncodingParametersBitrateUnlimited

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.mm:22: parameters.active = self.active;
usually we just access instance variable directly if it's nonatomic. _active

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.mm:32: if
(nativeParameters.max_bitrate_bps != kBitrateUnlimited) {
isn't there a C++ equivalent of unlimited bitrate? It would be good to stick
this kind of constant in C++ land.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.mm:33: self.maxBitrateBps =
ditto _maxBitrateBps = nativeParameters.max_bitrate_bps

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
File webrtc/api/objc/RTCRtpParameters.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.h:12: #import
"webrtc/api/objc/RTCRtpEncodingParameters.h"
ditto blank line

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.h:18: @property(nonatomic, copy)
NSArray<RTCRtpEncodingParameters*>* encodings;
rs *> *encodings;

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.h:20: - (instancetype)init NS_UNAVAILABLE;
this is non-init-able yet the encodings is a settable property. Do we ever init
a parameters from ObjC land and pass it along?

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
File webrtc/api/objc/RTCRtpParameters.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.mm:20: for (RTCRtpEncodingParameters* encoding
in self.encodings) {
ers *encoding in _encodings

(in case you're wondering -> style rules for ObjC in ObjC methods and classes,
C++ rules for C++ methods and classes within a .mm file)

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.mm:27: (const
webrtc::RtpParameters&)nativeParameters {
(const RtpParameters &)

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.mm:29: NSMutableArray* encodings =
[[NSMutableArray alloc] init];
ditto *, & and _ :)

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender.h
File webrtc/api/objc/RTCRtpSender.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.h:12: #import "webrtc/api/objc/RTCRtpParameters.h"
ditto blank line

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.h:17: @interface RTCRtpSender : NSObject
consider making this a protocol so that one day when we have time for unit tests
we can do it more easily.

@protocol RTCRtpSender : NSObject

parameters, track

@end

@class RTCRtpSender : NSObject <RTCRtpSender>
@end

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.h:19: @property(nonatomic, readonly)
RTCRtpParameters* parameters;
* in file

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.h:24: -
(BOOL)setParameters:(RTCRtpParameters*)parameters;
is this valuable? If it fails we won't know why. Is there any meaningful error
handling that can happen?

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender.mm
File webrtc/api/objc/RTCRtpSender.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.mm:12: #import
"webrtc/api/objc/RTCRtpParameters+Private.h"
nit: blank line after first header import

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.mm:21: RTCMediaStreamTrack* _track;
ditto * in file

[skvlad, 2016-04-05 23:21:28]

Updated with code review feedback. I hope I didn't forget anything.

I've also tested this change with a modified build of AppRTCDemo and confirmed
that it works; still not sure if I should also submit my changes to AppRTCDemo.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
File webrtc/api/objc/RTCPeerConnection.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.h:120: @property(nonatomic, readonly)
NSArray<RTCRtpSender*> *senders;
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> ObjC pointer style is space *
> 
> @property(nonatomic, readonly) NSArray<RTCRtpSender *> *senders;

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
File webrtc/api/objc/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:315: - (NSArray<RTCRtpSender*>*)senders {
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> ditto Sender *> *)

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:316:
std::vector<rtc::scoped_refptr<webrtc::RtpSenderInterface>> nativeSenders(
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> Clearly rusty on c++ -> compiler doesn't barf on >> anymore?

Since C++ 11 it isn't supposed to, and the Chromium .clang-format file appears
to prefer no space:
https://code.google.com/p/chromium/codesearch#chromium/src/.clang-format

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:318: NSMutableArray* senders =
[[NSMutableArray alloc] init];
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> pointer style: NSMutableArray *senders =

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:319: for (auto nativeSender :
nativeSenders) {
On 2016/04/05 17:48:03, Taylor Brandstetter wrote:
> nit: I think this creates a copy of the scoped_refptrs; usually we use "const
> auto&" to avoid this, though it shouldn't really matter.

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCPeerConnec...
webrtc/api/objc/RTCPeerConnection.mm:319: for (auto nativeSender :
nativeSenders) {
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> Consider adding a category in webrtc/base/objc similar to NSString+StdString
to
> deal with these common conversions (vector to NSArray, NSArray to vector).

Is there a way to define a generic NSArray to vector<T> conversion function?
Otherwise it's probably better to keep it local, as no other code will need to
convert a vector<refptr<RtpSenderInterface>> to an array.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
File webrtc/api/objc/RTCRtpEncodingParameters+Private.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters+Private.h:12: #import
"webrtc/api/objc/RTCRtpEncodingParameters.h"
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> nit: blank line after system imports

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
File webrtc/api/objc/RTCRtpEncodingParameters.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.h:17: @property(nonatomic, copy,
nullable) NSNumber* maxBitrateBps;
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> not common to use NSNumber outside of collection classes - just use whatever
the
> underlying C++ type is.

As discussed offline - this property contains an optional value, and a nullable
NSNumber seems to be appropriate. This corresponds to an rtc::Optional<int> in
the C++ codebase.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.h:18: @property(nonatomic, assign) BOOL
active;
On 2016/04/05 17:48:03, Taylor Brandstetter wrote:
> nit: could put "active" first, since it's first elsewhere.

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.h:18: @property(nonatomic, assign) BOOL
active;
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> ObjC style (at least in our codebase) is to prefix bools with is/has/should
> BOOL isActive;

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
File webrtc/api/objc/RTCRtpEncodingParameters.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.mm:18: const int kBitrateUnlimited =
-1;
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> no namespace, so either static linkage or prefix:
> RTCRtpEncodingParametersBitrateUnlimited

Made static.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.mm:22: parameters.active = self.active;
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> usually we just access instance variable directly if it's nonatomic. _active

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.mm:32: if
(nativeParameters.max_bitrate_bps != kBitrateUnlimited) {
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> isn't there a C++ equivalent of unlimited bitrate? It would be good to stick
> this kind of constant in C++ land.

The C++ equivalent will be replaced with rtc::Optional in an upcoming CL. I'll
update this code then. Added a TODO for now.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpEncodin...
webrtc/api/objc/RTCRtpEncodingParameters.mm:33: self.maxBitrateBps =
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> ditto _maxBitrateBps = nativeParameters.max_bitrate_bps

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
File webrtc/api/objc/RTCRtpParameters.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.h:12: #import
"webrtc/api/objc/RTCRtpEncodingParameters.h"
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> ditto blank line

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.h:18: @property(nonatomic, copy)
NSArray<RTCRtpEncodingParameters*>* encodings;
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> rs *> *encodings;

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.h:20: - (instancetype)init NS_UNAVAILABLE;
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> this is non-init-able yet the encodings is a settable property. Do we ever
init
> a parameters from ObjC land and pass it along?

I don't think there is any harm in allowing the app to create new
RTCRtpParameters in ObjC. Right now there's no real need to, and we expect apps
to use them in a read-modify-write pattern, but why not allow creating?


I've added a regular init method.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
File webrtc/api/objc/RTCRtpParameters.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.mm:20: for (RTCRtpEncodingParameters* encoding
in self.encodings) {
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> ers *encoding in _encodings
> 
> (in case you're wondering -> style rules for ObjC in ObjC methods and classes,
> C++ rules for C++ methods and classes within a .mm file)

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.mm:27: (const
webrtc::RtpParameters&)nativeParameters {
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> (const RtpParameters &)

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpParamet...
webrtc/api/objc/RTCRtpParameters.mm:29: NSMutableArray* encodings =
[[NSMutableArray alloc] init];
On 2016/04/05 18:48:15, tkchin_webrtc wrote:
> ditto *, & and _ :)

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender+...
File webrtc/api/objc/RTCRtpSender+Private.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender+...
webrtc/api/objc/RTCRtpSender+Private.h:19: /** Initialize an RTCMediaStream from
a native MediaStreamInterface. */
On 2016/04/05 17:48:03, Taylor Brandstetter wrote:
> MediaStream => RtpSender

Oops :)

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender.h
File webrtc/api/objc/RTCRtpSender.h (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.h:17: @interface RTCRtpSender : NSObject
On 2016/04/05 18:48:16, tkchin_webrtc wrote:
> consider making this a protocol so that one day when we have time for unit
tests
> we can do it more easily.
> 
> @protocol RTCRtpSender : NSObject
> 
> parameters, track
> 
> @end
> 
> @class RTCRtpSender : NSObject <RTCRtpSender>
> @end

Done.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.h:24: -
(BOOL)setParameters:(RTCRtpParameters*)parameters;
On 2016/04/05 18:48:16, tkchin_webrtc wrote:
> is this valuable? If it fails we won't know why. Is there any meaningful error
> handling that can happen?


It's only valuable in letting you know if the changes had effect or not. There
is no error code returned that tells you what exactly went wrong.

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender.mm
File webrtc/api/objc/RTCRtpSender.mm (right):

https://codereview.webrtc.org/1854393002/diff/1/webrtc/api/objc/RTCRtpSender....
webrtc/api/objc/RTCRtpSender.mm:29: _nativeRtpSender->track());
On 2016/04/05 17:48:03, Taylor Brandstetter wrote:
> I think it would be better if the track wasn't cached, and it was just
retrieved
> every time "track" is called. That way, the Obj-C RtpSender doesn't need to
keep
> track of when the track of the native RtpSender changes (due to, for example,
> PeerConnection.addTrack).
> 
> I know the Java binding caches the track, but that's due to its unique
ownership
> semantics (the RtpSender is supposed to "dispose" of its track). I plan to
> improve how that works at some point.

I've changed the code to create a new track each time.

[skvlad, 2016-04-05 23:38:21]

Patchset #2 (id:20001) has been deleted

[skvlad, 2016-04-05 23:38:30]

Patchset #2 (id:40001) has been deleted

[skvlad, 2016-04-05 23:40:32]

Patchset #2 (id:60001) has been deleted

[Taylor Brandstetter, 2016-04-06 01:03:28]

lgtm

[tkchin_webrtc, 2016-04-06 22:04:49]

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/api.gyp
File webrtc/api/api.gyp (right):

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/api.gyp#newco...
webrtc/api/api.gyp:175: 'objc/RTCRtpEncodingParameters+Private.h',
add to GN file too

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
File webrtc/api/objc/RTCRtpEncodingParameters.mm (right):

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:24: parameters.max_bitrate_bps =
[_maxBitrateBps intValue];
dot syntax for properties

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:33: //TODO(skvlad): Replace with
rtc::Optional once the C++ code is updated.
nit: space after. // TODO

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:38: _maxBitrateBps = nil;
not needed. ivar objects are initialized to nil

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:44: - (instancetype)init {
this should be declared after static const int kBitrateUnlimited. See ObjC style
guide.
you can just do
- (instancetype)init {
  return [super init];
}

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

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpPa...
webrtc/api/objc/RTCRtpParameters.mm:39: - (instancetype)init {
ditto

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpSe...
File webrtc/api/objc/RTCRtpSender.mm (right):

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpSe...
webrtc/api/objc/RTCRtpSender.mm:45: return [[RTCMediaStreamTrack alloc]
initWithNativeTrack:nativeTrack];
is this safe? If someone decides to store RTCMediaStreamTrack in ObjC land
somewhere after getting it here, is it possible for the track to change
underneath? In that case, would it be better to cache a copy here and just
change the native track on the cached RTCMediaStreamTrack instead?

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpSe...
webrtc/api/objc/RTCRtpSender.mm:48: }
nit: blank line after }

[skvlad, 2016-04-08 19:33:19]

Addressed code review comments.

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/api.gyp
File webrtc/api/api.gyp (right):

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/api.gyp#newco...
webrtc/api/api.gyp:175: 'objc/RTCRtpEncodingParameters+Private.h',
On 2016/04/06 22:04:48, tkchin_webrtc wrote:
> add to GN file too

Added commented-out lines, just like for the rest of the API. Commented-out code
(including build rules) may still get out of date rapidly.

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
File webrtc/api/objc/RTCRtpEncodingParameters.mm (right):

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:24: parameters.max_bitrate_bps =
[_maxBitrateBps intValue];
On 2016/04/06 22:04:48, tkchin_webrtc wrote:
> dot syntax for properties

Done.

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:33: //TODO(skvlad): Replace with
rtc::Optional once the C++ code is updated.
On 2016/04/06 22:04:48, tkchin_webrtc wrote:
> nit: space after. // TODO

Done.

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:38: _maxBitrateBps = nil;
On 2016/04/06 22:04:48, tkchin_webrtc wrote:
> not needed. ivar objects are initialized to nil

Done. I thought this made it clearer that the value for an unlimited bitrate is
nil, but the comment on the property is likely enough.

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:44: - (instancetype)init {
On 2016/04/06 22:04:48, tkchin_webrtc wrote:
> this should be declared after static const int kBitrateUnlimited. See ObjC
style
> guide.
> you can just do
> - (instancetype)init {
>   return [super init];
> }

Done.

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

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpPa...
webrtc/api/objc/RTCRtpParameters.mm:39: - (instancetype)init {
On 2016/04/06 22:04:49, tkchin_webrtc wrote:
> ditto

Done.

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpSe...
File webrtc/api/objc/RTCRtpSender.mm (right):

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpSe...
webrtc/api/objc/RTCRtpSender.mm:45: return [[RTCMediaStreamTrack alloc]
initWithNativeTrack:nativeTrack];
On 2016/04/06 22:04:49, tkchin_webrtc wrote:
> is this safe? If someone decides to store RTCMediaStreamTrack in ObjC land
> somewhere after getting it here, is it possible for the track to change
> underneath? In that case, would it be better to cache a copy here and just
> change the native track on the cached RTCMediaStreamTrack instead?

As discussed offline, returning a new track each time is conditionally safe if
the application compares them with isEqual: and not ==. However, we need to
decide a general approach for managing object identity in both Java and ObjC
wrappers. I have opened a bug for that:
https://bugs.chromium.org/p/webrtc/issues/detail?id=5756

https://codereview.webrtc.org/1854393002/diff/100001/webrtc/api/objc/RTCRtpSe...
webrtc/api/objc/RTCRtpSender.mm:48: }
On 2016/04/06 22:04:49, tkchin_webrtc wrote:
> nit: blank line after }

Done.

[tkchin_webrtc, 2016-04-08 22:45:47]

lgtm

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCMedia...
File webrtc/api/objc/RTCMediaStreamTrack.mm (right):

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCMedia...
webrtc/api/objc/RTCMediaStreamTrack.mm:54: if (![object
isKindOfClass:[RTCMediaStreamTrack class]]) {
[object isMemberOfClass:[self class]]? Do we care about subclasses?

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCMedia...
webrtc/api/objc/RTCMediaStreamTrack.mm:61: return (NSUInteger)
_nativeTrack.get();
nit: remove space
Is this a safe conversion?

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCMedia...
webrtc/api/objc/RTCMediaStreamTrack.mm:101: return _nativeTrack ==
track->_nativeTrack;
nit: don't use -> if possible
just access the nativeTrack property
also, I assume it's safe to do == on refptr like that?

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCRtpEn...
File webrtc/api/objc/RTCRtpEncodingParameters.mm (right):

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:25: (const
webrtc::RtpEncodingParameters &)nativeParameters {
nit: indent 4

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:31: [NSNumber
numberWithInt:nativeParameters.max_bitrate_bps];
nit: indent 4

[skvlad, 2016-04-08 23:23:23]

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCMedia...
File webrtc/api/objc/RTCMediaStreamTrack.mm (right):

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCMedia...
webrtc/api/objc/RTCMediaStreamTrack.mm:54: if (![object
isKindOfClass:[RTCMediaStreamTrack class]]) {
On 2016/04/08 22:45:46, tkchin_webrtc wrote:
> [object isMemberOfClass:[self class]]? Do we care about subclasses?

Changed to isMemberOfClass:.

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCMedia...
webrtc/api/objc/RTCMediaStreamTrack.mm:61: return (NSUInteger)
_nativeTrack.get();
On 2016/04/08 22:45:46, tkchin_webrtc wrote:
> nit: remove space
> Is this a safe conversion?

It's safe enough for a hash function, even if two different truncated pointers
end up having the same value. Two equal pointers will remain equal after
truncation.

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCMedia...
webrtc/api/objc/RTCMediaStreamTrack.mm:101: return _nativeTrack ==
track->_nativeTrack;
On 2016/04/08 22:45:46, tkchin_webrtc wrote:
> nit: don't use -> if possible
> just access the nativeTrack property
> also, I assume it's safe to do == on refptr like that?

Changed to use the property. refptr has operator== overridden to compare the
underlying raw pointers, so it will be doing the right thing here.

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCRtpEn...
File webrtc/api/objc/RTCRtpEncodingParameters.mm (right):

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:25: (const
webrtc::RtpEncodingParameters &)nativeParameters {
On 2016/04/08 22:45:47, tkchin_webrtc wrote:
> nit: indent 4

Done.

https://codereview.webrtc.org/1854393002/diff/140001/webrtc/api/objc/RTCRtpEn...
webrtc/api/objc/RTCRtpEncodingParameters.mm:31: [NSNumber
numberWithInt:nativeParameters.max_bitrate_bps];
On 2016/04/08 22:45:47, tkchin_webrtc wrote:
> nit: indent 4

Done.

[skvlad, 2016-04-08 23:27:26]

Patchset #6 (id:160001) has been deleted

[skvlad, 2016-04-09 00:09:44]

The CQ bit was checked by skvlad@webrtc.org

[skvlad, 2016-04-09 00:09:45]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1854393002/#ps180001 (title:
"CR feedback")

[commit-bot: I haz the power, 2016-04-09 00:09:50]

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

[commit-bot: I haz the power, 2016-04-09 00:28:59]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-04-09 00:29:06]

Description was changed from

==========
Objective C API to read and set RtpParameters

This change adds the Objective C API functions to get and set RtpSender's
RtpParameters, which allows setting bitrate limits for audio and video and
turning off RtpSenders to pre-initialize the encoder.

This CL adds only the smallest set of methods required to support bitrate
limiting - there is no way to create an RtpSender, for example, or to set
its track. The only supported functionality is this:
 	RTCPeerConnection.senders - a read-only property returning the array
	  of all RTCRtpSenders for the connection.
        RTCRtpSender.parameters - a read-only property returning the current
    	  parameters
	RTCRtpSender.setParameters: - a method to change the parameters.
	RTCRtpSender.track - a read-only property returning the
	  RTCMediaStreamTrack corresponding to the sender. It is necessary
	  to be able to identify RTCRtpSenders for video and audio. The
	  track object is of the base RTCMediaStreamTrack type, not of the
          specific subclass for audio and video - just like it is in the
	  Java API.

BUG=
==========

to

==========
Objective C API to read and set RtpParameters

This change adds the Objective C API functions to get and set RtpSender's
RtpParameters, which allows setting bitrate limits for audio and video and
turning off RtpSenders to pre-initialize the encoder.

This CL adds only the smallest set of methods required to support bitrate
limiting - there is no way to create an RtpSender, for example, or to set
its track. The only supported functionality is this:
 	RTCPeerConnection.senders - a read-only property returning the array
	  of all RTCRtpSenders for the connection.
        RTCRtpSender.parameters - a read-only property returning the current
    	  parameters
	RTCRtpSender.setParameters: - a method to change the parameters.
	RTCRtpSender.track - a read-only property returning the
	  RTCMediaStreamTrack corresponding to the sender. It is necessary
	  to be able to identify RTCRtpSenders for video and audio. The
	  track object is of the base RTCMediaStreamTrack type, not of the
          specific subclass for audio and video - just like it is in the
	  Java API.

BUG=

Committed: https://crrev.com/79b4b8720d151a773a42212d52949e8604c258c5
Cr-Commit-Position: refs/heads/master@{#12297}
==========

[commit-bot: I haz the power, 2016-04-09 00:29:07]

Patchset 6 (id:??) landed as
https://crrev.com/79b4b8720d151a773a42212d52949e8604c258c5
Cr-Commit-Position: refs/heads/master@{#12297}