Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555483005:
Add ability to scale to arbitrary factors (Closed)
|
Created:
4 years ago by kthelgason Modified:
4 years ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd ability to scale to arbitrary factors
This CL adds a fallback for the case when no optimized scale factor produces a low enough resolution for what was requested. It also ensures that all resolutions provided by the video adapter are divisible by four. This is required by some hardware implementations.
BUG=webrtc:6837
Committed: https://crrev.com/710c335d785b104bda4a912bd7909e4d27f9b04f
Cr-Commit-Position: refs/heads/master@{#15469}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Make resolution divisor configurable #Patch Set 3 : Code review nit #Patch Set 4 : rebase #
Total comments: 22
Patch Set 5 : Code review #
Total comments: 2
Patch Set 6 : Forgot an implementation #Patch Set 7 : update comment #
Messages
Total messages: 31 (21 generated)
Description was changed from ========== Add ability to scale to arbitrary factors BUG=webrtc:6837 ========== to ========== Add ability to scale to arbitrary factors This CL adds a fallback for the case when no optimized scale factor produces a low enough resolution for what was requested. It also ensures that all resolutions provided by the video adapter are divisible by four. This is required by some hardware implementations. BUG=webrtc:6837 ==========
magjed@webrtc.org changed reviewers: + magjed@webrtc.org
I couldn't spot any obvious error. https://codereview.webrtc.org/2555483005/diff/1/webrtc/media/base/videoadapte... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2555483005/diff/1/webrtc/media/base/videoadapte... webrtc/media/base/videoadapter.cc:60: if (diff < 0) { nit: I think this logic is cleaner: if (test_num_pixels <= target_num_pixels) return best_scale; ...modify best_scale...
It looks like you can reproduce the failing test reliably, so I would recommend bisecting your code changes until you find the problem. For example, remove the new kResolutionRequiredDivisor logic and put a RTC_CHECK(false) in the new FindScaleLessThanOrEqual function since it should never be executed.
The CQ bit was checked by kthelgason@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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13195) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15552) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15410) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/20091) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/20847) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11142)
The CQ bit was checked by kthelgason@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.
PTAL. I've tested this in AppRTCMobile on device, and it seems to fix the issue for the MediaCodec encoder. https://codereview.webrtc.org/2555483005/diff/1/webrtc/media/base/videoadapte... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2555483005/diff/1/webrtc/media/base/videoadapte... webrtc/media/base/videoadapter.cc:60: if (diff < 0) { On 2016/12/06 16:35:08, magjed_webrtc wrote: > nit: I think this logic is cleaner: > if (test_num_pixels <= target_num_pixels) > return best_scale; > ...modify best_scale... Acknowledged.
https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/adapted... File webrtc/media/base/adaptedvideotracksource.h (right): https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/adapted... webrtc/media/base/adaptedvideotracksource.h:56: virtual cricket::VideoAdapter* video_adapter() { return &video_adapter_; } Don't make this virtual. Make a protected AdaptedVideoTrackSource ctor that takes the alignment for video_adapter_ as an argument instead. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:44: int roundUp(int valueToRound, int multiple, int maxValue) { nit: I know it's unrelated but can you please clean up my naming mistake, i.e. rename |valueToRound| to |value_to_round| and same for |maxValue|. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:51: // Start searching from the last of the optimal fractions; It would be nice with a high level comment explaining what this functions does. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:57: if (target_num_pixels >= test_num_pixels) { Can we do like this instead: const float target_scale = sqrt(target_num_pixels / static_cast<float>(input_num_pixels)); Fraction best_scale = kScaleFractions[arraysize(kScaleFractions) - 1]; while (best_scale.numerator > target_scale * best_scale.denominator) { ... } ? https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:58: if (best_scale.numerator == 1) { nit: I think this makes the intent clearer: if (best_scale.numerator % 3 == 0 && best_scale.denominator % 2 == 0) { // Multiply with 2/3. best_scale.numerator /= 3; best_scale.denominator /= 2; } else { // Multiply with 3/4. best_scale.numerator *= 3; best_scale.denominator *= 4; } https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:124: // Try scale just above |max_pixel_count_step_up_|. I find the code in the existing FindOptimizedScaleLessThanOrEqual and FindOptimizedScaleLargerThan really ugly. I think it would be cleaner to remove them and do std::upper_bound on kScaleFractions directly here + logic for handling the max_pixel_count_step_up. But this is totally unrelated so do it only if you want and have time. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:255: // scale factor. Make sure the resulting dimensions are a multiple of 4 Update the comment. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:263: RTC_DCHECK_EQ( I think it's more clear to do: RTC_DCHECK_EQ(0, *cropped_width % scale.denominator); and RTC_DCHECK_EQ(0, *out_width % required_resolution_divisor_); https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (right): https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:28: VideoAdapter(int required_resolution_divisor); nit: I would name this alignment instead of divisor. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:68: int required_resolution_divisor_; const https://codereview.webrtc.org/2555483005/diff/60001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidvideotracksource.h (right): https://codereview.webrtc.org/2555483005/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidvideotracksource.h:71: cricket::VideoAdapter video_adapter_; Now this class has two adapters. Pass the alignment to the base class instead.
https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/adapted... File webrtc/media/base/adaptedvideotracksource.h (right): https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/adapted... webrtc/media/base/adaptedvideotracksource.h:56: virtual cricket::VideoAdapter* video_adapter() { return &video_adapter_; } On 2016/12/07 13:44:51, magjed_webrtc wrote: > Don't make this virtual. Make a protected AdaptedVideoTrackSource ctor that > takes the alignment for video_adapter_ as an argument instead. Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:44: int roundUp(int valueToRound, int multiple, int maxValue) { On 2016/12/07 13:44:51, magjed_webrtc wrote: > nit: I know it's unrelated but can you please clean up my naming mistake, i.e. > rename |valueToRound| to |value_to_round| and same for |maxValue|. Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:51: // Start searching from the last of the optimal fractions; On 2016/12/07 13:44:51, magjed_webrtc wrote: > It would be nice with a high level comment explaining what this functions does. Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:57: if (target_num_pixels >= test_num_pixels) { On 2016/12/07 13:44:51, magjed_webrtc wrote: > Can we do like this instead: > const float target_scale = > sqrt(target_num_pixels / static_cast<float>(input_num_pixels)); > Fraction best_scale = kScaleFractions[arraysize(kScaleFractions) - 1]; > while (best_scale.numerator > target_scale * best_scale.denominator) { > ... > } > ? Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:58: if (best_scale.numerator == 1) { On 2016/12/07 13:44:51, magjed_webrtc wrote: > nit: I think this makes the intent clearer: > if (best_scale.numerator % 3 == 0 && best_scale.denominator % 2 == 0) { > // Multiply with 2/3. > best_scale.numerator /= 3; > best_scale.denominator /= 2; > } else { > // Multiply with 3/4. > best_scale.numerator *= 3; > best_scale.denominator *= 4; > } Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:124: // Try scale just above |max_pixel_count_step_up_|. On 2016/12/07 13:44:51, magjed_webrtc wrote: > I find the code in the existing FindOptimizedScaleLessThanOrEqual and > FindOptimizedScaleLargerThan really ugly. I think it would be cleaner to remove > them and do std::upper_bound on kScaleFractions directly here + logic for > handling the max_pixel_count_step_up. > > But this is totally unrelated so do it only if you want and have time. I agree that this code is more complex than it needs to be, but in the interest of keeping this CL small and landing it sooner as is does fix an actual crash I'd like to do it in a follow-up CL. I filed 6848 and assigned to me to track it. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:255: // scale factor. Make sure the resulting dimensions are a multiple of 4 On 2016/12/07 13:44:51, magjed_webrtc wrote: > Update the comment. Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:263: RTC_DCHECK_EQ( On 2016/12/07 13:44:51, magjed_webrtc wrote: > I think it's more clear to do: > RTC_DCHECK_EQ(0, *cropped_width % scale.denominator); > and > RTC_DCHECK_EQ(0, *out_width % required_resolution_divisor_); Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (right): https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:28: VideoAdapter(int required_resolution_divisor); On 2016/12/07 13:44:51, magjed_webrtc wrote: > nit: I would name this alignment instead of divisor. Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:68: int required_resolution_divisor_; On 2016/12/07 13:44:51, magjed_webrtc wrote: > const Done. https://codereview.webrtc.org/2555483005/diff/60001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidvideotracksource.h (right): https://codereview.webrtc.org/2555483005/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidvideotracksource.h:71: cricket::VideoAdapter video_adapter_; On 2016/12/07 13:44:51, magjed_webrtc wrote: > Now this class has two adapters. Pass the alignment to the base class instead. Thanks, I didn't think of that. Done.
The CQ bit was checked by kthelgason@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: android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...) android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)
The CQ bit was checked by kthelgason@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/...
lgtm % nit https://codereview.webrtc.org/2555483005/diff/80001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2555483005/diff/80001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:42: // Round |valueToRound| to a multiple of |multiple|. Prefer rounding upwards, nit: update the variable names in these comments as well https://codereview.webrtc.org/2555483005/diff/80001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidvideotracksource.cc (right): https://codereview.webrtc.org/2555483005/diff/80001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidvideotracksource.cc:21: : AdaptedVideoTrackSource(2), Wasn't alignment to 4 needed to avoid the H264 encoder crash?
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2555483005/#ps120001 (title: "update comment")
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": 120001, "attempt_start_ts": 1481188561490810,
"parent_rev": "4b2557edb6e2c41cdd68c4ba9d96a1c99a698a79", "commit_rev":
"53ff0b70e583e45d3bfe44bbc14c403032191600"}
Message was sent while issue was closed.
Description was changed from ========== Add ability to scale to arbitrary factors This CL adds a fallback for the case when no optimized scale factor produces a low enough resolution for what was requested. It also ensures that all resolutions provided by the video adapter are divisible by four. This is required by some hardware implementations. BUG=webrtc:6837 ========== to ========== Add ability to scale to arbitrary factors This CL adds a fallback for the case when no optimized scale factor produces a low enough resolution for what was requested. It also ensures that all resolutions provided by the video adapter are divisible by four. This is required by some hardware implementations. BUG=webrtc:6837 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add ability to scale to arbitrary factors This CL adds a fallback for the case when no optimized scale factor produces a low enough resolution for what was requested. It also ensures that all resolutions provided by the video adapter are divisible by four. This is required by some hardware implementations. BUG=webrtc:6837 ========== to ========== Add ability to scale to arbitrary factors This CL adds a fallback for the case when no optimized scale factor produces a low enough resolution for what was requested. It also ensures that all resolutions provided by the video adapter are divisible by four. This is required by some hardware implementations. BUG=webrtc:6837 Committed: https://crrev.com/710c335d785b104bda4a912bd7909e4d27f9b04f Cr-Commit-Position: refs/heads/master@{#15469} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/710c335d785b104bda4a912bd7909e4d27f9b04f Cr-Commit-Position: refs/heads/master@{#15469}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.webrtc.org/2557323002/ by kthelgason@webrtc.org. The reason for reverting is: Issue discovered with scaling back up.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
