Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2492693003:
Propagate bitrate setting to RTCRtpSender. (Closed)
|
Created:
4 years, 1 month ago by daniela-webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPropagate bitrate setting to RTCRtpSender.
This CL wires everything up and enables actual setting of the max bitrate encoding parameter
on the video RTP sender.
The following changes were made
* Add maxbitrate property to the settings model and settings store. Make sure to store and
read the maxbitrate from storage (to persist between app launches and make testing easier)
* Fix setup of encoding parameters for the rtp sender as previous timing was not right.
* Fix header of RTCRtpSender to expose needed parameter
BUG=webrtc:6654
Committed: https://crrev.com/9af2b6012a782c113c1f5793ca13d5b08210d2f0
Cr-Commit-Position: refs/heads/master@{#15120}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Format #Patch Set 3 : Rename key constants #Patch Set 4 : Replace UITextFieldDelegate method with method with lesser availability value #
Total comments: 10
Patch Set 5 : Address comments #Messages
Total messages: 42 (25 generated)
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@webrtc.org
https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m (right): https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:15: static NSString *const kUserDefaultsBitrateKey = @"rtc_max_bitrate_key"; Why do these variable names contain 'UserDefaults'? I.e. wouldn't kBitrateKey be enough?
lgtm % minor comments https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right): https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:238: bitrateNumber = [NSNumber numberWithInteger:textField.text.intValue]; Do we need some error handling here, if the user types in something that's not a number? https://codereview.webrtc.org/2492693003/diff/1/webrtc/sdk/objc/Framework/Hea... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h (right): https://codereview.webrtc.org/2492693003/diff/1/webrtc/sdk/objc/Framework/Hea... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h:28: @property(nonatomic, copy) RTCRtpParameters* parameters; star should be right-leaning
https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m (right): https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:15: static NSString *const kUserDefaultsBitrateKey = @"rtc_max_bitrate_key"; On 2016/11/11 08:45:05, magjed_webrtc wrote: > Why do these variable names contain 'UserDefaults'? I.e. wouldn't kBitrateKey be > enough? No rule enforcing the UserDefaults in the name. I just like descriptive names (in true ObjC fashion) :D https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right): https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:238: bitrateNumber = [NSNumber numberWithInteger:textField.text.intValue]; On 2016/11/11 10:09:07, kthelgason wrote: > Do we need some error handling here, if the user types in something that's not a > number? That's why we allow only numerical keypad to appear, so that the user can only input numeric values. I guess it's still possible to paste alpha-numerics but in that case the numberWithInteger will fail and return nil. So technically it would be as if though nothing was entered :D
https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m (right): https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:15: static NSString *const kUserDefaultsBitrateKey = @"rtc_max_bitrate_key"; On 2016/11/11 13:45:12, denicija-webrtc wrote: > On 2016/11/11 08:45:05, magjed_webrtc wrote: > > Why do these variable names contain 'UserDefaults'? I.e. wouldn't kBitrateKey > be > > enough? > > No rule enforcing the UserDefaults in the name. I just like descriptive names > (in true ObjC fashion) :D I understand we can name the variable whatever and it will still compile. What I don't understand is why 'UserDefaults' is good to have in these variable names. For me this variable looks just like a key and including 'UserDefaults' is confusing for me since I interpret it as the key can be overridden. I see that we use these keys in an NSUserDefaults object below and that's where the name must come from, but for me it's confusing rather than descriptive.
https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m (right): https://codereview.webrtc.org/2492693003/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:15: static NSString *const kUserDefaultsBitrateKey = @"rtc_max_bitrate_key"; On 2016/11/14 13:05:27, magjed_webrtc wrote: > On 2016/11/11 13:45:12, denicija-webrtc wrote: > > On 2016/11/11 08:45:05, magjed_webrtc wrote: > > > Why do these variable names contain 'UserDefaults'? I.e. wouldn't > kBitrateKey > > be > > > enough? > > > > No rule enforcing the UserDefaults in the name. I just like descriptive names > > (in true ObjC fashion) :D > > I understand we can name the variable whatever and it will still compile. What I > don't understand is why 'UserDefaults' is good to have in these variable names. > For me this variable looks just like a key and including 'UserDefaults' is > confusing for me since I interpret it as the key can be overridden. I see that > we use these keys in an NSUserDefaults object below and that's where the name > must come from, but for me it's confusing rather than descriptive. Done.
lgtm
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2492693003/#ps60001 (title: "Replace UITextFieldDelegate method with method with lesser availability value")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10161)
Description was changed from ========== Propagate bitrate setting to RTCRtpSender. This CL wires everything up and enables actual setting of the max bitrate encoding parameter on the video RTP sender. The following changes were made * Add maxbitrate property to the settings model and settings store. Make sure to store and read the maxbitrate from storage (to persist between app launches and make testing easier) * Fix setup of encoding parameters for the rtp sender as previous timing was not right. * Fix header of RTCRtpSender to expose needed parameter BUG=webrtc:6654 ========== to ========== Propagate bitrate setting to RTCRtpSender. This CL wires everything up and enables actual setting of the max bitrate encoding parameter on the video RTP sender. The following changes were made * Add maxbitrate property to the settings model and settings store. Make sure to store and read the maxbitrate from storage (to persist between app launches and make testing easier) * Fix setup of encoding parameters for the rtp sender as previous timing was not right. * Fix header of RTCRtpSender to expose needed parameter BUG=webrtc:6654 ==========
denicija@webrtc.org changed reviewers: + tkchin@webrtc.org
https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.h (right): https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.h:15: * Model class for user defined settings. Not that this is wrong, but the rest of codebase uses different comment format for /** https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m (right): https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:20: - (NSUserDefaults *)storage { Why not store this as an instance variable? https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:24: - (nullable NSString *)videoResolutionConstraintsSetting { nit: a bit redundant to have a settingsstore object have each of its properties suffixed "setting" https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:29: [[self storage] setObject:constraintsString forKey:kMediaConstraintsKey]; It's more predictable if you force a sync after setting. Esp in demo app and when attached to debugger, some settings may not actually be persisted before app is killed. You can optimize further by caching ivars for each property to avoid user defaults disk read. https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right): https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:21: @interface ARDSettingsViewController ()<UITextFieldDelegate> { space after () https://codereview.webrtc.org/2492693003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h (right): https://codereview.webrtc.org/2492693003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h:43: - (void)setParameters:(RTCRtpParameters *)parameters; This isn't required. There is already a property above.
On 2016/11/14 21:14:32, tkchin_webrtc wrote: > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... > File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.h (right): > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... > webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.h:15: * Model class for > user defined settings. > Not that this is wrong, but the rest of codebase uses different comment format > for /** > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... > File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m (right): > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... > webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:20: - (NSUserDefaults > *)storage { > Why not store this as an instance variable? > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... > webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:24: - (nullable > NSString *)videoResolutionConstraintsSetting { > nit: a bit redundant to have a settingsstore object have each of its properties > suffixed "setting" > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... > webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:29: [[self storage] > setObject:constraintsString forKey:kMediaConstraintsKey]; > It's more predictable if you force a sync after setting. Esp in demo app and > when attached to debugger, some settings may not actually be persisted before > app is killed. You can optimize further by caching ivars for each property to > avoid user defaults disk read. > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... > File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right): > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... > webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:21: @interface > ARDSettingsViewController ()<UITextFieldDelegate> { > space after () > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h (right): > > https://codereview.webrtc.org/2492693003/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpSender.h:43: - > (void)setParameters:(RTCRtpParameters *)parameters; > This isn't required. There is already a property above. lgtm % sdk header update
The CQ bit was checked by denicija@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/...
https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m (right): https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:20: - (NSUserDefaults *)storage { On 2016/11/14 21:14:32, tkchin_webrtc wrote: > Why not store this as an instance variable? Done. https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:24: - (nullable NSString *)videoResolutionConstraintsSetting { On 2016/11/14 21:14:32, tkchin_webrtc wrote: > nit: a bit redundant to have a settingsstore object have each of its properties > suffixed "setting" Done. https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.m:29: [[self storage] setObject:constraintsString forKey:kMediaConstraintsKey]; On 2016/11/14 21:14:32, tkchin_webrtc wrote: > It's more predictable if you force a sync after setting. Esp in demo app and > when attached to debugger, some settings may not actually be persisted before > app is killed. You can optimize further by caching ivars for each property to > avoid user defaults disk read. Done. https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right): https://codereview.webrtc.org/2492693003/diff/60001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:21: @interface ARDSettingsViewController ()<UITextFieldDelegate> { On 2016/11/14 21:14:32, tkchin_webrtc wrote: > space after () Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/16154)
The CQ bit was checked by denicija@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kthelgason@webrtc.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2492693003/#ps80001 (title: "Address comments")
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
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/16158)
The CQ bit was checked by denicija@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: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/16163)
The CQ bit was checked by denicija@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/...
denicija@webrtc.org changed reviewers: - denicija@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by denicija@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Propagate bitrate setting to RTCRtpSender. This CL wires everything up and enables actual setting of the max bitrate encoding parameter on the video RTP sender. The following changes were made * Add maxbitrate property to the settings model and settings store. Make sure to store and read the maxbitrate from storage (to persist between app launches and make testing easier) * Fix setup of encoding parameters for the rtp sender as previous timing was not right. * Fix header of RTCRtpSender to expose needed parameter BUG=webrtc:6654 ========== to ========== Propagate bitrate setting to RTCRtpSender. This CL wires everything up and enables actual setting of the max bitrate encoding parameter on the video RTP sender. The following changes were made * Add maxbitrate property to the settings model and settings store. Make sure to store and read the maxbitrate from storage (to persist between app launches and make testing easier) * Fix setup of encoding parameters for the rtp sender as previous timing was not right. * Fix header of RTCRtpSender to expose needed parameter BUG=webrtc:6654 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Propagate bitrate setting to RTCRtpSender. This CL wires everything up and enables actual setting of the max bitrate encoding parameter on the video RTP sender. The following changes were made * Add maxbitrate property to the settings model and settings store. Make sure to store and read the maxbitrate from storage (to persist between app launches and make testing easier) * Fix setup of encoding parameters for the rtp sender as previous timing was not right. * Fix header of RTCRtpSender to expose needed parameter BUG=webrtc:6654 ========== to ========== Propagate bitrate setting to RTCRtpSender. This CL wires everything up and enables actual setting of the max bitrate encoding parameter on the video RTP sender. The following changes were made * Add maxbitrate property to the settings model and settings store. Make sure to store and read the maxbitrate from storage (to persist between app launches and make testing easier) * Fix setup of encoding parameters for the rtp sender as previous timing was not right. * Fix header of RTCRtpSender to expose needed parameter BUG=webrtc:6654 Committed: https://crrev.com/9af2b6012a782c113c1f5793ca13d5b08210d2f0 Cr-Commit-Position: refs/heads/master@{#15120} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9af2b6012a782c113c1f5793ca13d5b08210d2f0 Cr-Commit-Position: refs/heads/master@{#15120} |
