Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1934503002:
Delete unused video capture code for cropping, non-square pixels, and ARGB. (Closed)
|
Created:
4 years, 7 months ago by nisse-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, perkj_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete unused video capture support for cropping, non-square pixels, and ARGB screencast scaling.
First two are unused, because the instance variables ratio_w_,
ratio_h_, and square_pixel_aspect_ratio_, are never modified after
initialization to 0 and false.
ARGB is believed to be unused, and the scaling logic
is probably not appropriate in any case.
Also delete corresponding helper functions in
videocommon.cc.
BUG=webrtc:5682
Committed: https://crrev.com/ba6371ec861b511db000a9487650dd49e344b866
Cr-Commit-Position: refs/heads/master@{#12659}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Delete unused ARGB scaling logic. #
Total comments: 1
Patch Set 3 : Delete test case VideoCapturerTest.ScreencastScaledSuperLarge. #
Messages
Total messages: 28 (12 generated)
nisse@webrtc.org changed reviewers: + pbos@webrtc.org
Next, I need to figure out what the VideoFrameFactory methods do, what can be deleted, and how to reorganize what's left to not depend on the cricket::VideoFrame::Stretch* methods.
pbos@webrtc.org changed reviewers: + perkj@webrtc.org
https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.cc:226: if (IsScreencast()) { Are we scaling screencast? This sounds weird/wrong? Can you look into whether this code actually scales or not? Do we ever get FOURCC_ARGB frames? perkj@: Is this code intended to trigger? https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.cc:266: int adapted_width = captured_frame->width; Can you put a good comment here? This looks like it's input width, e.g. not adapted. If the above case doesn't kick in then don't call it adapted_width/adapted_height.
https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.cc:226: if (IsScreencast()) { On 2016/04/29 15:48:32, pbos-webrtc wrote: > Are we scaling screencast? This sounds weird/wrong? Can you look into whether > this code actually scales or not? > > Do we ever get FOURCC_ARGB frames? perkj@: Is this code intended to trigger? No, we don't get ARGB but there has been talks about it for a long time when it comes to screen share. This is old code. Remove? https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.cc:270: video_adapter_.AdaptFrameResolution(adapted_width, adapted_height); captured_frame->width and captured_frame->height ?
https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.cc:226: if (IsScreencast()) { On 2016/05/02 15:29:19, perkj_webrtc wrote: > On 2016/04/29 15:48:32, pbos-webrtc wrote: > > Are we scaling screencast? This sounds weird/wrong? Can you look into whether > > this code actually scales or not? > > > > Do we ever get FOURCC_ARGB frames? perkj@: Is this code intended to trigger? > > No, we don't get ARGB but there has been talks about it for a long time when it > comes to screen share. This is old code. Remove? I think removing is right, the behavior doesn't sound right to me either.
https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.cc:266: int adapted_width = captured_frame->width; On 2016/04/29 15:48:32, pbos-webrtc wrote: > Can you put a good comment here? This looks like it's input width, e.g. not > adapted. If the above case doesn't kick in then don't call it > adapted_width/adapted_height. Is "output_width" or "target_width" better? The variables are intended to track the size after adaptation (and in the case of no adaptation, equal to the input size).
Description was changed from ========== Delete unused video capture support for cropping and non-square pixels. Believed to be correct, because the instance variables ratio_w_, ratio_h_, and square_pixel_aspect_ratio_, are never modified after initialization to 0 and false. BUG=webrtc:5682 ========== to ========== Delete unused video capture support for cropping, non-square pixels, and ARGB screencast scaling. First two are unused, because the instance variables ratio_w_, ratio_h_, and square_pixel_aspect_ratio_, are never modified after initialization to 0 and false. ARGB is believed to be unused, and the scaling logic is probably not appropriate in any case. BUG=webrtc:5682 ==========
Description was changed from ========== Delete unused video capture support for cropping, non-square pixels, and ARGB screencast scaling. First two are unused, because the instance variables ratio_w_, ratio_h_, and square_pixel_aspect_ratio_, are never modified after initialization to 0 and false. ARGB is believed to be unused, and the scaling logic is probably not appropriate in any case. BUG=webrtc:5682 ========== to ========== Delete unused video capture support for cropping, non-square pixels, and ARGB screencast scaling. First two are unused, because the instance variables ratio_w_, ratio_h_, and square_pixel_aspect_ratio_, are never modified after initialization to 0 and false. ARGB is believed to be unused, and the scaling logic is probably not appropriate in any case. Also delete corresponding helper functions in videocommon.cc. BUG=webrtc:5682 ==========
https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.cc:226: if (IsScreencast()) { On 2016/05/02 16:14:40, pbos-webrtc wrote: > On 2016/05/02 15:29:19, perkj_webrtc wrote: > > On 2016/04/29 15:48:32, pbos-webrtc wrote: > > > Are we scaling screencast? This sounds weird/wrong? Can you look into > whether > > > this code actually scales or not? > > > > > > Do we ever get FOURCC_ARGB frames? perkj@: Is this code intended to trigger? > > > > No, we don't get ARGB but there has been talks about it for a long time when > it > > comes to screen share. This is old code. Remove? > > I think removing is right, the behavior doesn't sound right to me either. Done. https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... webrtc/media/base/videocapturer.cc:266: int adapted_width = captured_frame->width; On 2016/04/29 15:48:32, pbos-webrtc wrote: > Can you put a good comment here? This looks like it's input width, e.g. not > adapted. If the above case doesn't kick in then don't call it > adapted_width/adapted_height. I'm inclined to leave this as is. Naming is consistent with |adapted_frame| further down. Do you have any concrete suggestion for changes or a clarifying comment?
lgtm, do you want to check this with Noah to be on the safe side? https://codereview.webrtc.org/1934503002/diff/20001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/1934503002/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:248: // TODO(fbarchard): LOG more information about captured frame attributes. maybe remove this TODO too.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934503002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934503002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/1782)
On 2016/05/04 10:04:34, nisse-webrtc wrote: > https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... > File webrtc/media/base/videocapturer.cc (right): > > https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... > webrtc/media/base/videocapturer.cc:226: if (IsScreencast()) { > On 2016/05/02 16:14:40, pbos-webrtc wrote: > > I think removing is right, the behavior doesn't sound right to me either. > > Done. This breaks the test case VideoCapturerTest.ScreencastScaledSuperLarge. So I'll have to delete that too (assuming you still agree the ARGB screen capturing is unused and currently unneeded).
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934503002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934503002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/04 11:31:55, nisse-webrtc wrote: > On 2016/05/04 10:04:34, nisse-webrtc wrote: > > > https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... > > File webrtc/media/base/videocapturer.cc (right): > > > > > https://codereview.webrtc.org/1934503002/diff/1/webrtc/media/base/videocaptur... > > webrtc/media/base/videocapturer.cc:226: if (IsScreencast()) { > > On 2016/05/02 16:14:40, pbos-webrtc wrote: > > > I think removing is right, the behavior doesn't sound right to me either. > > > > Done. > > This breaks the test case VideoCapturerTest.ScreencastScaledSuperLarge. > So I'll have to delete that too (assuming you still agree the ARGB screen > capturing is unused and currently unneeded). I'm fine with that, lgtm.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1934503002/#ps40001 (title: "Delete test case VideoCapturerTest.ScreencastScaledSuperLarge.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934503002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934503002/40001
Message was sent while issue was closed.
Description was changed from ========== Delete unused video capture support for cropping, non-square pixels, and ARGB screencast scaling. First two are unused, because the instance variables ratio_w_, ratio_h_, and square_pixel_aspect_ratio_, are never modified after initialization to 0 and false. ARGB is believed to be unused, and the scaling logic is probably not appropriate in any case. Also delete corresponding helper functions in videocommon.cc. BUG=webrtc:5682 ========== to ========== Delete unused video capture support for cropping, non-square pixels, and ARGB screencast scaling. First two are unused, because the instance variables ratio_w_, ratio_h_, and square_pixel_aspect_ratio_, are never modified after initialization to 0 and false. ARGB is believed to be unused, and the scaling logic is probably not appropriate in any case. Also delete corresponding helper functions in videocommon.cc. BUG=webrtc:5682 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Delete unused video capture support for cropping, non-square pixels, and ARGB screencast scaling. First two are unused, because the instance variables ratio_w_, ratio_h_, and square_pixel_aspect_ratio_, are never modified after initialization to 0 and false. ARGB is believed to be unused, and the scaling logic is probably not appropriate in any case. Also delete corresponding helper functions in videocommon.cc. BUG=webrtc:5682 ========== to ========== Delete unused video capture support for cropping, non-square pixels, and ARGB screencast scaling. First two are unused, because the instance variables ratio_w_, ratio_h_, and square_pixel_aspect_ratio_, are never modified after initialization to 0 and false. ARGB is believed to be unused, and the scaling logic is probably not appropriate in any case. Also delete corresponding helper functions in videocommon.cc. BUG=webrtc:5682 Committed: https://crrev.com/ba6371ec861b511db000a9487650dd49e344b866 Cr-Commit-Position: refs/heads/master@{#12659} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ba6371ec861b511db000a9487650dd49e344b866 Cr-Commit-Position: refs/heads/master@{#12659} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
