Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562513003:
Add opus dep to media pipeline_integration_test. (Closed)
DescriptionAdd opus dep to media pipeline_integration_test.
arm devices require the OPUS_FIXED_POINT. This define got lost at some
point. Add a public_dep so the media_unittests gets the necessary
config.
BUG=672352
TEST=gn desc out/ //media:media_unittests
Committed: https://crrev.com/e9bc6779fab8a404ae2a01df4dd3c45ffd3a408e
Cr-Commit-Position: refs/heads/master@{#437628}
Patch Set 1 #
Total comments: 1
Patch Set 2 : change public_deps to plain deps. #
Total comments: 2
Patch Set 3 : Remove unnessary dep in media_unittests #Messages
Total messages: 25 (8 generated)
mbjorge@chromium.org changed reviewers: + watk@chromium.org, xhwang@chromium.org
https://codereview.chromium.org/2562513003/diff/1/media/test/BUILD.gn File media/test/BUILD.gn (right): https://codereview.chromium.org/2562513003/diff/1/media/test/BUILD.gn#newcode66 media/test/BUILD.gn:66: "//third_party/opus", OOC, why this needs to be a public_deps, will a plain deps work?
On 2016/12/08 at 05:50:27, xhwang wrote: > https://codereview.chromium.org/2562513003/diff/1/media/test/BUILD.gn > File media/test/BUILD.gn (right): > > https://codereview.chromium.org/2562513003/diff/1/media/test/BUILD.gn#newcode66 > media/test/BUILD.gn:66: "//third_party/opus", > OOC, why this needs to be a public_deps, will a plain deps work? I need the opus_config to propagate to the media_unittests target that depends on the //media/test:pipeline_integration_tests target. I tested out just using a plain dep in pipeline_integration_tests, but then OPUS_FIXED_POINT is not defined for the media_unittests. I also considered adding the plain dep to both targets (pipeline_integration_tests and media_unittests), but I think it's cleaner and makes more sense to just have the one dep on pipeline_intergration_tests and make it public, since that's really where the dependency is.
On 2016/12/08 19:25:02, mbjorge wrote: > On 2016/12/08 at 05:50:27, xhwang wrote: > > https://codereview.chromium.org/2562513003/diff/1/media/test/BUILD.gn > > File media/test/BUILD.gn (right): > > > > > https://codereview.chromium.org/2562513003/diff/1/media/test/BUILD.gn#newcode66 > > media/test/BUILD.gn:66: "//third_party/opus", > > OOC, why this needs to be a public_deps, will a plain deps work? > > I need the opus_config to propagate to the media_unittests target that depends > on the //media/test:pipeline_integration_tests target. I tested out just using a > plain dep in pipeline_integration_tests, but then OPUS_FIXED_POINT is not > defined for the media_unittests. I also considered adding the plain dep to both > targets (pipeline_integration_tests and media_unittests), but I think it's > cleaner and makes more sense to just have the one dep on > pipeline_intergration_tests and make it public, since that's really where the > dependency is. Thanks! Could you please point to me where in media_unittests we need OPUS_FIXED_POINT? In other words, if I decide to exclude pipeline_integration_tests from media_unittests, will any other test be broken?
On 2016/12/08 at 19:48:44, xhwang wrote: > On 2016/12/08 19:25:02, mbjorge wrote: > > On 2016/12/08 at 05:50:27, xhwang wrote: > > > https://codereview.chromium.org/2562513003/diff/1/media/test/BUILD.gn > > > File media/test/BUILD.gn (right): > > > > > > > > https://codereview.chromium.org/2562513003/diff/1/media/test/BUILD.gn#newcode66 > > > media/test/BUILD.gn:66: "//third_party/opus", > > > OOC, why this needs to be a public_deps, will a plain deps work? > > > > I need the opus_config to propagate to the media_unittests target that depends > > on the //media/test:pipeline_integration_tests target. I tested out just using a > > plain dep in pipeline_integration_tests, but then OPUS_FIXED_POINT is not > > defined for the media_unittests. I also considered adding the plain dep to both > > targets (pipeline_integration_tests and media_unittests), but I think it's > > cleaner and makes more sense to just have the one dep on > > pipeline_intergration_tests and make it public, since that's really where the > > dependency is. > > Thanks! Could you please point to me where in media_unittests we need OPUS_FIXED_POINT? In other words, if I decide to exclude pipeline_integration_tests from media_unittests, will any other test be broken? It looks like media/filters/audio_decoder_unittest.cc (https://cs.chromium.org/chromium/src/media/filters/audio_decoder_unittest.cc?...) is the only other target that uses OPUS_FIXED_POINT, and it's part of the //media:unit_tests target (https://cs.chromium.org/chromium/src/media/BUILD.gn?l=653). (Based on https://cs.chromium.org/search/?q=OPUS_FIXED_POINT+file:%5Esrc/media/&sq=pack...) Do you want me to add the same public_dep next to the filters/audio_decoder_unittest.cc?
Please add deps for whichever target is using OPUS_FIXED_POINT and do not use public_deps.
On 2016/12/08 at 20:31:02, xhwang wrote: > Please add deps for whichever target is using OPUS_FIXED_POINT and do not use public_deps. Ok, I removed the public_deps and just used plain deps everywhere
https://codereview.chromium.org/2562513003/diff/20001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2562513003/diff/20001/media/BUILD.gn#newcode748 media/BUILD.gn:748: "//third_party/opus", Why do we need this (from which test)?
https://codereview.chromium.org/2562513003/diff/20001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2562513003/diff/20001/media/BUILD.gn#newcode748 media/BUILD.gn:748: "//third_party/opus", On 2016/12/08 at 21:35:33, xhwang wrote: > Why do we need this (from which test)? Oh, d'oh. I was confused on how the configs were working. Not needed. Thanks
LGTM, thanks for fixing this!
The CQ bit was checked by mbjorge@chromium.org
The CQ bit was checked by mbjorge@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by mbjorge@chromium.org
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": 40001, "attempt_start_ts": 1481305935246510,
"parent_rev": "f6dd7a85222f60d6b3f298a9597a5d57835b5e02", "commit_rev":
"1c4e7d16c469f947c40a0880a1b95256880154d9"}
Message was sent while issue was closed.
Description was changed from ========== Add opus dep to media pipeline_integration_test. arm devices require the OPUS_FIXED_POINT. This define got lost at some point. Add a public_dep so the media_unittests gets the necessary config. BUG=672352 TEST=gn desc out/ //media:media_unittests ========== to ========== Add opus dep to media pipeline_integration_test. arm devices require the OPUS_FIXED_POINT. This define got lost at some point. Add a public_dep so the media_unittests gets the necessary config. BUG=672352 TEST=gn desc out/ //media:media_unittests Review-Url: https://codereview.chromium.org/2562513003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add opus dep to media pipeline_integration_test. arm devices require the OPUS_FIXED_POINT. This define got lost at some point. Add a public_dep so the media_unittests gets the necessary config. BUG=672352 TEST=gn desc out/ //media:media_unittests Review-Url: https://codereview.chromium.org/2562513003 ========== to ========== Add opus dep to media pipeline_integration_test. arm devices require the OPUS_FIXED_POINT. This define got lost at some point. Add a public_dep so the media_unittests gets the necessary config. BUG=672352 TEST=gn desc out/ //media:media_unittests Committed: https://crrev.com/e9bc6779fab8a404ae2a01df4dd3c45ffd3a408e Cr-Commit-Position: refs/heads/master@{#437628} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e9bc6779fab8a404ae2a01df4dd3c45ffd3a408e Cr-Commit-Position: refs/heads/master@{#437628} |
