Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2314403007:
Reland of "Separating video settings in VideoQualityTest". (Closed)
|
Created:
4 years, 3 months ago by minyue-webrtc Modified:
4 years, 1 month ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, perkj_webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReland of "Separating video settings in VideoQualityTest".
Earlier trial of landing: https://codereview.webrtc.org/2312613003
Reverted in https://codereview.webrtc.org/2325723002
BUG=webrtc:6609
Committed: https://crrev.com/16b6d6dc5b367746a9f910d1cebf9f65e8dd2c7f
Cr-Commit-Position: refs/heads/master@{#14785}
Patch Set 1 #Patch Set 2 : Adding initialization. #
Total comments: 4
Patch Set 3 : rebasing #Patch Set 4 : fixing #
Messages
Total messages: 28 (15 generated)
Description was changed from ========== on Stefan's comments refactoring video_quality_test BUG= ========== to ========== Reland of "Separating video settings in VideoQualityTest". BUG= ==========
minyue@webrtc.org changed reviewers: + stefan@webrtc.org
Description was changed from ========== on Stefan's comments refactoring video_quality_test BUG= ========== to ========== Reland of "Separating video settings in VideoQualityTest". BUG= ==========
minyue@webrtc.org changed reviewers: + stefan@webrtc.org
Hi Stefan, Relanding an earlier CL. The revert was because some fields in Params were not properly initialized. PS1 is the same as the earlier commit. PS2 fixes the problem.
Description was changed from ========== Reland of "Separating video settings in VideoQualityTest". BUG= ========== to ========== Reland of "Separating video settings in VideoQualityTest". Earlier trial of landing: https://codereview.webrtc.org/2312613003 Reverted in https://codereview.webrtc.org/2325723002 BUG= ==========
https://codereview.webrtc.org/2314403007/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2314403007/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:756: ss({}) {} Is this the right way to do this? It's not clear to me if these are reasonable default values. What field was read uninitialized by the test and what would have been a reasonable value for it?
https://codereview.webrtc.org/2314403007/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2314403007/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:756: ss({}) {} On 2016/09/09 11:42:09, stefan-webrtc (holmer) wrote: > Is this the right way to do this? It's not clear to me if these are reasonable > default values. What field was read uninitialized by the test and what would > have been a reasonable value for it? This means that all fields will be filled with 0s. This will have the same behavior of the old initialization of Param, which is params = {x, x, x, x} where {x, x , x, x} are incomplete. We can make it look better by filling "{}" with default values (0s), but I think the whole Params is subject to a re-structuring. That may be taken as a separate CL.
https://codereview.webrtc.org/2314403007/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2314403007/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:756: ss({}) {} On 2016/09/09 14:01:32, minyue-webrtc wrote: > On 2016/09/09 11:42:09, stefan-webrtc (holmer) wrote: > > Is this the right way to do this? It's not clear to me if these are reasonable > > default values. What field was read uninitialized by the test and what would > > have been a reasonable value for it? > > This means that all fields will be filled with 0s. This will have the same > behavior of the old initialization of Param, which is > params = {x, x, x, x} > where > {x, x , x, x} are incomplete. > > We can make it look better by filling "{}" with default values (0s), but I think > the whole Params is subject to a re-structuring. That may be taken as a > separate CL. But which parameter was used without being initialized that caused the revert? It's not clear to me that zero is the right value for it. Shouldn't that test instead set it correctly?
The CQ bit was checked by minyue@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/...
Hi Stefan, I started the work on audio-only mode to video loopback test. This is the pre-work. https://codereview.webrtc.org/2314403007/diff/20001/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2314403007/diff/20001/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:756: ss({}) {} On 2016/09/09 14:18:05, stefan-webrtc (holmer) wrote: > On 2016/09/09 14:01:32, minyue-webrtc wrote: > > On 2016/09/09 11:42:09, stefan-webrtc (holmer) wrote: > > > Is this the right way to do this? It's not clear to me if these are > reasonable > > > default values. What field was read uninitialized by the test and what would > > > have been a reasonable value for it? > > > > This means that all fields will be filled with 0s. This will have the same > > behavior of the old initialization of Param, which is > > params = {x, x, x, x} > > where > > {x, x , x, x} are incomplete. > > > > We can make it look better by filling "{}" with default values (0s), but I > think > > the whole Params is subject to a re-structuring. That may be taken as a > > separate CL. > > But which parameter was used without being initialized that caused the revert? > It's not clear to me that zero is the right value for it. Shouldn't that test > instead set it correctly? Fixing at the tests meaning adding many codes, yet, it is not really nice, since many of these are not initialized anyway, even though we may not use many of them. I now initialize them with meaningful values. How do you like the solution?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Hi Stefan, Kindly ping for review.
lgtm
The CQ bit was checked by minyue@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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9559)
Description was changed from ========== Reland of "Separating video settings in VideoQualityTest". Earlier trial of landing: https://codereview.webrtc.org/2312613003 Reverted in https://codereview.webrtc.org/2325723002 BUG= ========== to ========== Reland of "Separating video settings in VideoQualityTest". Earlier trial of landing: https://codereview.webrtc.org/2312613003 Reverted in https://codereview.webrtc.org/2325723002 BUG=webrtc:6609 ==========
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Reland of "Separating video settings in VideoQualityTest". Earlier trial of landing: https://codereview.webrtc.org/2312613003 Reverted in https://codereview.webrtc.org/2325723002 BUG=webrtc:6609 ========== to ========== Reland of "Separating video settings in VideoQualityTest". Earlier trial of landing: https://codereview.webrtc.org/2312613003 Reverted in https://codereview.webrtc.org/2325723002 BUG=webrtc:6609 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Reland of "Separating video settings in VideoQualityTest". Earlier trial of landing: https://codereview.webrtc.org/2312613003 Reverted in https://codereview.webrtc.org/2325723002 BUG=webrtc:6609 ========== to ========== Reland of "Separating video settings in VideoQualityTest". Earlier trial of landing: https://codereview.webrtc.org/2312613003 Reverted in https://codereview.webrtc.org/2325723002 BUG=webrtc:6609 Committed: https://crrev.com/16b6d6dc5b367746a9f910d1cebf9f65e8dd2c7f Cr-Commit-Position: refs/heads/master@{#14785} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/16b6d6dc5b367746a9f910d1cebf9f65e8dd2c7f Cr-Commit-Position: refs/heads/master@{#14785}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/2463733002/ by minyue@webrtc.org. The reason for reverting is: Some parameters were not treated correctly. Will redo some parts.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
