Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2354923002:
Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. (Closed)
|
Created:
4 years, 3 months ago by sakal-chromium Modified:
3 years, 11 months ago Reviewers:
tommi (sloooow) - chröme, agrieve, picksi1, boliu, mcasas, magjed_chromium, kerz_chromium CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, tommi Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate FPS selection logic in VideoCaptureCamera to select a wider FPS range.
The code fixes the bug referenced by the bug field. If requested fps is 15 and
the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously,
we would choose 15 - 15 fps. This is bad because it doesn't leave the camera
room to adjust based on the lighting conditions. Heuristic copied from WebRTC
chooses the 10 - 20 fps range instead.
BUG=630266
Review-Url: https://codereview.chromium.org/2354923002
Cr-Commit-Position: refs/heads/master@{#441755}
Committed: https://chromium.googlesource.com/chromium/src/+/072357660ad4bc52bcfb4c3c4bb4ad71730b4367
Patch Set 1 : Fix bug. #Patch Set 2 : Fix a typo in a comment. #Patch Set 3 : Make things public. #Patch Set 4 : Update comment to reference file instead of cs. #
Total comments: 9
Patch Set 5 : Changes according to comments. #
Total comments: 1
Patch Set 6 : Rebase. #Patch Set 7 : Rebase. #Patch Set 8 : Use WebRTC getClosestSupportedFramerateRange. #Patch Set 9 : Revert back to copying the code. #
Total comments: 6
Patch Set 10 : Match WebRTC behavior exactly. #Patch Set 11 : Rebase. #
Messages
Total messages: 68 (32 generated)
Description was changed from ========== Update FPS selection logic in VideoCaptureCamera to select wider FPS range. Previously, the logic would choose a tight FPS range around the target FPS. However, this is not good because it doesn't leave the camera room to adjust based on the lighting conditions. BUG=630266 ========== to ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. Previously, the logic would choose a tight FPS range around the target FPS. However, this is not good because it doesn't leave the camera room to adjust based on the lighting conditions. This logic is mostly copied from WebRTC and is working great there. BUG=630266 ==========
sakal@chromium.org changed reviewers: + magjed@chromium.org
PTAL
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by sakal@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by sakal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sakal@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sakal@chromium.org changed reviewers: + mcasas@chromium.org
PTAL
Ping mcasas.
Looking good. https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:175: // Progressive penalty if the upper bound is further away than |MAX_FPS_DIFF_THRESHOLD| Here and in l.181 I'd s/Progressive penalty/Threshold and penalty weights/ https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:196: MIN_FPS_LOW_VALUE_WEIGHT, MIN_FPS_HIGH_VALUE_WEIGHT); Haven't looked in depth and done the math, but to ease developers' lives, and looking at the values written in l.182-184, I'd write this as: int diff(FramerateRange range) { final int minFpsError = progressivePenalty(Math.abs(targetFramerate - range.min), MIN_FPS_DIFF_THRESHOLD, MIN_FPS_LOW_DIFF_WEIGHT, MIN_FPS_HIGH_DIFF_WEIGHT); final int maxFpsError = progressivePenalty(Math.abs(targetFramerate - range.max), MAX_FPS_DIFF_THRESHOLD, MAX_FPS_LOW_DIFF_WEIGHT, MAX_FPS_HIGH_DIFF_WEIGHT); return minFpsError + maxFpsError; } and rename s/MIN_FPS_THRESHOLD/MIN_FPS_DIFF_THRESHOLD/ s/MIN_FPS_LOW_VALUE_WEIGHT/MIN_FPS_LOW_DIFF_WEIGHT/ s/MIN_FPS_HIGH_VALUE_WEIGHT/MIN_FPS_HIGH_DIFF_WEIGHT/ which better reflects that the algorithm is symmetrical except for the used coefficients in said lines. https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:157: FramerateRange chosenFramerateRange = final https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:159: int[] chosenFpsRange = new int[] {chosenFramerateRange.min, chosenFramerateRange.max}; final
https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:175: // Progressive penalty if the upper bound is further away than |MAX_FPS_DIFF_THRESHOLD| On 2016/09/29 21:24:35, mcasas wrote: > Here and in l.181 I'd > s/Progressive penalty/Threshold and penalty weights/ Done. https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:196: MIN_FPS_LOW_VALUE_WEIGHT, MIN_FPS_HIGH_VALUE_WEIGHT); On 2016/09/29 21:24:35, mcasas wrote: > Haven't looked in depth and done the math, but to ease > developers' lives, and looking at the values written in > l.182-184, I'd write this as: > > int diff(FramerateRange range) { > final int minFpsError = progressivePenalty(Math.abs(targetFramerate - > range.min), > MIN_FPS_DIFF_THRESHOLD, MIN_FPS_LOW_DIFF_WEIGHT, > MIN_FPS_HIGH_DIFF_WEIGHT); > final int maxFpsError = progressivePenalty(Math.abs(targetFramerate - > range.max), > MAX_FPS_DIFF_THRESHOLD, MAX_FPS_LOW_DIFF_WEIGHT, > MAX_FPS_HIGH_DIFF_WEIGHT); > return minFpsError + maxFpsError; > } > > and rename > s/MIN_FPS_THRESHOLD/MIN_FPS_DIFF_THRESHOLD/ > s/MIN_FPS_LOW_VALUE_WEIGHT/MIN_FPS_LOW_DIFF_WEIGHT/ > s/MIN_FPS_HIGH_VALUE_WEIGHT/MIN_FPS_HIGH_DIFF_WEIGHT/ > > which better reflects that the algorithm is symmetrical > except for the used coefficients in said lines. The algorithm is not symmetrical. We are not trying to find a lower FPS close to the the target FPS. Instead, we are trying to find a FPS as low as possible but especially not over MIN_FPS_THRESHOLD or target FPS. The reason behind this is that we want to give the camera room to adjust based on the lighting conditions. https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:157: FramerateRange chosenFramerateRange = On 2016/09/29 21:24:35, mcasas wrote: > final Made everything possible that was changed final. https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:159: int[] chosenFpsRange = new int[] {chosenFramerateRange.min, chosenFramerateRange.max}; On 2016/09/29 21:24:35, mcasas wrote: > final Done.
Miguel - ping.
https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:196: MIN_FPS_LOW_VALUE_WEIGHT, MIN_FPS_HIGH_VALUE_WEIGHT); On 2016/09/30 13:19:32, sakal-chromium wrote: > On 2016/09/29 21:24:35, mcasas wrote: > > Haven't looked in depth and done the math, but to ease > > developers' lives, and looking at the values written in > > l.182-184, I'd write this as: > > > > int diff(FramerateRange range) { > > final int minFpsError = progressivePenalty(Math.abs(targetFramerate - > > range.min), > > MIN_FPS_DIFF_THRESHOLD, MIN_FPS_LOW_DIFF_WEIGHT, > > MIN_FPS_HIGH_DIFF_WEIGHT); > > final int maxFpsError = progressivePenalty(Math.abs(targetFramerate - > > range.max), > > MAX_FPS_DIFF_THRESHOLD, MAX_FPS_LOW_DIFF_WEIGHT, > > MAX_FPS_HIGH_DIFF_WEIGHT); > > return minFpsError + maxFpsError; > > } > > > > and rename > > s/MIN_FPS_THRESHOLD/MIN_FPS_DIFF_THRESHOLD/ > > s/MIN_FPS_LOW_VALUE_WEIGHT/MIN_FPS_LOW_DIFF_WEIGHT/ > > s/MIN_FPS_HIGH_VALUE_WEIGHT/MIN_FPS_HIGH_DIFF_WEIGHT/ > > > > which better reflects that the algorithm is symmetrical > > except for the used coefficients in said lines. > > The algorithm is not symmetrical. We are not trying to find a lower FPS close to > the the target FPS. Instead, we are trying to find a FPS as low as possible but > especially not over MIN_FPS_THRESHOLD or target FPS. > > The reason behind this is that we want to give the camera room to adjust based > on the lighting conditions. I understand the goal, but the algorithm looks obscure to me, although I've tried a few times. That might indicate that the idea is good but the implementation is unclear. Could you give it another go at writing the implementation so that it reveals (more) intent? https://codereview.chromium.org/2354923002/diff/120001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2354923002/diff/120001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:30: protected static class FramerateRange { Why not extending android.util.Range<T> ? https://developer.android.com/reference/android/util/Range.html
On 2016/10/18 17:30:44, mcasas wrote: > https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... > File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java > (right): > > https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... > media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:196: > MIN_FPS_LOW_VALUE_WEIGHT, MIN_FPS_HIGH_VALUE_WEIGHT); > On 2016/09/30 13:19:32, sakal-chromium wrote: > > On 2016/09/29 21:24:35, mcasas wrote: > > > Haven't looked in depth and done the math, but to ease > > > developers' lives, and looking at the values written in > > > l.182-184, I'd write this as: > > > > > > int diff(FramerateRange range) { > > > final int minFpsError = progressivePenalty(Math.abs(targetFramerate > - > > > range.min), > > > MIN_FPS_DIFF_THRESHOLD, MIN_FPS_LOW_DIFF_WEIGHT, > > > MIN_FPS_HIGH_DIFF_WEIGHT); > > > final int maxFpsError = progressivePenalty(Math.abs(targetFramerate > - > > > range.max), > > > MAX_FPS_DIFF_THRESHOLD, MAX_FPS_LOW_DIFF_WEIGHT, > > > MAX_FPS_HIGH_DIFF_WEIGHT); > > > return minFpsError + maxFpsError; > > > } > > > > > > and rename > > > s/MIN_FPS_THRESHOLD/MIN_FPS_DIFF_THRESHOLD/ > > > s/MIN_FPS_LOW_VALUE_WEIGHT/MIN_FPS_LOW_DIFF_WEIGHT/ > > > s/MIN_FPS_HIGH_VALUE_WEIGHT/MIN_FPS_HIGH_DIFF_WEIGHT/ > > > > > > which better reflects that the algorithm is symmetrical > > > except for the used coefficients in said lines. > > > > The algorithm is not symmetrical. We are not trying to find a lower FPS close > to > > the the target FPS. Instead, we are trying to find a FPS as low as possible > but > > especially not over MIN_FPS_THRESHOLD or target FPS. > > > > The reason behind this is that we want to give the camera room to adjust based > > on the lighting conditions. > > I understand the goal, but the algorithm looks obscure to > me, although I've tried a few times. That might indicate > that the idea is good but the implementation is unclear. > Could you give it another go at writing the implementation > so that it reveals (more) intent? I would rather keep the current implementation copied from WebRTC since it has already been used for a while and is stable. Rewriting it would have a danger of introducing bugs. > https://codereview.chromium.org/2354923002/diff/120001/media/capture/video/an... > File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java > (right): > > https://codereview.chromium.org/2354923002/diff/120001/media/capture/video/an... > media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:30: > protected static class FramerateRange { > Why not extending android.util.Range<T> ? > > https://developer.android.com/reference/android/util/Range.html It is only available from API level 21 forward so we can't use it yet.
On 2016/10/19 08:00:46, sakal-chromium wrote: > On 2016/10/18 17:30:44, mcasas wrote: > > > https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... > > File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java > > (right): > > > > > https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... > > media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:196: > > MIN_FPS_LOW_VALUE_WEIGHT, MIN_FPS_HIGH_VALUE_WEIGHT); > > On 2016/09/30 13:19:32, sakal-chromium wrote: > > > On 2016/09/29 21:24:35, mcasas wrote: > > > > Haven't looked in depth and done the math, but to ease > > > > developers' lives, and looking at the values written in > > > > l.182-184, I'd write this as: > > > > > > > > int diff(FramerateRange range) { > > > > final int minFpsError = > progressivePenalty(Math.abs(targetFramerate > > - > > > > range.min), > > > > MIN_FPS_DIFF_THRESHOLD, MIN_FPS_LOW_DIFF_WEIGHT, > > > > MIN_FPS_HIGH_DIFF_WEIGHT); > > > > final int maxFpsError = > progressivePenalty(Math.abs(targetFramerate > > - > > > > range.max), > > > > MAX_FPS_DIFF_THRESHOLD, MAX_FPS_LOW_DIFF_WEIGHT, > > > > MAX_FPS_HIGH_DIFF_WEIGHT); > > > > return minFpsError + maxFpsError; > > > > } > > > > > > > > and rename > > > > s/MIN_FPS_THRESHOLD/MIN_FPS_DIFF_THRESHOLD/ > > > > s/MIN_FPS_LOW_VALUE_WEIGHT/MIN_FPS_LOW_DIFF_WEIGHT/ > > > > s/MIN_FPS_HIGH_VALUE_WEIGHT/MIN_FPS_HIGH_DIFF_WEIGHT/ > > > > > > > > which better reflects that the algorithm is symmetrical > > > > except for the used coefficients in said lines. > > > > > > The algorithm is not symmetrical. We are not trying to find a lower FPS > close > > to > > > the the target FPS. Instead, we are trying to find a FPS as low as possible > > but > > > especially not over MIN_FPS_THRESHOLD or target FPS. > > > > > > The reason behind this is that we want to give the camera room to adjust > based > > > on the lighting conditions. > > > > I understand the goal, but the algorithm looks obscure to > > me, although I've tried a few times. That might indicate > > that the idea is good but the implementation is unclear. > > Could you give it another go at writing the implementation > > so that it reveals (more) intent? > > I would rather keep the current implementation copied from WebRTC since it has > already been used for a while and is stable. Rewriting it would have a danger of > introducing bugs. I understand, but code coming from 3rd parties adhere to different coding standards, which makes it necessary, sometimes, to rewrite them to Chromium code standards. If you want to keep the webrtc code, then I suggest we don't duplicate it, and just use it :) -- media can depend on third_party/webrtc (e.g. media/gpu does), so I propose you find a way to pull this method from org.webrtc package and just use it here, WDYT? > > > > https://codereview.chromium.org/2354923002/diff/120001/media/capture/video/an... > > File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java > > (right): > > > > > https://codereview.chromium.org/2354923002/diff/120001/media/capture/video/an... > > media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:30: > > protected static class FramerateRange { > > Why not extending android.util.Range<T> ? > > > > https://developer.android.com/reference/android/util/Range.html > > It is only available from API level 21 forward so we can't use it yet.
On 2016/10/19 16:28:21, mcasas wrote: > On 2016/10/19 08:00:46, sakal-chromium wrote: > > On 2016/10/18 17:30:44, mcasas wrote: > > > > > > https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... > > > File > media/capture/video/android/java/src/org/chromium/media/VideoCapture.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2354923002/diff/100001/media/capture/video/an... > > > > media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:196: > > > MIN_FPS_LOW_VALUE_WEIGHT, MIN_FPS_HIGH_VALUE_WEIGHT); > > > On 2016/09/30 13:19:32, sakal-chromium wrote: > > > > On 2016/09/29 21:24:35, mcasas wrote: > > > > > Haven't looked in depth and done the math, but to ease > > > > > developers' lives, and looking at the values written in > > > > > l.182-184, I'd write this as: > > > > > > > > > > int diff(FramerateRange range) { > > > > > final int minFpsError = > > progressivePenalty(Math.abs(targetFramerate > > > - > > > > > range.min), > > > > > MIN_FPS_DIFF_THRESHOLD, MIN_FPS_LOW_DIFF_WEIGHT, > > > > > MIN_FPS_HIGH_DIFF_WEIGHT); > > > > > final int maxFpsError = > > progressivePenalty(Math.abs(targetFramerate > > > - > > > > > range.max), > > > > > MAX_FPS_DIFF_THRESHOLD, MAX_FPS_LOW_DIFF_WEIGHT, > > > > > MAX_FPS_HIGH_DIFF_WEIGHT); > > > > > return minFpsError + maxFpsError; > > > > > } > > > > > > > > > > and rename > > > > > s/MIN_FPS_THRESHOLD/MIN_FPS_DIFF_THRESHOLD/ > > > > > s/MIN_FPS_LOW_VALUE_WEIGHT/MIN_FPS_LOW_DIFF_WEIGHT/ > > > > > s/MIN_FPS_HIGH_VALUE_WEIGHT/MIN_FPS_HIGH_DIFF_WEIGHT/ > > > > > > > > > > which better reflects that the algorithm is symmetrical > > > > > except for the used coefficients in said lines. > > > > > > > > The algorithm is not symmetrical. We are not trying to find a lower FPS > > close > > > to > > > > the the target FPS. Instead, we are trying to find a FPS as low as > possible > > > but > > > > especially not over MIN_FPS_THRESHOLD or target FPS. > > > > > > > > The reason behind this is that we want to give the camera room to adjust > > based > > > > on the lighting conditions. > > > > > > I understand the goal, but the algorithm looks obscure to > > > me, although I've tried a few times. That might indicate > > > that the idea is good but the implementation is unclear. > > > Could you give it another go at writing the implementation > > > so that it reveals (more) intent? > > > > I would rather keep the current implementation copied from WebRTC since it has > > already been used for a while and is stable. Rewriting it would have a danger > of > > introducing bugs. > > I understand, but code coming from 3rd parties adhere > to different coding standards, which makes it necessary, > sometimes, to rewrite them to Chromium code standards. > > If you want to keep the webrtc code, then I suggest we > don't duplicate it, and just use it :) -- media can depend > on third_party/webrtc (e.g. media/gpu does), so I propose > you find a way to pull this method from org.webrtc package > and just use it here, WDYT? > > > > > > > > > https://codereview.chromium.org/2354923002/diff/120001/media/capture/video/an... > > > File > media/capture/video/android/java/src/org/chromium/media/VideoCapture.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2354923002/diff/120001/media/capture/video/an... > > > > media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:30: > > > protected static class FramerateRange { > > > Why not extending android.util.Range<T> ? > > > > > > https://developer.android.com/reference/android/util/Range.html > > > > It is only available from API level 21 forward so we can't use it yet. ping sakal@
The CQ bit was checked by sakal@chromium.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.chromium.or...
I had to do a change in WebRTC to allow the Java classes to be used in Chromium. See the latest patch set. FYI, I measured the impact on the apk size for including the WebRTC classes. The increase was 8.6 kB. (From 46.626 MB to 46.634 MB.) I find this acceptable since we might share more Java code with WebRTC in the future.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/25 11:28:27, sakal-chromium wrote: > I had to do a change in WebRTC to allow the Java classes to be used in Chromium. > See the latest patch set. > > FYI, I measured the impact on the apk size for including the WebRTC classes. The > increase was 8.6 kB. (From 46.626 MB to 46.634 MB.) I find this acceptable since > we might share more Java code with WebRTC in the future. The changes LGTM - but please wait for more approvals
mcasas@chromium.org changed reviewers: + boliu@chromium.org
boliu@ for approval of the apk size increase mentioned in #31
boliu@chromium.org changed reviewers: + kerz@chromium.org, picksi@chromium.org
I'm not really responsible for sizes +kerz/picksi? Also I don't really understand what this feature is and why you need to pull in webrtc code. So no one can really make an informed decision. Technically, you only need CameraEnumerationAndroid, and looks like CameraEnumerationAndroid's dependency isn't that big, so perhaps it can be refactored out into a separate java lib, so you don't have to include everything in from libjingle_peerconnection_java
CameraEnumerationAndroid is pretty much self-contained. I would prefer not to modify WebRTC targets if possible. That would lead to a situation where we would have to move classes one by one from one target to another based on an external project. It should be possible to make even the JNI stuff work in the future. Then maybe WebRTC and Chromium could share more code that is similar.
also maybe you can figure out how to use proguard to strip out the unnecessary code, or maybe it's already doing that, I dunno much no one has explained yet what this code is actually for, and what's the downside of not including this code
The code fixes the bug referenced by the bug field: https://bugs.chromium.org/p/chromium/issues/detail?id=630266 Basically, if we request 15 fps, and the camera supports for example fps ranges of 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. We have to use some heuristic to choose the 10 - 20 fps range. This code provides the heuristic. In the first patch sets, I just copied the code to chromium but now the code is shared with WebRTC because of reasons discussed in the comments.
On 2016/10/27 07:40:44, sakal-chromium wrote: > The code fixes the bug referenced by the bug field: > https://bugs.chromium.org/p/chromium/issues/detail?id=630266 > > Basically, if we request 15 fps, and the camera supports for example fps ranges > of 0 - 30, 10 - 20, 14 - 16 and 15 - 15. > Previously we would choose 15 - 15 fps. This is bad because it doesn't leave the > camera room to adjust based on the lighting conditions. We have to use some > heuristic to choose the 10 - 20 fps range. This code provides the heuristic. > > In the first patch sets, I just copied the code to chromium but now the code is > shared with WebRTC because of reasons discussed in the comments. might want to put that in the description Did you look into how much code proguard is able to strip out of libjingle_peerconnection_java? is 8.6kB before or after proguard?
I was looking at the size of out/AndroidRelease/apks/ChromePublic.apk. Is that already run though proguard? If it is, the size difference might also go down once I land this CL https://codereview.webrtc.org/2448393003/ since it removes dependencies from CameraEnumerationAndroid.
Description was changed from ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. Previously, the logic would choose a tight FPS range around the target FPS. However, this is not good because it doesn't leave the camera room to adjust based on the lighting conditions. This logic is mostly copied from WebRTC and is working great there. BUG=630266 ========== to ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic imported from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 ==========
boliu@chromium.org changed reviewers: + agrieve@chromium.org
On 2016/10/27 15:18:47, sakal-chromium wrote: > I was looking at the size of out/AndroidRelease/apks/ChromePublic.apk. Is that > already run though proguard? > > If it is, the size difference might also go down once I land this CL > https://codereview.webrtc.org/2448393003/ since it removes dependencies from > CameraEnumerationAndroid. I'm not sure. Whether proguard is enabled depend on your gn settings, but I don't know which ones exactly. +agrieve who might know: rough context is this CL is pulling in java library as dependency, but only use a small subset from the library. will proguard strip out all the stuff that's not used? and what's the best way to tell the size regression will be?
On 2016/10/27 16:27:31, boliu wrote: > On 2016/10/27 15:18:47, sakal-chromium wrote: > > I was looking at the size of out/AndroidRelease/apks/ChromePublic.apk. Is that > > already run though proguard? > > > > If it is, the size difference might also go down once I land this CL > > https://codereview.webrtc.org/2448393003/ since it removes dependencies from > > CameraEnumerationAndroid. > > I'm not sure. Whether proguard is enabled depend on your gn settings, but I > don't know which ones exactly. > > +agrieve who might know: rough context is this CL is pulling in java library as > dependency, but only use a small subset from the library. will proguard strip > out all the stuff that's not used? and what's the best way to tell the size > regression will be? To check this, build with is_official_build=true with & without the change. Use "unzip -lv apks/ChromePublic.apk | grep classes" to see the size of classes.dex
We had an offline discussion with tommi and magjed about this issue. We reached a conclusion that we don't want to add a dependency from Chromium code to WebRTC Java code. So I reverted back to copying the code to Chromium. I'd like not to rewrite the algorithm because of the danger of introducing new bugs. PTAL
Please format the CL description to 80 chars wide. https://codereview.chromium.org/2354923002/diff/200001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2354923002/diff/200001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:168: * Assumes that all framerate values are multiplied by 1000. My concerns are still valid. I'd like to have a sufficient amount of explanation here so that the code can be understood, but, if the code needs a lot of comments, it's a sign that is too complicated. You say that you won't like to rewrite the algorithm, but won't like to pull from it either, which I don't really understand, since webrtc is already a third_party/ and included pervasively in the codebase, so that gives us just the comments option. Please write here a meaningful explanation as to how the code works in addition to what is it used for/what problem it solves. https://codereview.chromium.org/2354923002/diff/200001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:194: } This is not progressive, is actually having two different weights depending on |value| <> |threshold|, and then it's linear on it with two different weights. I think this is called "piecewise linear". The comment says to "use one weight for small |value|..." but doesn't say, e.g. that the "other weight above" applies only to the portion of |value| above |threshold| to make the y(x) continuous. For this method, I'd leave the comment out and try to find a better name, e.g. twoSlopePenalty() ? twoGradientPenalty() ? https://codereview.chromium.org/2354923002/diff/200001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:200: final int maxFpsError = progressivePenalty(Math.abs(targetFramerate - range.max), These variable names do not reveal their contents, i.e. |minFpsError| is more like a |weightedDiffToMinFps| and |maxFpsError| (really |maxFpsDiffError|), could be more a |weightedDiffToMaxfps|. Part of the reason why the 'algorithm' is hard to follow is because these namings are inadequate.
Description was changed from ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic imported from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 ========== to ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic imported from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 ==========
Description was changed from ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic imported from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 ========== to ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic imported from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 ==========
Description was changed from ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic imported from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 ========== to ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic copied from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 ==========
https://codereview.chromium.org/2354923002/diff/200001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/2354923002/diff/200001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:168: * Assumes that all framerate values are multiplied by 1000. On 2016/11/02 15:21:40, mcasas wrote: > My concerns are still valid. I'd like to have a sufficient > amount of explanation here so that the code can be > understood, but, if the code needs a lot of comments, > it's a sign that is too complicated. You say that you won't > like to rewrite the algorithm, but won't like to pull from it > either, which I don't really understand, since webrtc is > already a third_party/ and included pervasively in the > codebase, so that gives us just the comments option. Please > write here a meaningful explanation as to how the code > works in addition to what is it used for/what problem it solves. Clarification about the dependency; Chromium does not depend on code from native Android/iOS WebRTC, and tommi@ wanted to avoid adding this dependency. https://codereview.chromium.org/2354923002/diff/200001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:194: } On 2016/11/02 15:21:40, mcasas wrote: > This is not progressive, is actually having two different weights > depending on |value| <> |threshold|, and then it's linear > on it with two different weights. I think this is called > "piecewise linear". > > The comment says to "use one weight for small |value|..." > but doesn't say, e.g. that the "other weight above" applies > only to the portion of |value| above |threshold| to make the > y(x) continuous. For this method, I'd leave the comment out > and try to find a better name, e.g. twoSlopePenalty() ? > twoGradientPenalty() ? I can comment about the progressivePenalty naming since I wrote it; it's like progressive tax where the rate increase as the value increases. The actual term progressive refers to that the rate progresses from low to high. The function is also piecewise linear as you say, which would be nice to comment. Maybe the function would be clearer if it's written like this: return Math.min(threshold, value) * lowWeight + Math.max(0, value - threshold) * highWeight; https://codereview.chromium.org/2354923002/diff/200001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:200: final int maxFpsError = progressivePenalty(Math.abs(targetFramerate - range.max), On 2016/11/02 15:21:40, mcasas wrote: > These variable names do not reveal their contents, i.e. > |minFpsError| is more like a |weightedDiffToMinFps| and > |maxFpsError| (really |maxFpsDiffError|), could be more > a |weightedDiffToMaxfps|. Part of the reason why the > 'algorithm' is hard to follow is because these namings > are inadequate. I would like to see all comments copied from WebRTC. I think this one is pretty explaining: // Prefer a fps range with an upper bound close to |framerate|. Also prefer a fps range with a low // lower bound, to allow the framerate to fluctuate based on lightning conditions.
Also -- I'm not sure I understand why can we not depend on WebRtc Java. As boliu@ said in [#38]: "Technically, you only need CameraEnumerationAndroid, and looks like CameraEnumerationAndroid's dependency isn't that big, so perhaps it can be refactored out into a separate java lib, so you don't have to include everything in from libjingle_peerconnection_java". So we could pull a single .java file which would amount to probably less than a KB in size. Otherwise magjed@/sakal@ can you provide some rationale as to why "We reached a conclusion that we don't want to add a dependency from Chromium code to WebRTC Java code." [#48] ? (Which was a change from "It should be possible to make even the JNI stuff work in the future. Then maybe WebRTC and Chromium could share more code that is similar." [#31])
On 2016/11/02 16:06:21, mcasas wrote: > Also -- I'm not sure I understand why can we not > depend on WebRtc Java. > > As boliu@ said in [#38]: > "Technically, you only need CameraEnumerationAndroid, and looks like > CameraEnumerationAndroid's dependency isn't that big, so perhaps it can be > refactored out into a separate java lib, so you don't have to include everything > in from libjingle_peerconnection_java". > So we could pull a single .java file which would > amount to probably less than a KB in size. > > Otherwise magjed@/sakal@ can you provide some rationale as to why > "We reached a conclusion that we don't want to add a dependency > from Chromium code to WebRTC Java code." [#48] ? > (Which was a change from > "It should be possible to make even the JNI stuff work > in the future. Then maybe WebRTC and Chromium could > share more code that is similar." [#31]) Also, if we are so (legitimately) concerned about not changing the behaviour nor introducing new bugs, we could just add java tests for this particular functionality, and then refactor at will. See e.g. MediaSourceTest.java [1] for an example. [1] https://cs.chromium.org/chromium/src/chrome/android/junit/src/org/chromium/ch...
magjed@chromium.org changed reviewers: + tommi@webrtc.org
On 2016/11/02 16:06:21, mcasas wrote: > Also -- I'm not sure I understand why can we not > depend on WebRtc Java. > > As boliu@ said in [#38]: > "Technically, you only need CameraEnumerationAndroid, and looks like > CameraEnumerationAndroid's dependency isn't that big, so perhaps it can be > refactored out into a separate java lib, so you don't have to include everything > in from libjingle_peerconnection_java". > So we could pull a single .java file which would > amount to probably less than a KB in size. > > Otherwise magjed@/sakal@ can you provide some rationale as to why > "We reached a conclusion that we don't want to add a dependency > from Chromium code to WebRTC Java code." [#48] ? > (Which was a change from > "It should be possible to make even the JNI stuff work > in the future. Then maybe WebRTC and Chromium could > share more code that is similar." [#31]) +tommi@ tommi@ simply wasn't in the loop when the comment in [#31] was made. I CC:ed him in this review so that you can discuss with him directly.
magjed@chromium.org changed reviewers: - tommi@webrtc.org
On 2016/11/02 16:29:35, magjed_chromium wrote: > On 2016/11/02 16:06:21, mcasas wrote: > > Also -- I'm not sure I understand why can we not > > depend on WebRtc Java. > > > > As boliu@ said in [#38]: > > "Technically, you only need CameraEnumerationAndroid, and looks like > > CameraEnumerationAndroid's dependency isn't that big, so perhaps it can be > > refactored out into a separate java lib, so you don't have to include > everything > > in from libjingle_peerconnection_java". > > So we could pull a single .java file which would > > amount to probably less than a KB in size. > > > > Otherwise magjed@/sakal@ can you provide some rationale as to why > > "We reached a conclusion that we don't want to add a dependency > > from Chromium code to WebRTC Java code." [#48] ? > > (Which was a change from > > "It should be possible to make even the JNI stuff work > > in the future. Then maybe WebRTC and Chromium could > > share more code that is similar." [#31]) > > +tommi@ > tommi@ simply wasn't in the loop when the comment in [#31] was made. I CC:ed him > in this review so that you can discuss with him directly. Hey, sorry about the delay. The code in WebRTC and Chromium are on different paths and address different situations. The code in WebRTC might change and in theory could be removed since we're more and more relying on external implementations. We don't want to be in a position where Chromium depends on code in WebRTC that's only there for Chromium. We also don't want to pin down the current implementation that's there for tests and examples at the risk of breaking Chrome. I see the CL is over 2 months old now. Is it still up to date?
On 2016/12/01 10:05:47, tommi (chrömium) wrote: > On 2016/11/02 16:29:35, magjed_chromium wrote: > > On 2016/11/02 16:06:21, mcasas wrote: > > > Also -- I'm not sure I understand why can we not > > > depend on WebRtc Java. > > > > > > As boliu@ said in [#38]: > > > "Technically, you only need CameraEnumerationAndroid, and looks like > > > CameraEnumerationAndroid's dependency isn't that big, so perhaps it can be > > > refactored out into a separate java lib, so you don't have to include > > everything > > > in from libjingle_peerconnection_java". > > > So we could pull a single .java file which would > > > amount to probably less than a KB in size. > > > > > > Otherwise magjed@/sakal@ can you provide some rationale as to why > > > "We reached a conclusion that we don't want to add a dependency > > > from Chromium code to WebRTC Java code." [#48] ? > > > (Which was a change from > > > "It should be possible to make even the JNI stuff work > > > in the future. Then maybe WebRTC and Chromium could > > > share more code that is similar." [#31]) > > > > +tommi@ > > tommi@ simply wasn't in the loop when the comment in [#31] was made. I CC:ed > him > > in this review so that you can discuss with him directly. > > Hey, sorry about the delay. > > The code in WebRTC and Chromium are on different paths and address different > situations. > The code in WebRTC might change and in theory could be removed since we're more > and more relying on external implementations. We don't want to be in a position > where Chromium depends on code in WebRTC that's only there for Chromium. We > also don't want to pin down the current implementation that's there for tests > and examples at the risk of breaking Chrome. > > I see the CL is over 2 months old now. Is it still up to date? I rebased the CL and verified it still works now.
On 2016/12/06 16:08:11, sakal-chromium wrote: > On 2016/12/01 10:05:47, tommi (chrömium) wrote: > > On 2016/11/02 16:29:35, magjed_chromium wrote: > > > On 2016/11/02 16:06:21, mcasas wrote: > > > > Also -- I'm not sure I understand why can we not > > > > depend on WebRtc Java. > > > > > > > > As boliu@ said in [#38]: > > > > "Technically, you only need CameraEnumerationAndroid, and looks like > > > > CameraEnumerationAndroid's dependency isn't that big, so perhaps it can be > > > > refactored out into a separate java lib, so you don't have to include > > > everything > > > > in from libjingle_peerconnection_java". > > > > So we could pull a single .java file which would > > > > amount to probably less than a KB in size. > > > > > > > > Otherwise magjed@/sakal@ can you provide some rationale as to why > > > > "We reached a conclusion that we don't want to add a dependency > > > > from Chromium code to WebRTC Java code." [#48] ? > > > > (Which was a change from > > > > "It should be possible to make even the JNI stuff work > > > > in the future. Then maybe WebRTC and Chromium could > > > > share more code that is similar." [#31]) > > > > > > +tommi@ > > > tommi@ simply wasn't in the loop when the comment in [#31] was made. I CC:ed > > him > > > in this review so that you can discuss with him directly. > > > > Hey, sorry about the delay. > > > > The code in WebRTC and Chromium are on different paths and address different > > situations. > > The code in WebRTC might change and in theory could be removed since we're > more > > and more relying on external implementations. We don't want to be in a > position > > where Chromium depends on code in WebRTC that's only there for Chromium. We > > also don't want to pin down the current implementation that's there for tests > > and examples at the risk of breaking Chrome. > > > > I see the CL is over 2 months old now. Is it still up to date? > > I rebased the CL and verified it still works now. Re. tommi@s relpy: > The code in WebRTC and Chromium are on different paths and address different > situations. > The code in WebRTC might change This kind of makes my point: if the WebRtc implementation can change, then we would like to pull in those changes here automatically. If we have two verbatim copies, then bugfixes must be propagated by hand. > and in theory could be removed since we're more > and more relying on external implementations. We don't want to be in a position > where Chromium depends on code in WebRTC that's only there for Chromium. We > also don't want to pin down the current implementation that's there for tests > and examples at the risk of breaking Chrome. That's the other side of the coin: if you guys don't think the proposed code is stable, and might be removed at short notice, then why are we landing it in Chrome? OTOH, when and if WebRtc decided to ditch this code, that'll be the moment to move it verbatim to Chromium. We have loads of examples of Chromium depending on WebRTC's data types and executables. Why is this code different? I don't see any of these question answered unequivocally, but, tommi@ is an OWNER so he can RS any changes.
On 2016/12/19 20:32:31, mcasas (OOO till Jan-3) wrote: > On 2016/12/06 16:08:11, sakal-chromium wrote: > > On 2016/12/01 10:05:47, tommi (chrömium) wrote: > > > On 2016/11/02 16:29:35, magjed_chromium wrote: > > > > On 2016/11/02 16:06:21, mcasas wrote: > > > > > Also -- I'm not sure I understand why can we not > > > > > depend on WebRtc Java. > > > > > > > > > > As boliu@ said in [#38]: > > > > > "Technically, you only need CameraEnumerationAndroid, and looks like > > > > > CameraEnumerationAndroid's dependency isn't that big, so perhaps it can > be > > > > > refactored out into a separate java lib, so you don't have to include > > > > everything > > > > > in from libjingle_peerconnection_java". > > > > > So we could pull a single .java file which would > > > > > amount to probably less than a KB in size. > > > > > > > > > > Otherwise magjed@/sakal@ can you provide some rationale as to why > > > > > "We reached a conclusion that we don't want to add a dependency > > > > > from Chromium code to WebRTC Java code." [#48] ? > > > > > (Which was a change from > > > > > "It should be possible to make even the JNI stuff work > > > > > in the future. Then maybe WebRTC and Chromium could > > > > > share more code that is similar." [#31]) > > > > > > > > +tommi@ > > > > tommi@ simply wasn't in the loop when the comment in [#31] was made. I > CC:ed > > > him > > > > in this review so that you can discuss with him directly. > > > > > > Hey, sorry about the delay. > > > > > > The code in WebRTC and Chromium are on different paths and address different > > > situations. > > > The code in WebRTC might change and in theory could be removed since we're > > more > > > and more relying on external implementations. We don't want to be in a > > position > > > where Chromium depends on code in WebRTC that's only there for Chromium. We > > > also don't want to pin down the current implementation that's there for > tests > > > and examples at the risk of breaking Chrome. > > > > > > I see the CL is over 2 months old now. Is it still up to date? > > > > I rebased the CL and verified it still works now. > > Re. tommi@s relpy: > > > The code in WebRTC and Chromium are on different paths and address different > > situations. > > The code in WebRTC might change > > This kind of makes my point: if the WebRtc implementation can change, then > we would like to pull in those changes here automatically. If we have two > verbatim copies, then bugfixes must be propagated by hand. > > > and in theory could be removed since we're more > > and more relying on external implementations. We don't want to be in a > position > > where Chromium depends on code in WebRTC that's only there for Chromium. We > > also don't want to pin down the current implementation that's there for tests > > and examples at the risk of breaking Chrome. > > That's the other side of the coin: if you guys don't think the proposed > code is stable, and might be removed at short notice, then why are > we landing it in Chrome? OTOH, when and if WebRtc decided to ditch > this code, that'll be the moment to move it verbatim to Chromium. > We have loads of examples of Chromium depending on WebRTC's data types > and executables. Why is this code different? > > I don't see any of these question answered unequivocally, but, > tommi@ is an OWNER so he can RS any changes. The webrtc code isn't intended for Chromium's use case (the sandbox changes the capture layer quite a bit), so I'm OK with that. RS lgtm as requested.
The CQ bit was checked by magjed@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2354923002/#ps240001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1483646517461580,
"parent_rev": "92899656e0d9c7fca2713a46cce563e7b75ec68f", "commit_rev":
"072357660ad4bc52bcfb4c3c4bb4ad71730b4367"}
Message was sent while issue was closed.
Description was changed from ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic copied from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 ========== to ========== Update FPS selection logic in VideoCaptureCamera to select a wider FPS range. The code fixes the bug referenced by the bug field. If requested fps is 15 and the camera supports fps ranges 0 - 30, 10 - 20, 14 - 16 and 15 - 15. Previously, we would choose 15 - 15 fps. This is bad because it doesn't leave the camera room to adjust based on the lighting conditions. Heuristic copied from WebRTC chooses the 10 - 20 fps range instead. BUG=630266 Review-Url: https://codereview.chromium.org/2354923002 Cr-Commit-Position: refs/heads/master@{#441755} Committed: https://chromium.googlesource.com/chromium/src/+/072357660ad4bc52bcfb4c3c4bb4... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/072357660ad4bc52bcfb4c3c4bb4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
