Step #4: Run VideoProcessor integration test batch mode on task queue.

Run VideoProcessor integration test batch mode on task queue.

Since the AndroidMediaEncoder nowadays must be run on a task queue,
this CL changes the batch mode (which is always used for HW codecs)
to call the VideoProcessor on a task queue.

Further, it changes the PlotVideoProcessorIntegrationTest to be
parameterized over codec implementation.

BUG=webrtc:6634

[brandtr, 2017-03-10 08:55:27]

Description was changed from

==========
Run VideoProcessor integration test batch mode on task queue.

Since the AndroidMediaEncoder nowadays must be run on a task queue,
this CL changes the batch mode (which is always used for HW codecs)
to call the VideoProcessor on a task queue.

Further, it changes the PlotVideoProcessorIntegrationTest to be
parameterized over codec implementation.

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

to

==========
Run VideoProcessor integration test batch mode on task queue.

Since the AndroidMediaEncoder nowadays must be run on a task queue,
this CL changes the batch mode (which is always used for HW codecs)
to call the VideoProcessor on a task queue.

Further, it changes the PlotVideoProcessorIntegrationTest to be
parameterized over codec implementation.

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

[brandtr, 2017-03-10 09:44:48]

Rebase.

[brandtr, 2017-03-10 09:50:55]

With this CL, we can finally run the PlotVideoProcessor tests on Android HW
codecs. iOS HW codecs are not supported yet -- to do that we need to intercept
their callbacks and post them to the task queue, to ensure synchronization on
the VideoProcessor.

I decided to only move "batch mode" to a task queue, to not impact how the SW
correctness tests are run on the bots (in the modules_tests target).

Let me know what you think.

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:209:
encoder_->RegisterEncodeCompleteCallback(nullptr);
Must be called on the task queue.

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(left):

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:814:
int num_key_frames_;
Trying to reduce the number of members, to make it easier to reason about the
data flow during the test.

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

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:358:
RTC_CHECK(processor_->Init());
Must be called on the task queue.

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:563:
void ProcessFramesAndVerify(QualityThresholds quality_thresholds,
This function is called on the main thread, from the gtest bodies.

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:603:
sync_event.Wait(rtc::Event::kForever);
VideoProcessor is not thread safe, so we need to synchronize all access to it.

[brandtr, 2017-03-10 09:51:19]

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

[brandtr, 2017-03-10 09:51:19]

This time with reviewers....
==

With this CL, we can finally run the PlotVideoProcessor tests on Android HW
codecs. iOS HW codecs are not supported yet -- to do that we need to intercept
their callbacks and post them to the task queue, to ensure synchronization on
the VideoProcessor.

I decided to only move "batch mode" to a task queue, to not impact how the SW
correctness tests are run on the bots (in the modules_tests target).

Let me know what you think.

[tommi, 2017-03-10 09:57:12]

drive by suggestion

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):

https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:209:
encoder_->RegisterEncodeCompleteCallback(nullptr);
On 2017/03/10 09:50:54, brandtr wrote:
> Must be called on the task queue.

as a drive by comment, there aren't any thread or sequence checks in this class.
In this case it looks like you found out about it by hitting a check inside
RegisterEncodeCompleteCallback.
There aren't locks either in VideoProcessorImpl, which suggests that the class
is single threaded. I think it would be a good idea to add those checks both for
sake of runtime correctness/expectations and understanding how the class is
supposed to be used.

[brandtr, 2017-03-10 10:14:30]

On 2017/03/10 09:57:12, tommi (webrtc) wrote:
> drive by suggestion
> 
>
https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
> File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):
> 
>
https://codereview.webrtc.org/2741953002/diff/60001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/codecs/test/videoprocessor.cc:209:
> encoder_->RegisterEncodeCompleteCallback(nullptr);
> On 2017/03/10 09:50:54, brandtr wrote:
> > Must be called on the task queue.
> 
> as a drive by comment, there aren't any thread or sequence checks in this
class.
> In this case it looks like you found out about it by hitting a check inside
> RegisterEncodeCompleteCallback.
> There aren't locks either in VideoProcessorImpl, which suggests that the class
> is single threaded. I think it would be a good idea to add those checks both
for
> sake of runtime correctness/expectations and understanding how the class is
> supposed to be used.

Yes, that makes sense! I'll keep VideoProcessorImpl single-threaded and add a
SequencedTaskChecker to it. That should make it clear that we expect to run that
class either on the main thread, or solely on the task queue.

Thanks for the feedback.

[brandtr, 2017-03-10 12:23:04]

Rebase.

[brandtr, 2017-03-10 15:22:13]

Rebase.

[brandtr, 2017-03-13 08:49:03]

Rebase.

[brandtr, 2017-03-17 13:46:52]

Rebase.

[sprang_webrtc, 2017-03-20 19:36:17]

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:163:
VideoProcessorImpl::~VideoProcessorImpl() = default;
nit:  Could you declare ~VideoProcessor and ~VideoProcessorImpl in the same way?
Either inline in the header or here, but not one of both :)

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

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:72:
const int kNumMillisecondsPerSecond = 1000;
use rtc::kNumMillisPerSecond instead?

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:441:
sync_event.Wait(rtc::Event::kForever);
nit: ASSERT_TRUE() maybe?

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:645:
new rtc::TaskQueue("Integration test task queue."));
might want to choose a shorter name. think some oses has as 12 character limit
or something on the thread names...

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:666: //
so we do some rough pacing here.
How do they get "stressed out"? Any other form of backpressure we can add to
avoid sleeping?

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:674:
SleepMs(1 * kNumMillisecondsPerSecond);
drop "1 *"

[brandtr, 2017-03-21 09:40:53]

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

[brandtr, 2017-03-21 09:40:54]

Addressed tommi and sprang comments 1.

tommi: Would you mind taking a second look on how I use the task queue?

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:163:
VideoProcessorImpl::~VideoProcessorImpl() = default;
On 2017/03/20 19:36:17, språng wrote:
> nit:  Could you declare ~VideoProcessor and ~VideoProcessorImpl in the same
way?
> Either inline in the header or here, but not one of both :)

Done.

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

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:72:
const int kNumMillisecondsPerSecond = 1000;
On 2017/03/20 19:36:17, språng wrote:
> use rtc::kNumMillisPerSecond instead?

Done.

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:441:
sync_event.Wait(rtc::Event::kForever);
On 2017/03/20 19:36:17, språng wrote:
> nit: ASSERT_TRUE() maybe?

Done.

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:645:
new rtc::TaskQueue("Integration test task queue."));
On 2017/03/20 19:36:17, språng wrote:
> might want to choose a shorter name. think some oses has as 12 character limit
> or something on the thread names...

Done.

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:666: //
so we do some rough pacing here.
On 2017/03/20 19:36:17, språng wrote:
> How do they get "stressed out"? Any other form of backpressure we can add to
> avoid sleeping?

Expanded the comment.

Another approach would be to post delayed tasks with a linearly increasing delay
calculated something like "delay_ms = (1000/frame_rate) * frame_number". (This
assumes that the posting of the tasks is almost instantaneous.) This would leave
the pacing of the encode operations to the task queue but I would have to post a
delayed "synchronization task" at the end, which seems a bit odd. I don't fully
understand what happens when normal and delayed tasks are mixed on the same task
queue and that's why I went with the sleep operation instead.

https://codereview.webrtc.org/2741953002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:674:
SleepMs(1 * kNumMillisecondsPerSecond);
On 2017/03/20 19:36:17, språng wrote:
> drop "1 *"

Done.

[tommi, 2017-03-21 10:03:27]

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:214:
RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
if DeregisterCallbacks() is always followed by deleting the object, we could
just merge the two and post a task to delete the object on the task queue.

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:318:
RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
nit: keep these checks at the top of each function for consistency. same below.

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor.h (right):

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.h:142: virtual void
DeregisterCallbacks() = 0;
Add documentation?
(e.g. if there is a condition that must be met before calling this method, what
callbacks are unregistered, if there's a post condition such as no other methods
should be called etc).

If DeregisterCallbacks matches with Init(), then it would be good to call that
out and even change the name to something like Term, Terminate or
Uninitialize().  Having said that - could all the work done in Init() be done in
the constructor?

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.h:217:
task_queue_(rtc::TaskQueue::Current()) {}
if task_queue_ should never be null, please DCHECK that in the ctor so that we
catch the error as early as possible

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.h:225: if (task_queue_ &&
!task_queue_->IsCurrent()) {
it feels to me that the class has too many ways of operating.  I'd prefer to
either require a task queue or not. If it requires a task queue, we should never
need to check if task_queue_ is null.  It would also be good to either never
have to Post or always, inside this function.  If OnEncodedImage is always
called outside the task queue, we shouldn't have to check if it is, etc. Rather
DCHECK the expected condition.

[brandtr, 2017-03-27 11:17:38]

Clean up usage of FrameInfos.

[brandtr, 2017-06-28 07:34:52]

Fixes.

[brandtr, 2017-06-28 07:47:23]

Fix gn again.

[brandtr, 2017-06-28 09:40:25]

Rebased and updated. Please take a look again :)

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:214:
RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
On 2017/03/21 10:03:27, tommi (webrtc) wrote:
> if DeregisterCallbacks() is always followed by deleting the object, we could
> just merge the two and post a task to delete the object on the task queue.

The calling of Deinit() is not directly followed by the destruction, and that's
why it's useful to have this separate method.

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:318:
RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_);
On 2017/03/21 10:03:27, tommi (webrtc) wrote:
> nit: keep these checks at the top of each function for consistency. same
below.

Done.

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor.h (right):

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.h:142: virtual void
DeregisterCallbacks() = 0;
On 2017/03/21 10:03:27, tommi (webrtc) wrote:
> Add documentation?
> (e.g. if there is a condition that must be met before calling this method,
what
> callbacks are unregistered, if there's a post condition such as no other
methods
> should be called etc).
> 
> If DeregisterCallbacks matches with Init(), then it would be good to call that
> out and even change the name to something like Term, Terminate or
> Uninitialize().  Having said that - could all the work done in Init() be done
in
> the constructor?

Documentation added. Semantics and name have changed to better match the
::Release() semantics of the underlying encoder.

The work done in Init() could be done in the ctor, but the work done in
Release() could not be done in the dtor. The reason for the latter being that we
want to ensure that the encoding/decoding is completely done (i.e., no lingering
frames anywhere) when we access the state of |this|, at the end of a measurement
run.

I'm keeping Init/Release this way, as it matches how we init and release the
codecs.

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.h:217:
task_queue_(rtc::TaskQueue::Current()) {}
On 2017/03/21 10:03:27, tommi (webrtc) wrote:
> if task_queue_ should never be null, please DCHECK that in the ctor so that we
> catch the error as early as possible

|task_queue_| can be null when the VideoProcessor is operated on a "normal"
thread. This happens in the SW codec tests on the bots, but not in the manual HW
codec tests on device.

The next step should be to split up these tests, so we don't have to make this
distinction.

https://codereview.webrtc.org/2741953002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.h:225: if (task_queue_ &&
!task_queue_->IsCurrent()) {
On 2017/03/21 10:03:27, tommi (webrtc) wrote:
> it feels to me that the class has too many ways of operating.  I'd prefer to
> either require a task queue or not. If it requires a task queue, we should
never
> need to check if task_queue_ is null.  It would also be good to either never
> have to Post or always, inside this function.  If OnEncodedImage is always
> called outside the task queue, we shouldn't have to check if it is, etc.
Rather
> DCHECK the expected condition.

I agree that it is messy. The underlying reason is our lack of guarantees for
what thread is being used for the codec callbacks. This is currently my
impression of how things are implemented:

Encoders
==
- SW VP8 calls OnEncodedImage synchronously (+)
- Android MediaCodec calls OnEncodedImage from a recurring task running on an
external task queue
- iOS VideoToolbox calls OnEncodedImage from an internal GCD task queue (*)

Decoders
==
- SW VP8 calls Decoded synchronously (+)
- Android MediaCodec calls Decoded from an internal thread (*)
- iOS VideoToolbox calls Decoded from an internal GCD task queue (*)

For the cases marked by (+), this class is not created on a task queue, and the
callbacks are directly forwarded. For the cases marked by (*), this class is
created on a task queue but the callbacks are arriving on some other thread, so
this class posts the callbacks on the correct task queue. For the final case
(Android encode), the callback is arriving on the same task queue as the class
was created on, so it is directly forwarded.

By splitting up the bot tests from the manual tests, I might be able to clean
this up and require the VideoProcessor to run on a task queue at all times.