Add optional visualization file writers to VideoProcessor tests.

Add optional visualization file writers to VideoProcessor tests.

The purpose of this visualization CL is to add the ability to record
video at the source, after encode, and after decode, in the VideoProcessor
tests. These output files can then be replayed and used as a subjective
complement to the objective metric plots given by the existing Python
plotting script.

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

[brandtr, 2017-02-16 14:01:19]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-02-16 14:12:01]

Patchset #1 (id:20001) has been deleted

[brandtr, 2017-02-16 14:12:08]

Patchset #1 (id:40001) has been deleted

[brandtr, 2017-02-16 14:42:13]

Patchset #1 (id:60001) has been deleted

[brandtr, 2017-02-17 08:21:30]

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

[brandtr, 2017-02-17 08:21:31]

asapersson: General review.
sprang: Usage of IvfFileWriter + video_coding OWNER
kjellander: webrtc/test file reader/writer changes.

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/frame_reader.cc (left):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
webrtc/test/testsupport/frame_reader.cc:60: if (input_file_ != NULL) {
Reordered method definitions to match order of method declarations.

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/frame_reader.cc (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
webrtc/test/testsupport/frame_reader.cc:82: return frame_length_in_bytes_;
git cl format did this

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/frame_writer.h (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
webrtc/test/testsupport/frame_writer.h:71: class Y4mFrameWriterImpl : public
YuvFrameWriterImpl {
Y4M files are more convenient to play back in media players than YUV files,
since the former contain metadata about frame size and frame rate.

[sprang_webrtc, 2017-02-17 10:08:23]

lgtm with comments fixed

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor.h (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor.h:172: FrameWriter*
decoded_frame_writer);
What's the difference between analysis_frame_writer and decoded_frame_writer?
Maybe a comment would be good :)

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/frame_reader.h (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
webrtc/test/testsupport/frame_reader.h:56: virtual ~YuvFrameReaderImpl()
override;
"virtual" only in the base class, all subclasses just "override".

[åsapersson, 2017-02-17 15:47:35]

lgtm

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/frame_writer.cc (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
webrtc/test/testsupport/frame_writer.cc:102: output_filename_.c_str());
return false?

[kjellander_webrtc, 2017-02-19 09:02:37]

I find it a little unexpected to find YuvFrameWriterImpl and Y4mFrameWriterImpl
inside frame_writer.cc. Would it make sense to move them into their own files
instead?

I'd also like to see unit tests for the new Y4mFrameWriterImpl code.

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/frame_writer_unittest.cc (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
webrtc/test/testsupport/frame_writer_unittest.cc:64: 
Please add unit tests for Y4mFrameWriterImpl as well.

[brandtr, 2017-02-20 12:23:54]

Patchset #3 (id:120001) has been deleted

[brandtr, 2017-02-20 12:30:35]

Patchset #3 (id:140001) has been deleted

[brandtr, 2017-02-20 12:40:14]

Patchset #3 (id:160001) has been deleted

[brandtr, 2017-02-20 12:45:20]

Patchset #3 (id:180001) has been deleted

[brandtr, 2017-02-20 13:17:19]

On 2017/02/19 09:02:37, kjellander_webrtc wrote:
> I find it a little unexpected to find YuvFrameWriterImpl and
Y4mFrameWriterImpl
> inside frame_writer.cc. Would it make sense to move them into their own files
> instead?

Yep, that makes sense. I separated the Yuv and Y4m writers into separate .cc's,
and also renamed the Yuv file reader.

> 
> I'd also like to see unit tests for the new Y4mFrameWriterImpl code.

Done. I also took the opportunity to update the style of the old unit tests a
bit.

>
https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
> File webrtc/test/testsupport/frame_writer_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
> webrtc/test/testsupport/frame_writer_unittest.cc:64: 
> Please add unit tests for Y4mFrameWriterImpl as well.

[brandtr, 2017-02-20 13:17:31]

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor.h (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor.h:172: FrameWriter*
decoded_frame_writer);
On 2017/02/17 10:08:22, språng wrote:
> What's the difference between analysis_frame_writer and decoded_frame_writer?
> Maybe a comment would be good :)
Done.

The difference is very small: |decoded_frame_writer| writes out the same frames
as |analysis_frame_writer| but with Y4M headers, and further, the resulting
files are not deleted at the end of the test run. It's not a super efficient
solution, but a simple way to output the decoded stream so that it is easily
playable in external media players.

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/frame_reader.h (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
webrtc/test/testsupport/frame_reader.h:56: virtual ~YuvFrameReaderImpl()
override;
On 2017/02/17 10:08:22, språng wrote:
> "virtual" only in the base class, all subclasses just "override".

Done.

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/frame_writer_unittest.cc (right):

https://codereview.webrtc.org/2700493006/diff/80001/webrtc/test/testsupport/f...
webrtc/test/testsupport/frame_writer_unittest.cc:64: 
On 2017/02/19 09:02:37, kjellander_webrtc wrote:
> Please add unit tests for Y4mFrameWriterImpl as well.

Done.

[kjellander_webrtc, 2017-02-20 21:41:47]

Nice work, mainly nits.

https://codereview.webrtc.org/2700493006/diff/200001/webrtc/test/testsupport/...
File webrtc/test/testsupport/y4m_frame_writer.cc (right):

https://codereview.webrtc.org/2700493006/diff/200001/webrtc/test/testsupport/...
webrtc/test/testsupport/y4m_frame_writer.cc:2: *  Copyright (c) 2011 The WebRTC
project authors. All Rights Reserved.
New file means 2017. Same for the other new files.

https://codereview.webrtc.org/2700493006/diff/200001/webrtc/test/testsupport/...
webrtc/test/testsupport/y4m_frame_writer.cc:49: output_filename_.c_str());
return false?

[brandtr, 2017-02-21 07:59:31]

Headers updated.

https://codereview.webrtc.org/2700493006/diff/200001/webrtc/test/testsupport/...
File webrtc/test/testsupport/y4m_frame_writer.cc (right):

https://codereview.webrtc.org/2700493006/diff/200001/webrtc/test/testsupport/...
webrtc/test/testsupport/y4m_frame_writer.cc:49: output_filename_.c_str());
On 2017/02/20 21:41:47, kjellander_webrtc wrote:
> return false?

Yes! Not sure why that was left out..

[kjellander_webrtc, 2017-02-21 08:34:09]

lgtm

[brandtr, 2017-02-21 08:39:33]

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

[commit-bot: I haz the power, 2017-02-21 08:39:48]

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-02-21 11:52:35]

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

[commit-bot: I haz the power, 2017-02-21 11:52:36]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-02-21 11:56:09]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-02-21 11:56:10]

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

[commit-bot: I haz the power, 2017-02-21 11:56:18]

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-02-21 11:59:21]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487678169438710,
"parent_rev": "7d59f6b1c46cf551d2b55849ed061756ba25f1b0", "commit_rev":
"872104ac41d7764f8676c9ea55555210bea4605c"}

[commit-bot: I haz the power, 2017-02-21 11:59:25]

Description was changed from

==========
Add optional visualization file writers to VideoProcessor tests.

The purpose of this visualization CL is to add the ability to record
video at the source, after encode, and after decode, in the VideoProcessor
tests. These output files can then be replayed and used as a subjective
complement to the objective metric plots given by the existing Python
plotting script.

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

to

==========
Add optional visualization file writers to VideoProcessor tests.

The purpose of this visualization CL is to add the ability to record
video at the source, after encode, and after decode, in the VideoProcessor
tests. These output files can then be replayed and used as a subjective
complement to the objective metric plots given by the existing Python
plotting script.

BUG=webrtc:6634

Review-Url: https://codereview.webrtc.org/2700493006
Cr-Commit-Position: refs/heads/master@{#16738}
Committed:
https://chromium.googlesource.com/external/webrtc/+/872104ac41d7764f8676c9ea5...
==========

[commit-bot: I haz the power, 2017-02-21 11:59:26]

Committed patchset #4 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/872104ac41d7764f8676c9ea5...

[brandtr, 2017-02-21 13:23:51]

A revert of this CL (patchset #4 id:220001) has been created in
https://codereview.webrtc.org/2708103002/ by brandtr@webrtc.org.

The reason for reverting is: Breaks downstream project..