Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2595543002:
Refactor webrtc/modules/video_processing for GN check (Closed)
|
Created:
4 years ago by mbonadei Modified:
4 years ago Reviewers:
*kjellander_webrtc CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactor webrtc/modules/video_processing for GN check
This moves some GN check configurations out of .gn to individual
targets.
The now checked target is:
"//webrtc/modules/video_processing/*"
BUG=webrtc:6828
NOTRY=True
Review-Url: https://codereview.webrtc.org/2595543002
Cr-Commit-Position: refs/heads/master@{#15732}
Committed: https://chromium.googlesource.com/external/webrtc/+/00a810b844b2393b64f7e36ce1524dd9e3dccb4b
Patch Set 1 #
Total comments: 4
Patch Set 2 : Avoiding to refactor :video_processing_sse2 (cyclic dependency) #Patch Set 3 : Avoiding to refactor :video_processing_sse2 (cyclic dependency) #Messages
Total messages: 20 (9 generated)
mbonadei@webrtc.org changed reviewers: + kjellander@webrtc.org
mbonadei@webrtc.org changed required reviewers: + kjellander@webrtc.org
https://codereview.webrtc.org/2595543002/diff/1/webrtc/modules/video_processi... File webrtc/modules/video_processing/BUILD.gn (right): https://codereview.webrtc.org/2595543002/diff/1/webrtc/modules/video_processi... webrtc/modules/video_processing/BUILD.gn:51: "util/denoiser_filter.cc", This looks incorrect. These files are already listed under the "video_processing" target above. This target, IIUC, is only meant to contain the SEE2 implementation.
https://codereview.webrtc.org/2595543002/diff/1/webrtc/modules/video_processi... File webrtc/modules/video_processing/BUILD.gn (right): https://codereview.webrtc.org/2595543002/diff/1/webrtc/modules/video_processi... webrtc/modules/video_processing/BUILD.gn:51: "util/denoiser_filter.cc", On 2016/12/20 13:25:19, kjellander_webrtc wrote: > This looks incorrect. These files are already listed under the > "video_processing" target above. > This target, IIUC, is only meant to contain the SEE2 implementation. > Yes, that's correct. But `util/denoiser_filter_sse2.h` includes `util/denoiser_filter.h` and then `util/denoiser_filter.cc` includes `util/denoiser_filter_c.h`. The tools suggestion is an explixit `dep` to `video_processing` but this causes a circular dependency because `video_processing` depends on `video_processing_sse2` (line 36). This is basically a workaround to break the circular dependency but yes... It doesn't look good to me too. Am I missing something?
https://codereview.webrtc.org/2595543002/diff/1/webrtc/modules/video_processi... File webrtc/modules/video_processing/BUILD.gn (right): https://codereview.webrtc.org/2595543002/diff/1/webrtc/modules/video_processi... webrtc/modules/video_processing/BUILD.gn:51: "util/denoiser_filter.cc", On 2016/12/20 13:39:43, mbonadei wrote: > On 2016/12/20 13:25:19, kjellander_webrtc wrote: > > This looks incorrect. These files are already listed under the > > "video_processing" target above. > > This target, IIUC, is only meant to contain the SEE2 implementation. > > > > Yes, that's correct. But `util/denoiser_filter_sse2.h` includes > `util/denoiser_filter.h` and then `util/denoiser_filter.cc` includes > `util/denoiser_filter_c.h`. > > The tools suggestion is an explixit `dep` to `video_processing` but this causes > a circular dependency because `video_processing` depends on > `video_processing_sse2` (line 36). > > This is basically a workaround to break the circular dependency but yes... It > doesn't look good to me too. Am I missing something? I see. It might be better to let the code owners of this part deal with the refactoring to remove the circular dependency. But if you want to try, you could move the util/denoiser_filter* files into a new target 'video_processing_base' or similar, and make both 'video_processing' and 'video_processing_sse2' depend on that instead. That might solve the circular dependency. If not, I think it's out of the scope for our work to resolve such existing circular dependencies. Then just disable the include checks for this target with a comment like this (previous CLs by me and Edward did this for some of the targets): # TODO(mbonadei): Remove (bugs.webrtc.org/6828) # Errors on cyclic dependency with :video_processing if enabled. check_includes = false
https://codereview.webrtc.org/2595543002/diff/1/webrtc/modules/video_processi... File webrtc/modules/video_processing/BUILD.gn (right): https://codereview.webrtc.org/2595543002/diff/1/webrtc/modules/video_processi... webrtc/modules/video_processing/BUILD.gn:51: "util/denoiser_filter.cc", On 2016/12/20 14:00:21, kjellander_webrtc wrote: > On 2016/12/20 13:39:43, mbonadei wrote: > > On 2016/12/20 13:25:19, kjellander_webrtc wrote: > > > This looks incorrect. These files are already listed under the > > > "video_processing" target above. > > > This target, IIUC, is only meant to contain the SEE2 implementation. > > > > > > > Yes, that's correct. But `util/denoiser_filter_sse2.h` includes > > `util/denoiser_filter.h` and then `util/denoiser_filter.cc` includes > > `util/denoiser_filter_c.h`. > > > > The tools suggestion is an explixit `dep` to `video_processing` but this > causes > > a circular dependency because `video_processing` depends on > > `video_processing_sse2` (line 36). > > > > This is basically a workaround to break the circular dependency but yes... It > > doesn't look good to me too. Am I missing something? > > I see. It might be better to let the code owners of this part deal with the > refactoring to remove the circular dependency. > But if you want to try, you could move the util/denoiser_filter* files into a > new target 'video_processing_base' or similar, and make both 'video_processing' > and 'video_processing_sse2' depend on that instead. That might solve the > circular dependency. > If not, I think it's out of the scope for our work to resolve such existing > circular dependencies. Then just disable the include checks for this target with > a comment like this (previous CLs by me and Edward did this for some of the > targets): > > # TODO(mbonadei): Remove (bugs.webrtc.org/6828) > # Errors on cyclic dependency with :video_processing if enabled. > check_includes = false Yes, unfortunately it is not possible to remove it without a refactoring of the code. `denoiser_filter.cc` depends on `denoiser_filter_sse2` in the `DenoiserFilter::Create` method. Thanks for the wise advice.
Description was changed from ========== Refactor webrtc/modules/video_processing for GN check This moves some GN check configurations out of .gn to individual targets. The now checked target is: "//webrtc/modules/video_processing/*" BUG=6828 ========== to ========== Refactor webrtc/modules/video_processing for GN check This moves some GN check configurations out of .gn to individual targets. The now checked target is: "//webrtc/modules/video_processing/*" BUG=webrtc:6828 NOTRY=True ==========
lgtm CCing stefan@webrtc.org since it would be nice to get this cleaned up.
The CQ bit was checked by mbonadei@webrtc.org
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
Failed to apply patch for .gn: While running git apply --index -p1; error: patch failed: .gn:30 error: .gn: patch does not apply Patch: .gn Index: .gn diff --git a/.gn b/.gn index a0fd50aeada9811738c767ae2275a7ad430c5b48..d85c6cbae6a42a7fb78c6d101ed9d7f7c25cf424 100644 --- a/.gn +++ b/.gn @@ -30,6 +30,7 @@ check_targets = [ "//webrtc/modules/audio_processing/*", "//webrtc/modules/video_capture/*", "//webrtc/modules/video_coding/*", + "//webrtc/modules/video_processing/*", "//webrtc/stats:rtc_stats", "//webrtc/voice_engine", "//webrtc/voice_engine:level_indicator",
The CQ bit was checked by mbonadei@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2595543002/#ps40001 (title: "Avoiding to refactor :video_processing_sse2 (cyclic dependency)")
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": 40001, "attempt_start_ts": 1482314281714050,
"parent_rev": "dbb64d8f27e8aaee4e37b6e16503bb5f7406aeed", "commit_rev":
"00a810b844b2393b64f7e36ce1524dd9e3dccb4b"}
Message was sent while issue was closed.
Description was changed from ========== Refactor webrtc/modules/video_processing for GN check This moves some GN check configurations out of .gn to individual targets. The now checked target is: "//webrtc/modules/video_processing/*" BUG=webrtc:6828 NOTRY=True ========== to ========== Refactor webrtc/modules/video_processing for GN check This moves some GN check configurations out of .gn to individual targets. The now checked target is: "//webrtc/modules/video_processing/*" BUG=webrtc:6828 NOTRY=True Review-Url: https://codereview.webrtc.org/2595543002 Cr-Commit-Position: refs/heads/master@{#15732} Committed: https://chromium.googlesource.com/external/webrtc/+/00a810b844b2393b64f7e36ce... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/00a810b844b2393b64f7e36ce...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2594973002/ by mbonadei@webrtc.org. The reason for reverting is: This CL broke some buildbots. I will investigate it later.. |
