Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2877933004:
objc wrapper for PeerConnection::SetBitrate (Closed)
|
Created:
3 years, 7 months ago by Zach Stein Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org, the sun, tterriberry_mozilla.com, kwiberg-webrtc, magjed_webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
Descriptionobjc wrapper for PeerConnection::SetBitrate
BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2877933004
Cr-Commit-Position: refs/heads/master@{#19294}
Committed: https://chromium.googlesource.com/external/webrtc/+/03adb7c6dc791e36c7096ce94299975281902506
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove native stub now that the real implementation has been submitted. #
Total comments: 2
Patch Set 3 : Document return value of SetBitrate and rebase. #
Total comments: 2
Messages
Total messages: 21 (12 generated)
https://codereview.webrtc.org/2877933004/diff/1/webrtc/sdk/objc/Framework/Uni... File webrtc/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm (right): https://codereview.webrtc.org/2877933004/diff/1/webrtc/sdk/objc/Framework/Uni... webrtc/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm:63: EXPECT_TRUE([peerConnection setBitrate:nullptr toCurrent:nullptr toMax:nullptr]); Maybe [peerConnection setBitrateMinBpsTo:x currentBpsTo:y maxBpsTo:z] is better.
Description was changed from ========== objc wrapper for PeerConnection::SetBitrate Currently written against a native stub - everything outside of webrtc/sdk will be removed once the native implementation is submitted. BUG=webrtc:7395 ========== to ========== objc wrapper for PeerConnection::SetBitrate BUG=webrtc:7395 ==========
The CQ bit was checked by zstein@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zstein@webrtc.org changed reviewers: + magjed@webrtc.org
PTAL
magjed@webrtc.org changed reviewers: + denicija@webrtc.org - magjed@webrtc.org
On 2017/08/01 19:04:44, Zach Stein wrote: > PTAL Friendly ping :)
LGTM with one small comment https://codereview.webrtc.org/2877933004/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/2877933004/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:187: /** Updates bandwidth estimation parameters. Null parameters will be unchanged. */ Can you document the return parameter as well? i.e when can YES and when can NO be expected.
https://codereview.webrtc.org/2877933004/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/2877933004/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:187: /** Updates bandwidth estimation parameters. Null parameters will be unchanged. */ On 2017/08/09 08:01:38, daniela-webrtc wrote: > Can you document the return parameter as well? i.e when can YES and when can NO > be expected. Done.
The CQ bit was checked by zstein@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/2877933004/#ps40001 (title: "Document return value of SetBitrate and rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1502311543864330,
"parent_rev": "e5f8323711c9b8df0ee1d9feb65bf908772d2a5f", "commit_rev":
"03adb7c6dc791e36c7096ce94299975281902506"}
Message was sent while issue was closed.
Description was changed from ========== objc wrapper for PeerConnection::SetBitrate BUG=webrtc:7395 ========== to ========== objc wrapper for PeerConnection::SetBitrate BUG=webrtc:7395 Review-Url: https://codereview.webrtc.org/2877933004 Cr-Commit-Position: refs/heads/master@{#19294} Committed: https://chromium.googlesource.com/external/webrtc/+/03adb7c6dc791e36c7096ce94... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/03adb7c6dc791e36c7096ce94...
Message was sent while issue was closed.
tkchin@webrtc.org changed reviewers: + tkchin@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2877933004/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): https://codereview.webrtc.org/2877933004/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:190: - (BOOL)setBitrateToMin:(NSNumber *_Nullable)minBitrateBps Could you update this with more details? What bitrate is this? Data channel / video / audio? Is this specific to bandwidth estimation only? What does it mean to set the current bwe bitrate? Does that mean we are disregarding stuff from the estimator? Comment nit: "Nil parameters", Returns YES also style nit: can we update the naming to not repeat "to", and make sure the argument name matches the method name? arg nit: use (nullable NSNumber *) instead setBweMinBitrateBps:minBitrateBps currentBitrateBps:currentBitrateBps maxBitrateBps:maxBitrateBps https://codereview.webrtc.org/2877933004/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm (right): https://codereview.webrtc.org/2877933004/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm:68: toMax:nullptr]); nil not nullptr nil is for objc objects and means (id)0 nullptr is for C++ objects and means (void *)0
Message was sent while issue was closed.
On 2017/08/31 21:20:48, tkchin_webrtc wrote: > https://codereview.webrtc.org/2877933004/diff/40001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h (right): > > https://codereview.webrtc.org/2877933004/diff/40001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnection.h:190: - > (BOOL)setBitrateToMin:(NSNumber *_Nullable)minBitrateBps > Could you update this with more details? > > What bitrate is this? Data channel / video / audio? > Is this specific to bandwidth estimation only? > What does it mean to set the current bwe bitrate? Does that mean we are > disregarding stuff from the estimator? > > Comment nit: "Nil parameters", Returns YES > > also style nit: can we update the naming to not repeat "to", and make sure the > argument name matches the method name? > > arg nit: use (nullable NSNumber *) instead > > setBweMinBitrateBps:minBitrateBps > currentBitrateBps:currentBitrateBps > maxBitrateBps:maxBitrateBps > > https://codereview.webrtc.org/2877933004/diff/40001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm (right): > > https://codereview.webrtc.org/2877933004/diff/40001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm:68: > toMax:nullptr]); > nil not nullptr > > nil is for objc objects and means (id)0 > nullptr is for C++ objects and means (void *)0 Addressed here: https://codereview.chromium.org/3011013002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
