New helper function test::ReadI420Buffer, refactor FrameReader to use it.

New helper function test::ReadI420Buffer, refactor FrameReader to use it.

This change reduces the number of places where we first fread a I420
frame into a uint8_t buffer, followed by a copy into a frame buffer
object.

BUG=None

Committed: https://crrev.com/115bd153c7f5a64eabbac222bad66e223abbfe2b
Cr-Commit-Position: refs/heads/master@{#14456}

[nisse-webrtc, 2016-09-22 11:06:30]

nisse@webrtc.org changed reviewers:
+ kjellander@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2016-09-22 11:06:30]

PTAL.

Kjellander: FrameReader, in the test_support target, now uses a helper in the
video_test_common target, so I had to tweak dependencies, please review.

[kjellander_webrtc, 2016-09-22 11:29:37]

lgtm with comments addressed.
Are you sure you cannot reference this to any existing bug?

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
File webrtc/test/testsupport/frame_reader.cc (right):

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader.cc:40: width_ * height_ + 2 * ((width_ + 1)
/ 2) * ((height_ + 1) / 2);
I assume this is for I420? Should the class be renamed or at least documented so
it's clear it can only read I420 frames?

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
File webrtc/test/testsupport/frame_reader.h (right):

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader.h:36: // Reads a frame into the supplied
buffer, which must contain enough space
This comment also is a bit odd now, a I420Buffer should always have "enough
space for the frame size" :)

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader.h:57: //   frame_length_in_bytes   The size
of each frame.
Update/remove this documentation

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
File webrtc/test/testsupport/frame_reader_unittest.cc (right):

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader_unittest.cc:21: const size_t kFrameLength =
3;
Please move this into the InitSuccess test instead. It's just confusing to have
it here since it's now only used in that method.

[nisse-webrtc, 2016-09-22 11:48:15]

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
File webrtc/test/testsupport/frame_reader.cc (right):

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader.cc:40: width_ * height_ + 2 * ((width_ + 1)
/ 2) * ((height_ + 1) / 2);
On 2016/09/22 11:29:37, kjellander_webrtc wrote:
> I assume this is for I420? Should the class be renamed or at least documented
so
> it's clear it can only read I420 frames?

I'm updating the comment in the header file.

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
File webrtc/test/testsupport/frame_reader.h (right):

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader.h:36: // Reads a frame into the supplied
buffer, which must contain enough space
On 2016/09/22 11:29:37, kjellander_webrtc wrote:
> This comment also is a bit odd now, a I420Buffer should always have "enough
> space for the frame size" :)

Rewriting the comment.

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader.h:57: //   frame_length_in_bytes   The size
of each frame.
On 2016/09/22 11:29:37, kjellander_webrtc wrote:
> Update/remove this documentation

Done.

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
File webrtc/test/testsupport/frame_reader_unittest.cc (right):

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader_unittest.cc:21: const size_t kFrameLength =
3;
On 2016/09/22 11:29:37, kjellander_webrtc wrote:
> Please move this into the InitSuccess test instead. It's just confusing to
have
> it here since it's now only used in that method.

Hmm, but it's also the length of the string on the preceding line. So I think it
makes sense to keep the definitions together.

[nisse-webrtc, 2016-09-22 11:57:06]

On 2016/09/22 11:29:37, kjellander_webrtc wrote:
> lgtm with comments addressed.
> Are you sure you cannot reference this to any existing bug?

I find no obvious bug to associate it with, but maybe I'm not using the right
search keywords (tried "FrameReader" and "copy").

I could create one, of course.

[kjellander_webrtc, 2016-09-22 13:46:40]

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
File webrtc/test/testsupport/frame_reader_unittest.cc (right):

https://codereview.webrtc.org/2362683002/diff/1/webrtc/test/testsupport/frame...
webrtc/test/testsupport/frame_reader_unittest.cc:21: const size_t kFrameLength =
3;
On 2016/09/22 11:48:15, nisse-webrtc wrote:
> On 2016/09/22 11:29:37, kjellander_webrtc wrote:
> > Please move this into the InitSuccess test instead. It's just confusing to
> have
> > it here since it's now only used in that method.
> 
> Hmm, but it's also the length of the string on the preceding line. So I think
it
> makes sense to keep the definitions together.

Oh, yes it is..  That's an odd video frame :) Maybe we should read a binary byte
stream from disk instead?

[nisse-webrtc, 2016-09-27 14:27:18]

Commenting on my own cl.

https://codereview.webrtc.org/2362683002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right):

https://codereview.webrtc.org/2362683002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:151: // No
scaling in out case, just a copy, to add stride to the image.
Typo, will change s/out/our/.

https://codereview.webrtc.org/2362683002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:155: new
VideoFrame(compact_buffer, kVideoRotation_0, 0));
This should be stride_buffer. However, tests seem to pass even with
compact_buffer here. Why do we use stride != width in these tests?

https://codereview.webrtc.org/2362683002/diff/80001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/video_processing_unittest.cc (left):

https://codereview.webrtc.org/2362683002/diff/80001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/video_processing_unittest.cc:230: offset_x
&= 1;
This was completely broken, but uncaught in earlier tests. Hopefully fixed now.

[stefan-webrtc, 2016-09-29 12:57:38]

I've not reviewed webrtc/test in detail, but I trust kjellander, lgtm.

[nisse-webrtc, 2016-09-30 06:37:52]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-09-30 06:37:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2362683002/#ps100001 (title:
"Fix stride logic in TestVp8Impl.")

[commit-bot: I haz the power, 2016-09-30 06:38:03]

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, 2016-09-30 07:02:21]

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

[commit-bot: I haz the power, 2016-09-30 07:02:22]

Try jobs failed on following builders:
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/18236)

[nisse-webrtc, 2016-09-30 07:37:38]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-09-30 07:37:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2362683002/#ps120001 (title:
"Added size_t/int casts, needed for windows builds.")

[commit-bot: I haz the power, 2016-09-30 07:37:41]

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, 2016-09-30 07:46:10]

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

[commit-bot: I haz the power, 2016-09-30 07:46:11]

Try jobs failed on following builders:
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/18242)

[nisse-webrtc, 2016-09-30 09:16:28]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-09-30 09:16:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2362683002/#ps140001 (title:
"Another try to fix the 64-bit windows build.")

[commit-bot: I haz the power, 2016-09-30 09:16:39]

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, 2016-09-30 11:14:11]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-30 11:14:16]

Description was changed from

==========
New helper function test::ReadI420Buffer, refactor FrameReader to use it.

This change reduces the number of places where we first fread a I420
frame into a uint8_t buffer, followed by a copy into a frame buffer
object.

BUG=None
==========

to

==========
New helper function test::ReadI420Buffer, refactor FrameReader to use it.

This change reduces the number of places where we first fread a I420
frame into a uint8_t buffer, followed by a copy into a frame buffer
object.

BUG=None

Committed: https://crrev.com/115bd153c7f5a64eabbac222bad66e223abbfe2b
Cr-Commit-Position: refs/heads/master@{#14456}
==========

[commit-bot: I haz the power, 2016-09-30 11:14:17]

Patchset 8 (id:??) landed as
https://crrev.com/115bd153c7f5a64eabbac222bad66e223abbfe2b
Cr-Commit-Position: refs/heads/master@{#14456}