Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1966273002:
VideoAdapter: Add cropping based on OnOutputFormatRequest() (Closed)
|
Created:
4 years, 7 months ago by magjed_webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, AlexG Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionVideoAdapter: Add cropping based on OnOutputFormatRequest()
If OnOutputFormatRequest() is called, VideoAdapter will crop to the same
aspect ratio as the requested format. The output from
VideoAdapter.AdaptFrameResolution() now contains both how to crop the
input frame, and how to scale the cropped frame to the final adapted
resolution.
BUG=b/28622232
Committed: https://crrev.com/709f73c04eaa7aee66cafb1f5e7b28069f20d325
Cr-Commit-Position: refs/heads/master@{#12732}
Patch Set 1 : #
Total comments: 15
Patch Set 2 : Add cropping test #Patch Set 3 : Set |interval_next_frame_| to 0 in OnOutputFormatRequest() #Patch Set 4 : Adjust cropping to get even scale factor #
Total comments: 1
Patch Set 5 : reset interval_next_frame_ #Patch Set 6 : Remove unused variable #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== VideoAdapter: Add cropping based on OnOutputFormatRequest() If OnOutputFormatRequest() is called, VideoAdapter will crop to the same aspect ratio as the requested format. The output from VideoAdapter.AdaptFrameResolution() now contains both how to crop the input frame, and how to scale the cropped frame to the final adapted resolution. BUG=b/28622232 ========== to ========== VideoAdapter: Add cropping based on OnOutputFormatRequest() If OnOutputFormatRequest() is called, VideoAdapter will crop to the same aspect ratio as the requested format. The output from VideoAdapter.AdaptFrameResolution() now contains both how to crop the input frame, and how to scale the cropped frame to the final adapted resolution. BUG=b/28622232 ==========
magjed@webrtc.org changed reviewers: + nisse@webrtc.org
nisse@ - Please take a look. glaznev@ - FYI, but review if you want.
Patchset #1 (id:1) has been deleted
nisse@webrtc.org changed reviewers: + perkj@webrtc.org
I'd like perkj@ to have a look too, since he wrote most of the current version of the videoadapter. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (right): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:43: int* out_width, Would it make sense to collect the output parameters into a struct AdaptedFrameFormat or some such? I'd also consider adding the crop offset, not because it's terribly useful, but because I think it's nice to have a single place which decides all the details on how adapt the frame. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (left): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:229: // VideoAdapter dropped the frame. I'd prefer to use return value of AdaptFrameResolution to signal dropped frames. True ==> keep, false ==> drop.
perkj - Please take a look.
lgtm if one test is added. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter_unittest.cc (right): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter_unittest.cc:713: please add test for cpu adapt where input is 640*480 - request 640*360 and adapt one step down and one step up again.
https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (right): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:43: int* out_width, On 2016/05/12 09:48:46, nisse-webrtc wrote: > Would it make sense to collect the output parameters into a struct > AdaptedFrameFormat or some such? > > I'd also consider adding the crop offset, not because it's terribly useful, but > because I think it's nice to have a single place which decides all the details > on how adapt the frame. Since you are working on refactoring this already, how about I land this as is, and you can refactor it however you want afterwards? https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter_unittest.cc (right): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter_unittest.cc:713: On 2016/05/12 13:32:34, perkj_webrtc wrote: > please add test for cpu adapt where input is 640*480 - request 640*360 and adapt > one step down and one step up again. Done. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (left): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:229: // VideoAdapter dropped the frame. On 2016/05/12 09:48:46, nisse-webrtc wrote: > I'd prefer to use return value of AdaptFrameResolution to signal dropped frames. > True ==> keep, false ==> drop. Same here, you can change it in your refactor CL if you want.
https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:151: interval_next_frame_ %= requested_format_->interval; If it is ensured that input_interval < requested_format_->interval (which is the case where dropping frames make sense, right?), the expensive %= can be replaced by -=. So I'd suggest 1. Change the outer check to "requested_format_->interval > input_interval". 2. Change %= to -=. 3. Reset interval_next_frame to zero whenever requested_format_->interval is changed. This is a *very* minor performance improvement (order of 50 cycles per frame), but I also think it makes the logic a bit clearer. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:191: const float requested_aspect = I liked my integer-only crop logic in https://codereview.webrtc.org/1960073002/patch/160001/170005 I.e., something like // Size of the used portion of the input frame. crop_width = in_width; crop_height = in_height; if (... requested format specified...) { // Crop the input frame, if needed to preserve aspect ratio. int aspect_diff = crop_width * dst_height - crop_height * dst_width; if (aspect_diff > 0) { // Horizontal crop. crop_width -= aspect_diff / dst_height; } else if (aspect_diff < 0) { // Vertical crop. crop_height -= (-aspect_diff) / dst_width; } } (My cl used long for aspect_diff, to allow width and height beyond 16 bits. But since max_pixel_count is a plain int anyway, I guess a plain int should be good enough here too). https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:206: *out_width = static_cast<int>(*cropped_width * scale + .5f); But now I'm getting confused... I thought the requested format above was the intended output format. I'm trying to understand how this is supposed to work, let me think aloud. It might be a bit more natural to switch things around: Start with the requested format, compare to max pixels constraints etc, and decide on the output format. And after the output size is decided, compute cropping parameters. But then you pass the cropped size as input to the FindScale function. The purpose FindScale is to get a nice scaling ratio, right? But if so, then computing the output size with rounding seems bad, since any rounding error can destroy the ratio, changing it from, say, 0.25 to 0.2501. To get things right, I'm afraid we'd need integer logic, and either unconditionally force crop size to be multiple of 16 , or tweak the cropping after the scale is chosen. And in either case, potentially resulting in a small error in aspect ratio. Say we have the scale 3/16, and crop size 680. The out size is 127.5, say we round it down to 127 (or up to 128, neither is any good). Then input size for an exact scaling ratio would be 677.3333, which is no good. We need to round the output size down to a multiple of 3, i.e., 126. Thus, to get exact 3/16 scaling for a crop size close to 680, we'd have to select a crop size of 672 and an output size of 126. Next larger possible choice is crop size 688, output size 129. I think this ought to be fixed, maybe in this cl?
https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:151: interval_next_frame_ %= requested_format_->interval; On 2016/05/13 07:58:19, nisse-webrtc wrote: > If it is ensured that input_interval < requested_format_->interval (which is the > case where dropping frames make sense, right?), the expensive %= can be replaced > by -=. So I'd suggest > > 1. Change the outer check to "requested_format_->interval > input_interval". > > 2. Change %= to -=. > > 3. Reset interval_next_frame to zero whenever requested_format_->interval is > changed. > > This is a *very* minor performance improvement (order of 50 cycles per frame), > but I also think it makes the logic a bit clearer. 1) I think in the near future, we want to remove the fixed |input_interval_| and calculate it on a per-frame basis from the frame timestamps instead, and then that check doesn't make sense. 2) I think '%=' is more robust than '-=', because then we know for sure that we will never accumulate a big value. 3) Done. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:191: const float requested_aspect = On 2016/05/13 07:58:19, nisse-webrtc wrote: > I liked my integer-only crop logic in > > https://codereview.webrtc.org/1960073002/patch/160001/170005 > > I.e., something like > > // Size of the used portion of the input frame. > crop_width = in_width; > crop_height = in_height; > > if (... requested format specified...) { > > // Crop the input frame, if needed to preserve aspect ratio. > int aspect_diff = crop_width * dst_height - > crop_height * dst_width; > if (aspect_diff > 0) { > // Horizontal crop. > crop_width -= aspect_diff / dst_height; > } else if (aspect_diff < 0) { > // Vertical crop. > crop_height -= (-aspect_diff) / dst_width; > } > } > > (My cl used long for aspect_diff, to allow width and height beyond 16 bits. But > since max_pixel_count is a plain int anyway, I guess a plain int should be good > enough here too). Your interger-only crop is clever, but I don't see the point. This code is not performance critical and your code is longer, and IMO less intuitive. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:206: *out_width = static_cast<int>(*cropped_width * scale + .5f); On 2016/05/13 07:58:19, nisse-webrtc wrote: > But now I'm getting confused... I thought the requested format above was the > intended output format. I'm trying to understand how this is supposed to work, > let me think aloud. > > It might be a bit more natural to switch things around: Start with the requested > format, compare to max pixels constraints etc, and decide on the output format. > > And after the output size is decided, compute cropping parameters. > > But then you pass the cropped size as input to the FindScale function. The > purpose FindScale is to get a nice scaling ratio, right? But if so, then > computing the output size with rounding seems bad, since any rounding error can > destroy the ratio, changing it from, say, 0.25 to 0.2501. > > To get things right, I'm afraid we'd need integer logic, and either > unconditionally force crop size to be multiple of 16 , or tweak the cropping > after the scale is chosen. And in either case, potentially resulting in a small > error in aspect ratio. > > Say we have the scale 3/16, and crop size 680. The out size is 127.5, say we > round it down to 127 (or up to 128, neither is any good). Then input size for an > exact scaling ratio would be 677.3333, which is no good. We need to round the > output size down to a multiple of 3, i.e., 126. > > Thus, to get exact 3/16 scaling for a crop size close to 680, we'd have to > select a crop size of 672 and an output size of 126. Next larger possible choice > is crop size 688, output size 129. > > I think this ought to be fixed, maybe in this cl? I need to base the scale factor on the number of cropped input pixels, not on the requested format, that's why I'm starting with cropping. Yes, the output size is rounded and can theoretically destroy the optimized path in libyuv::Scale. This was the case before my CL as well. I think it's very unlikely given normal resolutions, and it's not directly catastrophically when it fails, it will just be slightly slower. For textures it doesn't matter at all. To get a perfect scale factor, we need to align both cropped width and height to the denominator of the scale factor. I can fix it, but I think it's an overkill.
nisse - Please take another look. Cropping is now adjusted to get a perfect scale factor.
LGTM Please consider the % comment, but I don't insist. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:151: interval_next_frame_ %= requested_format_->interval; On 2016/05/13 09:12:59, magjed_webrtc wrote: > 1) I think in the near future, we want to remove the fixed |input_interval_| and > calculate it on a per-frame basis from the frame timestamps instead, and then > that check doesn't make sense. I see. Don't bother changing that now, then. > 2) I think '%=' is more robust than '-=', because then we know for sure that we > will never accumulate a big value. I still don't quite see that % makes sense. For robustness, I think if (next >= interval) { next -= interval; if (next >= interval) next = 0; } would be more reasonable. In the case that the result of the first subtraction is too large, that means that we could have produced additional in-between frames, and the remainder from %= represents the time since the latest of those in-between frame. But since the in-between frames are non-existing, that value isn't very useful. > 3) Done. Good. https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:191: const float requested_aspect = On 2016/05/13 09:12:59, magjed_webrtc wrote: > On 2016/05/13 07:58:19, nisse-webrtc wrote: > > I liked my integer-only crop logic in > > > > https://codereview.webrtc.org/1960073002/patch/160001/170005 > > > > I.e., something like > > > > // Size of the used portion of the input frame. > > crop_width = in_width; > > crop_height = in_height; > > > > if (... requested format specified...) { > > > > // Crop the input frame, if needed to preserve aspect ratio. > > int aspect_diff = crop_width * dst_height - > > crop_height * dst_width; > > if (aspect_diff > 0) { > > // Horizontal crop. > > crop_width -= aspect_diff / dst_height; > > } else if (aspect_diff < 0) { > > // Vertical crop. > > crop_height -= (-aspect_diff) / dst_width; > > } > > } > > > > (My cl used long for aspect_diff, to allow width and height beyond 16 bits. > But > > since max_pixel_count is a plain int anyway, I guess a plain int should be > good > > enough here too). > > Your interger-only crop is clever, but I don't see the point. This code is not > performance critical and your code is longer, and IMO less intuitive. Ok. https://codereview.webrtc.org/1966273002/diff/80001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1966273002/diff/80001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:220: *cropped_width = roundUp(*cropped_width, scale.denominator, in_width); Looks nice and simple, and there's even a test case. Good!
https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1966273002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:151: interval_next_frame_ %= requested_format_->interval; On 2016/05/13 11:47:06, nisse-webrtc wrote: > On 2016/05/13 09:12:59, magjed_webrtc wrote: > > 1) I think in the near future, we want to remove the fixed |input_interval_| > and > > calculate it on a per-frame basis from the frame timestamps instead, and then > > that check doesn't make sense. > > I see. Don't bother changing that now, then. > > > 2) I think '%=' is more robust than '-=', because then we know for sure that > we > > will never accumulate a big value. > > I still don't quite see that % makes sense. For robustness, I think > > if (next >= interval) { > next -= interval; > if (next >= interval) > next = 0; > } > > would be more reasonable. In the case that the result of the first subtraction > is too large, that means that we could have produced additional in-between > frames, and the remainder from %= represents the time since the latest of those > in-between frame. But since the in-between frames are non-existing, that value > isn't very useful. > > > 3) Done. > > Good. Done.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org, nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/1966273002/#ps100001 (title: "reset interval_next_frame_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1966273002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1966273002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/13310)
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1966273002/#ps120001 (title: "Remove unused variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1966273002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1966273002/120001
Message was sent while issue was closed.
Description was changed from ========== VideoAdapter: Add cropping based on OnOutputFormatRequest() If OnOutputFormatRequest() is called, VideoAdapter will crop to the same aspect ratio as the requested format. The output from VideoAdapter.AdaptFrameResolution() now contains both how to crop the input frame, and how to scale the cropped frame to the final adapted resolution. BUG=b/28622232 ========== to ========== VideoAdapter: Add cropping based on OnOutputFormatRequest() If OnOutputFormatRequest() is called, VideoAdapter will crop to the same aspect ratio as the requested format. The output from VideoAdapter.AdaptFrameResolution() now contains both how to crop the input frame, and how to scale the cropped frame to the final adapted resolution. BUG=b/28622232 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== VideoAdapter: Add cropping based on OnOutputFormatRequest() If OnOutputFormatRequest() is called, VideoAdapter will crop to the same aspect ratio as the requested format. The output from VideoAdapter.AdaptFrameResolution() now contains both how to crop the input frame, and how to scale the cropped frame to the final adapted resolution. BUG=b/28622232 ========== to ========== VideoAdapter: Add cropping based on OnOutputFormatRequest() If OnOutputFormatRequest() is called, VideoAdapter will crop to the same aspect ratio as the requested format. The output from VideoAdapter.AdaptFrameResolution() now contains both how to crop the input frame, and how to scale the cropped frame to the final adapted resolution. BUG=b/28622232 Committed: https://crrev.com/709f73c04eaa7aee66cafb1f5e7b28069f20d325 Cr-Commit-Position: refs/heads/master@{#12732} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/709f73c04eaa7aee66cafb1f5e7b28069f20d325 Cr-Commit-Position: refs/heads/master@{#12732} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
