Add VideoFrameRecorder for use recording test frame sequences.

Add VideoFrameRecorder for use recording test frame sequences.

This will be used to record representative frame sequences for performance evaluation of the new VP9 encoder.

BUG=260879
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279795

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280255

[Wez, 2014-06-18 17:50:07]

PTAL!

[Wez, 2014-06-19 22:41:41]

On 2014/06/18 17:50:07, Wez wrote:
> PTAL!

Ping :)

[Sergey Ulanov, 2014-06-20 22:32:13]

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_recorder.cc (right):

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:20: const int64 kDefaultMaxContentBytes =
10 * 1024 * 1024;
int64_t here and everywhere else in this file.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:20: const int64 kDefaultMaxContentBytes =
10 * 1024 * 1024;
10MB fits about 2 frames - is that enough?

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:22: static int64 FrameContentBytes(const
webrtc::DesktopFrame* frame) {
nit: Maybe GetFrameContentSize()? or just FrameContentSize()

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:23: return frame->stride() *
frame->size().height();
that will be broken when stride is negative for flipped frames. Add DCHECK?

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:26: class
VideoFrameRecorder::RecordingVideoEncoder : public VideoEncoder {
add short comment to describe what this class is used for.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:31: : encoder_(encoder.Pass()),
: needs to be indented 4 spaces,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:45: enable_recording_ = enable_recording;
DCHECK we are on |encoder_taks_runner_|?

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:101: : content_bytes_(0),
indentation

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:155: recorded_frames_.pop_front();
update content_bytes_?

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:172: SetEnableRecording(true);
It looks wrong to reset enable_recording to false, just to set it to true again.
Either post task for SetEnableRecording() here or move PostTask() from
SetEnableRecording() to a separate method and call that method from here.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_recorder.h (right):

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.h:26: // instance that can be passed to the
frame capture engine in its place.
Threading semantics is not very clear. It looks like all methods, including 
WrapVideoEncoder() must be called on the same thread, but the returned
VideoEncode can be called on a different thread.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.h:33: // will route frames to the recorder,
as well as passing them for encoding.
Looks like this function is not allowed to be called more than once (DCHECK in
VideoFrameRecorder::SetEncoderTaskRunner()). Comment should mention it.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.h:42: void SetMaxContentBytes(int64
max_content_bytes);
nit: not sure if it's documented anywhere, but it's better to use int64_t in new
code, instead of int64. E.g. current style guide doesn't even mention int64.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_recorder_unittest.cc (right):

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder_unittest.cc:17: namespace webrtc {
ah, so == operator must be defined in the same namespace?

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder_unittest.cc:19: static bool operator==(const
DesktopRegion& a,
comment why you need to define these.

[Wez, 2014-06-24 01:02:15]

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_recorder.cc (right):

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:20: const int64 kDefaultMaxContentBytes =
10 * 1024 * 1024;
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> int64_t here and everywhere else in this file.

Done.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:20: const int64 kDefaultMaxContentBytes =
10 * 1024 * 1024;
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> 10MB fits about 2 frames - is that enough?

The intention is really for the caller to choose a suitable size; I'm going to
take this default out and require that callers specify the size explicitly.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:22: static int64 FrameContentBytes(const
webrtc::DesktopFrame* frame) {
On 2014/06/20 22:32:12, Sergey Ulanov wrote:
> nit: Maybe GetFrameContentSize()? or just FrameContentSize()

<foo>Size() is too easy to confuse as being DesktopSize related; FrameDataSize()
might make sense, or how about FrameDataBytes()?

Have gone with FrameContentSize() for now.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:23: return frame->stride() *
frame->size().height();
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> that will be broken when stride is negative for flipped frames. Add DCHECK?

FrameContentSize() is only ever called for BasicDesktopFrame()s in this
implementation, FWIW.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:26: class
VideoFrameRecorder::RecordingVideoEncoder : public VideoEncoder {
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> add short comment to describe what this class is used for.

Done.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:31: : encoder_(encoder.Pass()),
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> : needs to be indented 4 spaces,
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Done.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:45: enable_recording_ = enable_recording;
On 2014/06/20 22:32:12, Sergey Ulanov wrote:
> DCHECK we are on |encoder_taks_runner_|?

Since SetEnableRecording() is a simple setter and part of the internal
implementation of the class, I don't think that's really necessary, but sure,
why not. :P

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:101: : content_bytes_(0),
On 2014/06/20 22:32:12, Sergey Ulanov wrote:
> indentation

Done.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:155: recorded_frames_.pop_front();
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> update content_bytes_?

Done + added a test case.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.cc:172: SetEnableRecording(true);
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> It looks wrong to reset enable_recording to false, just to set it to true
again.
> Either post task for SetEnableRecording() here or move PostTask() from
> SetEnableRecording() to a separate method and call that method from here.

Done.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_recorder.h (right):

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.h:26: // instance that can be passed to the
frame capture engine in its place.
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> Threading semantics is not very clear. It looks like all methods, including 
> WrapVideoEncoder() must be called on the same thread, but the returned
> VideoEncode can be called on a different thread.

Done.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.h:33: // will route frames to the recorder,
as well as passing them for encoding.
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> Looks like this function is not allowed to be called more than once (DCHECK in
> VideoFrameRecorder::SetEncoderTaskRunner()). Comment should mention it.

Done.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder.h:42: void SetMaxContentBytes(int64
max_content_bytes);
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> nit: not sure if it's documented anywhere, but it's better to use int64_t in
new
> code, instead of int64. E.g. current style guide doesn't even mention int64.

Done.

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_recorder_unittest.cc (right):

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder_unittest.cc:17: namespace webrtc {
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> ah, so == operator must be defined in the same namespace?

Yeah, apparently so. :(

https://codereview.chromium.org/339073002/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_recorder_unittest.cc:19: static bool operator==(const
DesktopRegion& a,
On 2014/06/20 22:32:13, Sergey Ulanov wrote:
> comment why you need to define these.

Done.

[Wez, 2014-06-25 16:23:22]

Ping.

[Sergey Ulanov, 2014-06-25 16:29:02]

lgtm

[Wez, 2014-06-25 16:55:41]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2014-06-25 16:56:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/339073002/80001

[commit-bot: I haz the power, 2014-06-25 20:03:38]

Change committed as 279795

[Avi (use Gerrit), 2014-06-25 20:41:24]

A revert of this CL has been created in
https://codereview.chromium.org/350903003/ by avi@chromium.org.

The reason for reverting is: Brand new test fails in ASAN:

Direct leak of 112 byte(s) in 1 object(s) allocated from:
#0 0x462ccb in operator new(unsigned long)
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55
#1 0x7ee8f9 in CreateNextFrame remoting/host/video_frame_recorder_unittest.cc:96
#2 0x7ee8f9 in remoting::VideoFrameRecorderTest::CreateTestFrames()
remoting/host/video_frame_recorder_unittest.cc:112
#3 0x7f1184 in
remoting::VideoFrameRecorderTest_EncodeButDontRecord_Test::TestBody()
remoting/host/video_frame_recorder_unittest.cc:269
#4 0xb3bf38 in HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2045
#5 0xb3bf38 in testing::Test::Run() testing/gtest/src/gtest.cc:2061
#6 0xb3e1c9 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237
#7 0xb3ef56 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344
#8 0xb5206a in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4065
#9 0xb516a0 in
HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2045
#10 0xb516a0 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697
#11 0xab66bc in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231
#12 0xab66bc in base::TestSuite::Run() base/test/test_suite.cc:227
#13 0xaada32 in Run base/callback.h:401
#14 0xaada32 in base::(anonymous
namespace)::LaunchUnitTestsInternal(base::Callback<int ()> const&, int, bool,
base::Callback<void ()> const&) base/test/launcher/unit_test_launcher.cc:498
#15 0xaad3ad in base::LaunchUnitTests(int, char**, base::Callback<int ()>
const&) base/test/launcher/unit_test_launcher.cc:553
#16 0x4e8067 in main chrome/test/base/run_all_remoting_unittests.cc:31
#17 0x7f1f660df76c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226
.

[Wez, 2014-06-25 20:46:19]

Rather than reverting, can we just add the missing
STLDeleteElements(test_frames_) in TearDown()?

[Wez, 2014-06-25 22:28:43]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2014-06-25 22:29:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/339073002/100001

[commit-bot: I haz the power, 2014-06-27 04:34:29]

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

[commit-bot: I haz the power, 2014-06-27 04:34:30]

Exceeded time limit waiting for builds to trigger.

[Wez, 2014-06-27 05:29:00]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2014-06-27 05:29:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/339073002/100001

[commit-bot: I haz the power, 2014-06-27 07:39:16]

Change committed as 280255