Step #2: Add batch mode to VideoProcessor integration tests.

Add batch mode to VideoProcessor integration tests.

Prior to this CL, the encoding/decoding in the VideoProcessor integration
tests were run "online", in the sense that rate allocations could be
changed in between frames. This is useful for evaluating the rate control
of SW codecs, which is one of the reasons for the existence of these
integration tests in the first place.

This CL adds a batch mode, in which the tests are run "offline". The two
main differences to the original mode are: 1) rate control metrics are
calculated after the fact, and 2) no rate allocation changes are allowed
during the test. Difference 1) is the reason for this CL, as HW codecs
that are pipelining will not work well when rate control metrics are
calculated right after a frame has been sent for encode. Difference 2)
is a side effect of the introduction of the batch mode. If we want to
be able to support online rate allocation for pipelining HW codecs in
the future, this can be introduced by adding a delay between encoding
and rate allocation. This was not deemed necessary at this point in time,
and hence this CL does not do that.

The batch mode is only intended to be used for manual experimentation
on devices with HW codecs, and the integration tests running on the
bots should thus NOT use batch mode.

BUG=webrtc:6634
Review-Url: https://codereview.webrtc.org/2707023008
Cr-Commit-Position: refs/heads/master@{#17164}
Committed: https://chromium.googlesource.com/external/webrtc/+/b2def1d06f9edd2b5192675180679847d5318fc7

[brandtr, 2017-02-23 14:35:48]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-02-23 14:59:22]

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

[brandtr, 2017-02-23 14:59:23]

Step #2.

[brandtr, 2017-02-23 15:04:20]

Description was changed from

==========
Add batch mode to VideoProcessor integration tests.

Prior to this CL, the encoding/decoding in the VideoProcessor integration
tests were run "online", in the sense that rate allocations could be
changed in between frames. This is useful for evaluating the rate control
of SW codecs, which is one of the reasons for the existence of these
integration tests in the first place.

This CL adds a batch mode, in which the tests are run "offline". The two
main differences to the original mode are: 1) rate control metrics are
calculated after the fact, and 2) no rate allocation changes are allowed
during the test. Difference 1) is the reason for this CL, as HW codecs
that are pipelining will not work well when rate control metrics are
calculated right after a frame has been sent for encode. Difference 2)
is a side effect of the introduction of the batch mode. If we want to
be able to support online rate allocation for pipelining HW codecs in
the future, this can be introduced by adding a delay between encoding
and rate allocation. This was not deemed necessary at this point in time,
and hence this CL does not do that.

The batch mode is only intended to be used for manual experimentation
on devices with HW codecs, and the integration tests running on the
bots should thus NOT use batch mode.

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

to

==========
Add batch mode to VideoProcessor integration tests.

Prior to this CL, the encoding/decoding in the VideoProcessor integration
tests were run "online", in the sense that rate allocations could be
changed in between frames. This is useful for evaluating the rate control
of SW codecs, which is one of the reasons for the existence of these
integration tests in the first place.

This CL adds a batch mode, in which the tests are run "offline". The two
main differences to the original mode are: 1) rate control metrics are
calculated after the fact, and 2) no rate allocation changes are allowed
during the test. Difference 1) is the reason for this CL, as HW codecs
that are pipelining will not work well when rate control metrics are
calculated right after a frame has been sent for encode. Difference 2)
is a side effect of the introduction of the batch mode. If we want to
be able to support online rate allocation for pipelining HW codecs in
the future, this can be introduced by adding a delay between encoding
and rate allocation. This was not deemed necessary at this point in time,
and hence this CL does not do that.

The batch mode is only intended to be used for manual experimentation
on devices with HW codecs, and the integration tests running on the
bots should thus NOT use batch mode.

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

[brandtr, 2017-02-24 08:19:35]

https://codereview.webrtc.org/2707023008/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2707023008/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:567: //
In batch mode, we calculate the metrics for all frames after all frames
I thought about it, and perhaps it's better to add the batch mode directly to
PlotVideoProcessorIntegrationTest, instead of making the general code more
complicated.

asapersson, WDYT?

[brandtr, 2017-02-28 19:00:22]

Rebase.

[brandtr, 2017-03-06 15:28:54]

Rebase.

[brandtr, 2017-03-06 16:08:15]

https://codereview.webrtc.org/2707023008/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2707023008/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:567: //
In batch mode, we calculate the metrics for all frames after all frames
On 2017/02/24 08:19:35, brandtr wrote:
> I thought about it, and perhaps it's better to add the batch mode directly to
> PlotVideoProcessorIntegrationTest, instead of making the general code more
> complicated.
> 
> asapersson, WDYT?

This test uses data members as globals, so it will be messy to move out this
functionality to another class. I suggest that I leave this functionality here
for now in order to simplify the reviewing, and after I have added HW codec
support (~4 CLs left), I might move this functionality to another class.

[brandtr, 2017-03-07 09:28:51]

Rebase.

[åsapersson, 2017-03-08 15:26:46]

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(left):

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:593:
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, decoder_->Release());
maybe make this change if needed in upcoming cl

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:575:
for (frame_number = 0; frame_number < num_frames; ++frame_number) {
use frame_number <= num_frames and remove the ProcessFrame call below the loop?

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:606:
size_t encoded_frame_size = processor_->EncodedFrameSize(frame_number);
maybe get frame_type and encoded_frame_size in UpdateRateControlMetrics

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:616:
int num_resize_actions = processor_->NumberSpatialResizes();
ditto

[sprang_webrtc, 2017-03-08 16:20:21]

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:385:
int layer = LayerIndexForFrame(frame_number);
Not obvious to me which layer this refers to?

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:575:
for (frame_number = 0; frame_number < num_frames; ++frame_number) {
On 2017/03/08 15:26:46, åsapersson wrote:
> use frame_number <= num_frames and remove the ProcessFrame call below the
loop?

Then the expect becomes a little more complicated though, but might make sense?
Might even make sense to indicate which frame failed in that case.

EXPECT_TRUE(frame_number < num_frames,
            processor_->ProcessFrame(frame_number)) << frame_number;

In any case, might be good to comment on why we want to do that last call.

[brandtr, 2017-03-09 15:03:04]

Patchset #6 (id:120001) has been deleted

[brandtr, 2017-03-09 15:18:27]

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(left):

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:593:
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, decoder_->Release());
On 2017/03/08 15:26:46, åsapersson wrote:
> maybe make this change if needed in upcoming cl

Done.

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:385:
int layer = LayerIndexForFrame(frame_number);
On 2017/03/08 16:20:21, språng wrote:
> Not obvious to me which layer this refers to?

This function returns what TL this frame belongs to. I've updated the naming a
bit to make that more clear.

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:575:
for (frame_number = 0; frame_number < num_frames; ++frame_number) {
On 2017/03/08 16:20:21, språng wrote:
> On 2017/03/08 15:26:46, åsapersson wrote:
> > use frame_number <= num_frames and remove the ProcessFrame call below the
> loop?
> 
> Then the expect becomes a little more complicated though, but might make
sense?
> Might even make sense to indicate which frame failed in that case.
> 
> EXPECT_TRUE(frame_number < num_frames,
>             processor_->ProcessFrame(frame_number)) << frame_number;
> 
> In any case, might be good to comment on why we want to do that last call.

asapersson: Done.

sprang: The last call to ProcessFrame is intended and should actually always
succeed, so I added an EXPECT_TRUE to that call as well.

What's confusing/annoying is that |num_frames| is set as "actual_num_frames" - 1
in the calling code, leading to a bunch of "+1"'s in other places, see e.g.,
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...

This should be cleaned up, so I left a TODO for that.

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:606:
size_t encoded_frame_size = processor_->EncodedFrameSize(frame_number);
On 2017/03/08 15:26:46, åsapersson wrote:
> maybe get frame_type and encoded_frame_size in UpdateRateControlMetrics

Done.

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:616:
int num_resize_actions = processor_->NumberSpatialResizes();
On 2017/03/08 15:26:46, åsapersson wrote:
> ditto

Done.

[åsapersson, 2017-03-10 08:43:27]

lgtm

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:616:
int num_resize_actions = processor_->NumberSpatialResizes();
On 2017/03/09 15:18:27, brandtr wrote:
> On 2017/03/08 15:26:46, åsapersson wrote:
> > ditto
> 
> Done.

Perhaps same here.

[brandtr, 2017-03-10 08:48:40]

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:616:
int num_resize_actions = processor_->NumberSpatialResizes();
On 2017/03/10 08:43:27, åsapersson wrote:
> On 2017/03/09 15:18:27, brandtr wrote:
> > On 2017/03/08 15:26:46, åsapersson wrote:
> > > ditto
> > 
> > Done.
> 
> Perhaps same here.

Sorry for missing that.. :/

[sprang_webrtc, 2017-03-10 09:09:37]

lgtm

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2707023008/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:575:
for (frame_number = 0; frame_number < num_frames; ++frame_number) {
On 2017/03/09 15:18:27, brandtr wrote:
> On 2017/03/08 16:20:21, språng wrote:
> > On 2017/03/08 15:26:46, åsapersson wrote:
> > > use frame_number <= num_frames and remove the ProcessFrame call below the
> > loop?
> > 
> > Then the expect becomes a little more complicated though, but might make
> sense?
> > Might even make sense to indicate which frame failed in that case.
> > 
> > EXPECT_TRUE(frame_number < num_frames,
> >             processor_->ProcessFrame(frame_number)) << frame_number;
> > 
> > In any case, might be good to comment on why we want to do that last call.
> 
> asapersson: Done.
> 
> sprang: The last call to ProcessFrame is intended and should actually always
> succeed, so I added an EXPECT_TRUE to that call as well.
> 
> What's confusing/annoying is that |num_frames| is set as "actual_num_frames" -
1
> in the calling code, leading to a bunch of "+1"'s in other places, see e.g.,
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
> 
> This should be cleaned up, so I left a TODO for that.

Acknowledged.

[brandtr, 2017-03-10 11:53:58]

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

[commit-bot: I haz the power, 2017-03-10 11:54:01]

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-03-10 12:14:24]

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

[commit-bot: I haz the power, 2017-03-10 12:14:25]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-03-10 12:17:47]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-03-10 12:17:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2707023008/#ps180001 (title:
"asapersson comments 2.")

[commit-bot: I haz the power, 2017-03-10 12:17: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-03-10 12:20:11]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489148267094110,
"parent_rev": "c37d2b96b812b4ec2c81d7bce1bb57be13ecaacf", "commit_rev":
"b2def1d06f9edd2b5192675180679847d5318fc7"}

[commit-bot: I haz the power, 2017-03-10 12:20:13]

Description was changed from

==========
Add batch mode to VideoProcessor integration tests.

Prior to this CL, the encoding/decoding in the VideoProcessor integration
tests were run "online", in the sense that rate allocations could be
changed in between frames. This is useful for evaluating the rate control
of SW codecs, which is one of the reasons for the existence of these
integration tests in the first place.

This CL adds a batch mode, in which the tests are run "offline". The two
main differences to the original mode are: 1) rate control metrics are
calculated after the fact, and 2) no rate allocation changes are allowed
during the test. Difference 1) is the reason for this CL, as HW codecs
that are pipelining will not work well when rate control metrics are
calculated right after a frame has been sent for encode. Difference 2)
is a side effect of the introduction of the batch mode. If we want to
be able to support online rate allocation for pipelining HW codecs in
the future, this can be introduced by adding a delay between encoding
and rate allocation. This was not deemed necessary at this point in time,
and hence this CL does not do that.

The batch mode is only intended to be used for manual experimentation
on devices with HW codecs, and the integration tests running on the
bots should thus NOT use batch mode.

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

to

==========
Add batch mode to VideoProcessor integration tests.

Prior to this CL, the encoding/decoding in the VideoProcessor integration
tests were run "online", in the sense that rate allocations could be
changed in between frames. This is useful for evaluating the rate control
of SW codecs, which is one of the reasons for the existence of these
integration tests in the first place.

This CL adds a batch mode, in which the tests are run "offline". The two
main differences to the original mode are: 1) rate control metrics are
calculated after the fact, and 2) no rate allocation changes are allowed
during the test. Difference 1) is the reason for this CL, as HW codecs
that are pipelining will not work well when rate control metrics are
calculated right after a frame has been sent for encode. Difference 2)
is a side effect of the introduction of the batch mode. If we want to
be able to support online rate allocation for pipelining HW codecs in
the future, this can be introduced by adding a delay between encoding
and rate allocation. This was not deemed necessary at this point in time,
and hence this CL does not do that.

The batch mode is only intended to be used for manual experimentation
on devices with HW codecs, and the integration tests running on the
bots should thus NOT use batch mode.

BUG=webrtc:6634

Review-Url: https://codereview.webrtc.org/2707023008
Cr-Commit-Position: refs/heads/master@{#17164}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b2def1d06f9edd2b519267518...
==========

[commit-bot: I haz the power, 2017-03-10 12:20:14]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/b2def1d06f9edd2b519267518...