Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3011043002:
Add VideoProcessorIntegrationTest for MediaCodec implementations. (Closed)
|
Created:
3 years, 3 months ago by brandtr Modified:
3 years, 3 months ago 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, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd VideoProcessorIntegrationTest for MediaCodec implementations.
* Add VideoProcessorIntegrationTestMediaCodec tests, including
a standard foreman test and a forced SW fallback test.
* Morph PlotVideoProcessorIntegrationTest into
VideoProcessorIntegrationTestParameterized. This test is intended
to be patched locally, depending on what metric is of interest.
It is run on the bots to ensure that it doesn't break.
* Remove the plot_videoprocessor_integrationtest binary. The test
above is instead moved to the modules_tests binary.
BUG=webrtc:6634
Review-Url: https://codereview.webrtc.org/3011043002
Cr-Commit-Position: refs/heads/master@{#19727}
Committed: https://chromium.googlesource.com/external/webrtc/+/df232992597bd2865fdbede3e9d7e45445e4f122
Patch Set 1 #Patch Set 2 : Loosen thresholds. #
Total comments: 12
Patch Set 3 : mbonadei comments 1. #Patch Set 4 : asapersson comments 1. #Dependent Patchsets: Messages
Total messages: 47 (30 generated)
The CQ bit was checked by brandtr@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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/27568)
Threshold tweaks.
The CQ bit was checked by brandtr@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/...
Threshold tweaks.
The CQ bit was checked by brandtr@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/...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Threshold tweaks.
The CQ bit was checked by brandtr@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.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from
==========
Add VideoProcessorIntegrationTest for MediaCodec implementations.
* Add VideoProcessorIntegrationTestMediaCodec that is only enabled on
Android platforms.
- First set of tests include a standard foreman tests, and a forced
SW fallback test.
* Morph PlotVideoProcessorIntegrationTest into
VideoProcessorIntegrationTestParameterized.
- This test is intended to be patched locally, depending on what metric
is of interest. It is run on the bots to ensure that it doesn't break.
* Remove the plot_videoprocessor_integrationtest binary. The test above is moved
to the modules_tests binary.
BUG=webrtc:6634
==========
to
==========
Add VideoProcessorIntegrationTest for MediaCodec implementations.
* Add VideoProcessorIntegrationTestMediaCodec that is only enabled on
Android platforms. First set of tests include a standard foreman test
and a forced SW fallback test.
* Morph PlotVideoProcessorIntegrationTest into
VideoProcessorIntegrationTestParameterized. This test is intended to
be patched locally, depending on what metric is of interest. It is
run on the bots to ensure that it doesn't break.
* Remove the plot_videoprocessor_integrationtest binary. The test above is moved
to the modules_tests binary.
BUG=webrtc:6634
==========
Loosen thresholds.
Description was changed from ========== Add VideoProcessorIntegrationTest for MediaCodec implementations. * Add VideoProcessorIntegrationTestMediaCodec that is only enabled on Android platforms. First set of tests include a standard foreman test and a forced SW fallback test. * Morph PlotVideoProcessorIntegrationTest into VideoProcessorIntegrationTestParameterized. This test is intended to be patched locally, depending on what metric is of interest. It is run on the bots to ensure that it doesn't break. * Remove the plot_videoprocessor_integrationtest binary. The test above is moved to the modules_tests binary. BUG=webrtc:6634 ========== to ========== Add VideoProcessorIntegrationTest for MediaCodec implementations. * Add VideoProcessorIntegrationTestMediaCodec tests, including a standard foreman test and a forced SW fallback test. * Morph PlotVideoProcessorIntegrationTest into VideoProcessorIntegrationTestParameterized. This test is intended to be patched locally, depending on what metric is of interest. It is run on the bots to ensure that it doesn't break. * Remove the plot_videoprocessor_integrationtest binary. The test above is instead moved to the modules_tests binary. BUG=webrtc:6634 ==========
brandtr@webrtc.org changed reviewers: + asapersson@webrtc.org, mbonadei@webrtc.org
Please take a look. asapersson: General review. mbonadei: BUILD.gn changes.
mbonadei@webrtc.org changed reviewers: + kjellander@webrtc.org
lgtm, but I've also added kjellander@ because I would like to understand if there is something we can do for the Java dependency. https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn#... webrtc/modules/BUILD.gn:84: "../sdk/android:libjingle_peerconnection_java", Henrik: do you know something about this? https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/BUILD.gn (right): https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/BUILD.gn:419: "../../system_wrappers:system_wrappers", This should be "../../system_wrappers" since it will implicitly pick "../../system_wrappers:system_wrappers" (the same for "../../common_video:common_video", it should be "../../common_video").
I think you can do what you initially wanted, see my https://codereview.webrtc.org/3005233002/ https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn#... webrtc/modules/BUILD.gn:84: "../sdk/android:libjingle_peerconnection_java", On 2017/09/07 07:57:25, mbonadei wrote: > Henrik: do you know something about this? No, I'm not familiar with this and I don't see why it wouldn't be possible. I created https://codereview.webrtc.org/3005233002/ and it seems it is possible to add it where you initially wanted. Maybe you had some other error when you tried it?
lgtm https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/plot_webrtc_test_logs.py (right): https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/plot_webrtc_test_logs.py:23: EVENT_END = 'OK ] CodecSettings/VideoProcessorIntegrationTestParameterized.' VideoProcessorIntegrationParameterized in videoprocessor_integrationtest_parameterized.cc https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc (right): https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc:43: ForemanCif500kbpsMediaCodecVp8) { maybe ForemanCif500kbpsVp8 https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc:44: config_.input_filename = ResourcePath(config_.filename, "yuv"); remove line? https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc:66: Foreman240p100kbpsMediaCodecVp8WithForcedSwFallback) { same here
On 2017/09/07 09:57:31, kjellander_webrtc wrote: > I think you can do what you initially wanted, see my > https://codereview.webrtc.org/3005233002/ > > https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn > File webrtc/modules/BUILD.gn (right): > > https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn#... > webrtc/modules/BUILD.gn:84: "../sdk/android:libjingle_peerconnection_java", > On 2017/09/07 07:57:25, mbonadei wrote: > > Henrik: do you know something about this? > > No, I'm not familiar with this and I don't see why it wouldn't be possible. > I created https://codereview.webrtc.org/3005233002/ and it seems it is possible > to add it where you initially wanted. Maybe you had some other error when you > tried it? It builds, but for some reason the Java class files are not included in the apk so you get a runtime error here: https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/c... Compare the output of the non-compile bots on https://codereview.webrtc.org/3005233002/#ps20001, vs the output of the non-compile bots in this CL. I have no idea why this is the case, but adding the _java target to the rtc_test instead of the rtc_source_set seems to solve the problem...
kjellander@webrtc.org changed reviewers: + sakal@webrtc.org
On 2017/09/07 12:45:50, brandtr wrote: > On 2017/09/07 09:57:31, kjellander_webrtc wrote: > > I think you can do what you initially wanted, see my > > https://codereview.webrtc.org/3005233002/ > > > > https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn > > File webrtc/modules/BUILD.gn (right): > > > > > https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn#... > > webrtc/modules/BUILD.gn:84: "../sdk/android:libjingle_peerconnection_java", > > On 2017/09/07 07:57:25, mbonadei wrote: > > > Henrik: do you know something about this? > > > > No, I'm not familiar with this and I don't see why it wouldn't be possible. > > I created https://codereview.webrtc.org/3005233002/ and it seems it is > possible > > to add it where you initially wanted. Maybe you had some other error when you > > tried it? > > It builds, but for some reason the Java class files are not included in the apk > so you get a runtime error here: > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/c... > > Compare the output of the non-compile bots on > https://codereview.webrtc.org/3005233002/#ps20001, vs the output of the > non-compile bots in this CL. > > I have no idea why this is the case, but adding the _java target to the rtc_test > instead of the rtc_source_set seems to solve the problem... Hmm, this reminds me a lot about similar issues we faced in https://codereview.webrtc.org/2646093004 Mirko/Sami: do you remember how this worked? lgtm if you want to just get this in now and worry about the technical debt later.
On 2017/09/07 12:54:54, kjellander_webrtc wrote: > On 2017/09/07 12:45:50, brandtr wrote: > > On 2017/09/07 09:57:31, kjellander_webrtc wrote: > > > I think you can do what you initially wanted, see my > > > https://codereview.webrtc.org/3005233002/ > > > > > > https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn > > > File webrtc/modules/BUILD.gn (right): > > > > > > > > > https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn#... > > > webrtc/modules/BUILD.gn:84: "../sdk/android:libjingle_peerconnection_java", > > > On 2017/09/07 07:57:25, mbonadei wrote: > > > > Henrik: do you know something about this? > > > > > > No, I'm not familiar with this and I don't see why it wouldn't be possible. > > > I created https://codereview.webrtc.org/3005233002/ and it seems it is > > possible > > > to add it where you initially wanted. Maybe you had some other error when > you > > > tried it? > > > > It builds, but for some reason the Java class files are not included in the > apk > > so you get a runtime error here: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/c... > > > > Compare the output of the non-compile bots on > > https://codereview.webrtc.org/3005233002/#ps20001, vs the output of the > > non-compile bots in this CL. > > > > I have no idea why this is the case, but adding the _java target to the > rtc_test > > instead of the rtc_source_set seems to solve the problem... > > Hmm, this reminds me a lot about similar issues we faced in > https://codereview.webrtc.org/2646093004 > Mirko/Sami: do you remember how this worked? > > lgtm if you want to just get this in now and worry about the technical debt > later. I already had this problem in https://codereview.webrtc.org/2695653002, so this CL is just moving the problem around :)
On 2017/09/07 13:03:03, brandtr wrote: > On 2017/09/07 12:54:54, kjellander_webrtc wrote: > > On 2017/09/07 12:45:50, brandtr wrote: > > > On 2017/09/07 09:57:31, kjellander_webrtc wrote: > > > > I think you can do what you initially wanted, see my > > > > https://codereview.webrtc.org/3005233002/ > > > > > > > > > https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn > > > > File webrtc/modules/BUILD.gn (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/BUILD.gn#... > > > > webrtc/modules/BUILD.gn:84: > "../sdk/android:libjingle_peerconnection_java", > > > > On 2017/09/07 07:57:25, mbonadei wrote: > > > > > Henrik: do you know something about this? > > > > > > > > No, I'm not familiar with this and I don't see why it wouldn't be > possible. > > > > I created https://codereview.webrtc.org/3005233002/ and it seems it is > > > possible > > > > to add it where you initially wanted. Maybe you had some other error when > > you > > > > tried it? > > > > > > It builds, but for some reason the Java class files are not included in the > > apk > > > so you get a runtime error here: > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/c... > > > > > > Compare the output of the non-compile bots on > > > https://codereview.webrtc.org/3005233002/#ps20001, vs the output of the > > > non-compile bots in this CL. > > > > > > I have no idea why this is the case, but adding the _java target to the > > rtc_test > > > instead of the rtc_source_set seems to solve the problem... > > > > Hmm, this reminds me a lot about similar issues we faced in > > https://codereview.webrtc.org/2646093004 > > Mirko/Sami: do you remember how this worked? > > > > lgtm if you want to just get this in now and worry about the technical debt > > later. > > I already had this problem in https://codereview.webrtc.org/2695653002, so this > CL is just moving the problem around :) Btw, I will soonish try to add Android HW codec support to the webrtc_perf_tests target too. I guess I have to use this ugly workaround there as well, unless we figure out a better way to solve the problem.
> > > Hmm, this reminds me a lot about similar issues we faced in > > > https://codereview.webrtc.org/2646093004 > > > Mirko/Sami: do you remember how this worked? > > > > > > lgtm if you want to just get this in now and worry about the technical debt > > > later. > > > > I already had this problem in https://codereview.webrtc.org/2695653002, so > this > > CL is just moving the problem around :) > > Btw, I will soonish try to add Android HW codec support to the webrtc_perf_tests > target too. I guess I have to use this ugly workaround there as well, unless we > figure out a better way to solve the problem. Good finding Henrik, I will look into https://codereview.webrtc.org/2646093004 later (maybe tomorrow morning). brandtr@ feel free to land, this is not blocking. I will update you if I will find something.
After a quick look I spotted a difference between rtc_test and rtc_source_set templates. In the rtc_test GN template we are implicitly adding android_manifest and we are adding a dependency on "test:native_test_java" (https://cs.chromium.org/chromium/src/third_party/webrtc/webrtc.gni?q=webrtc/w...) while are not doing anything in the rtc_source_set template (https://cs.chromium.org/chromium/src/third_party/webrtc/webrtc.gni?q=webrtc/w...). I will follow up.
Addressed comments. https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/BUILD.gn (right): https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/BUILD.gn:419: "../../system_wrappers:system_wrappers", On 2017/09/07 07:57:25, mbonadei wrote: > This should be "../../system_wrappers" since it will implicitly pick > "../../system_wrappers:system_wrappers" (the same for > "../../common_video:common_video", it should be "../../common_video"). > Done. https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/plot_webrtc_test_logs.py (right): https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/plot_webrtc_test_logs.py:23: EVENT_END = 'OK ] CodecSettings/VideoProcessorIntegrationTestParameterized.' On 2017/09/07 10:41:58, åsapersson wrote: > VideoProcessorIntegrationParameterized in > videoprocessor_integrationtest_parameterized.cc Thanks! Fixed in .cc. https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc (right): https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc:43: ForemanCif500kbpsMediaCodecVp8) { On 2017/09/07 10:41:58, åsapersson wrote: > maybe ForemanCif500kbpsVp8 Done. https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc:44: config_.input_filename = ResourcePath(config_.filename, "yuv"); On 2017/09/07 10:41:58, åsapersson wrote: > remove line? Done. https://codereview.webrtc.org/3011043002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest_mediacodec.cc:66: Foreman240p100kbpsMediaCodecVp8WithForcedSwFallback) { On 2017/09/07 10:41:58, åsapersson wrote: > same here Done.
The CQ bit was checked by brandtr@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.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from mbonadei@webrtc.org, asapersson@webrtc.org Link to the patchset: https://codereview.webrtc.org/3011043002/#ps160001 (title: "asapersson comments 1.")
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": 160001, "attempt_start_ts": 1504795658471600,
"parent_rev": "310e32b92a588ed31dad63c3b94a19cd025190f4", "commit_rev":
"df232992597bd2865fdbede3e9d7e45445e4f122"}
Message was sent while issue was closed.
Description was changed from ========== Add VideoProcessorIntegrationTest for MediaCodec implementations. * Add VideoProcessorIntegrationTestMediaCodec tests, including a standard foreman test and a forced SW fallback test. * Morph PlotVideoProcessorIntegrationTest into VideoProcessorIntegrationTestParameterized. This test is intended to be patched locally, depending on what metric is of interest. It is run on the bots to ensure that it doesn't break. * Remove the plot_videoprocessor_integrationtest binary. The test above is instead moved to the modules_tests binary. BUG=webrtc:6634 ========== to ========== Add VideoProcessorIntegrationTest for MediaCodec implementations. * Add VideoProcessorIntegrationTestMediaCodec tests, including a standard foreman test and a forced SW fallback test. * Morph PlotVideoProcessorIntegrationTest into VideoProcessorIntegrationTestParameterized. This test is intended to be patched locally, depending on what metric is of interest. It is run on the bots to ensure that it doesn't break. * Remove the plot_videoprocessor_integrationtest binary. The test above is instead moved to the modules_tests binary. BUG=webrtc:6634 Review-Url: https://codereview.webrtc.org/3011043002 Cr-Commit-Position: refs/heads/master@{#19727} Committed: https://chromium.googlesource.com/external/webrtc/+/df232992597bd2865fdbede3e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/df232992597bd2865fdbede3e... |
