VideoProcessorIntegrationTest: Group member variables into two structs containing target/actual rate

VideoProcessorIntegrationTest: Group member variables into two structs containing target/actual rates.

- Group member variables into two structs: target rates/actual rates.
- Split verify and print of rate control metrics into separate functions.
- Rename member variables.

BUG=webrtc:6634
Review-Url: https://codereview.webrtc.org/3009423002
Cr-Commit-Position: refs/heads/master@{#19925}
Committed: https://webrtc.googlesource.com/src/+/55c7eded9457bd918927d91080290ea9f6fc6f86

[åsapersson, 2017-09-13 15:05:04]

Patchset #5 (id:80001) has been deleted

[åsapersson, 2017-09-14 09:18:20]

Patchset #7 (id:140001) has been deleted

[åsapersson, 2017-09-14 11:46:25]

Patchset #7 (id:160001) has been deleted

[åsapersson, 2017-09-14 12:03:14]

Patchset #7 (id:180001) has been deleted

[åsapersson, 2017-09-15 10:23:06]

Patchset #10 (id:260001) has been deleted

[åsapersson, 2017-09-15 10:23:51]

Patchset #9 (id:240001) has been deleted

[åsapersson, 2017-09-15 10:36:04]

Patchset #10 (id:300001) has been deleted

[åsapersson, 2017-09-15 10:36:12]

Patchset #9 (id:280001) has been deleted

[åsapersson, 2017-09-15 10:41:22]

Patchset #10 (id:340001) has been deleted

[åsapersson, 2017-09-15 10:45:15]

Patchset #10 (id:360001) has been deleted

[åsapersson, 2017-09-15 10:49:33]

Description was changed from

==========
VideoProcessorIntegrationTest..

BUG=
==========

to

==========
VideoProcessorIntegrationTest: Group member variables into two structs
containing target/actual rates.

- Group member variables into two structs: target rates/actual rates.
- Split verify and print of rate control metrics into separate functions.
- Rename member variables.

BUG=webrtc:6634
==========

[åsapersson, 2017-09-15 10:50:01]

Patchset #8 (id:220001) has been deleted

[åsapersson, 2017-09-15 10:50:23]

Patchset #7 (id:200001) has been deleted

[åsapersson, 2017-09-15 10:50:42]

Patchset #7 (id:320001) has been deleted

[åsapersson, 2017-09-15 10:51:36]

Patchset #2 (id:20001) has been deleted

[åsapersson, 2017-09-15 10:51:56]

Patchset #3 (id:60001) has been deleted

[åsapersson, 2017-09-15 10:52:25]

Patchset #2 (id:40001) has been deleted

[åsapersson, 2017-09-15 10:54:04]

Patchset #2 (id:100001) has been deleted

[åsapersson, 2017-09-15 10:54:41]

Patchset #2 (id:120001) has been deleted

[åsapersson, 2017-09-15 11:29:38]

asapersson@webrtc.org changed reviewers:
+ brandtr@webrtc.org

[brandtr, 2017-09-21 08:40:10]

Good improvement! lgtm

I added some small comments and some ideas for followup CLs.

Also, would you mind checking that the text output of the
VideoProcessorIntegrationTests are identical before and after this CL? I usually
run

==
git checkout BEFORE
ninja -C out/Debug modules_tests && out/Debug/modules_tests
--gtest_filter="*VideoProcessor*" 2>&1 >/tmp/before.log

git checkout AFTER
ninja -C out/Debug modules_tests && out/Debug/modules_tests
--gtest_filter="*VideoProcessor*" 2>&1 >/tmp/after.log

diff /tmp/before.log /tmp/after.log
==

and then I verify that only the timings are different.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (left):

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:440: const
int tl_idx = TemporalLayerIndexForFrame(frame_number);
I think I prefer the name |tl_idx| here.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right):

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:498: if
(!rc_thresholds)
Why not split this function into PrintRateControlMetrics and
VerifyRateControlMetrics, and then do the diversion at the call site?

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
File modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right):

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.h:151: void
PrintRateControlMetrics(
Declare above PrintAndMaybeVerifyRateControlMetrics, as that functions uses this
function.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.h:172: struct
TestResults {
Move definition to the top of the private: section.

https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.h:179: int
DeltaFrameSizeMismatchPercent(int i) const {
In a followup CL, it would be nice to output the percent as a float instead. I
don't see a good reason to round to the nearest integer really.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.h:199:
TestResults actual_;
What do you think about getting rid of |actual_| and |target_| as member
variables? We could either just pass them into the corresponding rate control
metrics functions, or split out the entire rate control metric calculation into
a utility class.

Let's discuss what the right thing to do is, and then we can think of some
followup CL.

[åsapersson, 2017-09-22 08:06:50]

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (left):

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:440: const
int tl_idx = TemporalLayerIndexForFrame(frame_number);
On 2017/09/21 08:40:10, brandtr wrote:
> I think I prefer the name |tl_idx| here.

Done.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right):

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:498: if
(!rc_thresholds)
On 2017/09/21 08:40:09, brandtr wrote:
> Why not split this function into PrintRateControlMetrics and
> VerifyRateControlMetrics, and then do the diversion at the call site?

Done.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
File modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right):

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.h:151: void
PrintRateControlMetrics(
On 2017/09/21 08:40:10, brandtr wrote:
> Declare above PrintAndMaybeVerifyRateControlMetrics, as that functions uses
this
> function.

Done.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.h:172: struct
TestResults {
On 2017/09/21 08:40:10, brandtr wrote:
> Move definition to the top of the private: section.
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.h:179: int
DeltaFrameSizeMismatchPercent(int i) const {
On 2017/09/21 08:40:10, brandtr wrote:
> In a followup CL, it would be nice to output the percent as a float instead. I
> don't see a good reason to round to the nearest integer really.

Acknowledged.

https://codereview.webrtc.org/3009423002/diff/380001/modules/video_coding/cod...
modules/video_coding/codecs/test/videoprocessor_integrationtest.h:199:
TestResults actual_;
On 2017/09/21 08:40:10, brandtr wrote:
> What do you think about getting rid of |actual_| and |target_| as member
> variables? We could either just pass them into the corresponding rate control
> metrics functions, or split out the entire rate control metric calculation
into
> a utility class.
> 
> Let's discuss what the right thing to do is, and then we can think of some
> followup CL.

Sounds good.

[brandtr, 2017-09-22 08:15:21]

lgtm

[åsapersson, 2017-09-22 09:49:34]

The CQ bit was checked by asapersson@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-22 09:49:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-22 10:11:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-22 10:11:35]

Dry run: This issue passed the CQ dry run.

[åsapersson, 2017-09-22 10:42:49]

The CQ bit was checked by asapersson@webrtc.org

[commit-bot: I haz the power, 2017-09-22 10:42:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-22 10:45:16]

CQ is committing da patch.

Bot data: {"patchset_id": 410001, "attempt_start_ts": 1506076969736100,
"parent_rev": "22d3da9235a93558162133fb43671e6db2669f8c", "commit_rev":
"55c7eded9457bd918927d91080290ea9f6fc6f86"}

[commit-bot: I haz the power, 2017-09-22 10:45:20]

Description was changed from

==========
VideoProcessorIntegrationTest: Group member variables into two structs
containing target/actual rates.

- Group member variables into two structs: target rates/actual rates.
- Split verify and print of rate control metrics into separate functions.
- Rename member variables.

BUG=webrtc:6634
==========

to

==========
VideoProcessorIntegrationTest: Group member variables into two structs
containing target/actual rates.

- Group member variables into two structs: target rates/actual rates.
- Split verify and print of rate control metrics into separate functions.
- Rename member variables.

BUG=webrtc:6634

Review-Url: https://codereview.webrtc.org/3009423002
Cr-Commit-Position: refs/heads/master@{#19925}
Committed:
https://webrtc.googlesource.com/src/+/55c7eded9457bd918927d91080290ea9f6fc6f86
==========

[commit-bot: I haz the power, 2017-09-22 10:45:21]

Committed patchset #4 (id:410001) as
https://webrtc.googlesource.com/src/+/55c7eded9457bd918927d91080290ea9f6fc6f86