Add support for writing raw encoder output to .ivf files.

Add support for writing raw encoder output to .ivf files.

Also refactor GenericEncoder to use these file writers, and remove use
of preprocessor to enable file writing.

BUG=

Committed: https://crrev.com/3911c26bc01fa4d5236c6d84b6d7a1e3443a79ba
Cr-Commit-Position: refs/heads/master@{#12372}

[sprang_webrtc, 2016-04-01 15:35:57]

sprang@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2016-04-04 11:22:23]

https://codereview.webrtc.org/1853813002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/1853813002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.h:79: static const bool
kEnableFrameRecording = false;
I think this should be plumbed further out through:

webrtc/video/vie_encoder.cc:411:int32_t ViEEncoder::SendData(const uint8_t
payload_type,

And that the file writing should be done there.

[sprang_webrtc, 2016-04-05 10:35:38]

https://codereview.webrtc.org/1853813002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/1853813002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.h:79: static const bool
kEnableFrameRecording = false;
On 2016/04/04 11:22:23, pbos-webrtc wrote:
> I think this should be plumbed further out through:
> 
> webrtc/video/vie_encoder.cc:411:int32_t ViEEncoder::SendData(const uint8_t
> payload_type,
> 
> And that the file writing should be done there.

Done.

[pbos-webrtc, 2016-04-05 13:31:28]

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.h:78: VideoRotation _rotation;
Can you git cl format this entire class so that we don't have inconsistent
spacing?

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/ivf_file_writer.cc (right):

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:17: namespace webrtc {
I think this class needs unittests, can you add any simple such tests covering
all codecs?

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/ivf_file_writer.h (right):

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/ivf_file_writer.h:34: explicit
IvfFileWriter(const char* file_name, FILE* file);
drop explicit

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:431: if (kEnableFrameRecording) {
This isn't done on a known thread, for simulcast with hardware encoders you have
data races here (all hardware encoders use separate threads for callbacks).

[sprang_webrtc, 2016-04-05 16:00:53]

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.h:78: VideoRotation _rotation;
On 2016/04/05 13:31:27, pbos-webrtc wrote:
> Can you git cl format this entire class so that we don't have inconsistent
> spacing?

Done, reformatted and cleaned up generic_encoder.(h|cc)

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/ivf_file_writer.cc (right):

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:17: namespace webrtc {
On 2016/04/05 13:31:28, pbos-webrtc wrote:
> I think this class needs unittests, can you add any simple such tests covering
> all codecs?

Uhh.. That feels like a lot of work for a hack you need to hard-code enable to
use. I should never have cleaned up the old code :)
But seriously, let's do that with a follow-up CL? I plan to use this in a test
and possibly on the receive side anyway.

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/ivf_file_writer.h (right):

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/ivf_file_writer.h:34: explicit
IvfFileWriter(const char* file_name, FILE* file);
On 2016/04/05 13:31:28, pbos-webrtc wrote:
> drop explicit

Done.

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1853813002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:431: if (kEnableFrameRecording) {
On 2016/04/05 13:31:28, pbos-webrtc wrote:
> This isn't done on a known thread, for simulcast with hardware encoders you
have
> data races here (all hardware encoders use separate threads for callbacks).

Ah. I'm assuming now that callbacks are at least not overlapping per simulcast
stream? Otherwise we could have reordering even before packetization?

[sprang_webrtc, 2016-04-06 15:55:33]

Added tests. ptal

[pbos-webrtc, 2016-04-07 16:16:04]

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:2: *  Copyright (c) 2012 The
WebRTC project authors. All Rights Reserved.
Comment formatting broken.

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:63: };  // VCMEncodedFrameCallback
Remove comment

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:74: int32_t Release();  // Free
encoder memory.
Remove comment

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/ivf_file_writer.cc (right):

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:21: rtc::StreamInterface*
out_stream)
Should this take an unique_ptr as part of the interface?

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:28: RTC_CHECK(out_stream
!= nullptr);
drop != nullptr

[pbos-webrtc, 2016-04-07 16:16:15]

lgtm, I trust you on the rest

[sprang_webrtc, 2016-04-11 15:57:28]

I had to rework this to not depend on rtc_base, and extended file recording to
the receive side as well. Plus some shotgun reformatting of video_receiver and
video_coding_impl. Oops.

Maybe you want to take another pass?

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:2: *  Copyright (c) 2012 The
WebRTC project authors. All Rights Reserved.
On 2016/04/07 16:16:04, pbos-webrtc wrote:
> Comment formatting broken.

Done.

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:63: };  // VCMEncodedFrameCallback
On 2016/04/07 16:16:04, pbos-webrtc wrote:
> Remove comment

Done.

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:74: int32_t Release();  // Free
encoder memory.
On 2016/04/07 16:16:04, pbos-webrtc wrote:
> Remove comment

Done.

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/ivf_file_writer.cc (right):

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:21: rtc::StreamInterface*
out_stream)
On 2016/04/07 16:16:04, pbos-webrtc wrote:
> Should this take an unique_ptr as part of the interface?

Done.

https://codereview.webrtc.org/1853813002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:28: RTC_CHECK(out_stream
!= nullptr);
On 2016/04/07 16:16:04, pbos-webrtc wrote:
> drop != nullptr

Done.

[mflodman, 2016-04-12 08:02:37]

mflodman@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[mflodman, 2016-04-12 08:02:38]

https://codereview.webrtc.org/1853813002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/ivf_file_writer.cc (right):

https://codereview.webrtc.org/1853813002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:64:
ByteWriter<uint16_t>::WriteLittleEndian(&ivf_header[4], 0);   // Verison.
Verison typo.

[mflodman, 2016-04-12 08:05:49]

Then ViEEncoder LGTM when pbos is happy.

[pbos-webrtc, 2016-04-12 11:20:56]

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.h (right):

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:195: rtc::CriticalSection
process_lock_;
s/lock/crit for consistency

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:222: rtc::CriticalSection
file_write_lock_;
crit_, but I think all of this should be moved to VideoReceiveStream

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:223: uint32_t ssrc_;
There shouldn't be a SSRC for a video-coding module.

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_receiver.cc:297:
pre_decode_image_callback_->Encoded(encoded_image, frame->CodecSpecific(),
Can you wire up this through here instead? Then you could put the logic for
writing the file in VideoReceiveStream instead of down in this module. I don't
think any SSRC things belong this far down in the module.

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_receiver.cc:370: bool ok =
file_writer_->WriteFrame(frame.EncodedImage());
This is now done under the receive critsect?

[pbos-webrtc, 2016-04-12 11:22:34]

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_receiver.cc:562: pre_decode_image_callback_ =
observer;
Actually, can we require this to be set before receiving starts so that we don't
have to grab receive_lock_ while writing to file?

[sprang_webrtc, 2016-04-12 14:06:18]

https://codereview.webrtc.org/1853813002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/ivf_file_writer.cc (right):

https://codereview.webrtc.org/1853813002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:64:
ByteWriter<uint16_t>::WriteLittleEndian(&ivf_header[4], 0);   // Verison.
On 2016/04/12 08:02:38, mflodman wrote:
> Verison typo.

Done.

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.h (right):

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:195: rtc::CriticalSection
process_lock_;
On 2016/04/12 11:20:55, pbos-webrtc wrote:
> s/lock/crit for consistency

Done.

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:222: rtc::CriticalSection
file_write_lock_;
On 2016/04/12 11:20:55, pbos-webrtc wrote:
> crit_, but I think all of this should be moved to VideoReceiveStream

Done.

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:223: uint32_t ssrc_;
On 2016/04/12 11:20:55, pbos-webrtc wrote:
> There shouldn't be a SSRC for a video-coding module.

Acknowledged.

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_receiver.cc:297:
pre_decode_image_callback_->Encoded(encoded_image, frame->CodecSpecific(),
On 2016/04/12 11:20:55, pbos-webrtc wrote:
> Can you wire up this through here instead? Then you could put the logic for
> writing the file in VideoReceiveStream instead of down in this module. I don't
> think any SSRC things belong this far down in the module.

Done.

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_receiver.cc:370: bool ok =
file_writer_->WriteFrame(frame.EncodedImage());
On 2016/04/12 11:20:55, pbos-webrtc wrote:
> This is now done under the receive critsect?

Yes.

https://codereview.webrtc.org/1853813002/diff/240001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_receiver.cc:562: pre_decode_image_callback_ =
observer;
On 2016/04/12 11:22:33, pbos-webrtc wrote:
> Actually, can we require this to be set before receiving starts so that we
don't
> have to grab receive_lock_ while writing to file?

Done.

[pbos-webrtc, 2016-04-13 22:01:14]

lgtm

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.h (right):

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:136: KeyFrameRequestSender*
keyframe_request_sender = nullptr,
no default arguments

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:203: GUARDED_BY(process_crit_);
git cl format

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:92: static const bool kEnableFrameRecording
= false;
This (= false) part should go in the cc file I think. And if it's only used in
the .cc file, then keep it inside the .cc file.

[sprang_webrtc, 2016-04-14 07:36:23]

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.h (right):

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:136: KeyFrameRequestSender*
keyframe_request_sender = nullptr,
On 2016/04/13 22:01:14, pbos-webrtc wrote:
> no default arguments

Done.

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:203: GUARDED_BY(process_crit_);
On 2016/04/13 22:01:14, pbos-webrtc wrote:
> git cl format

That's already the output from git cl format.

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/1853813002/diff/280001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:92: static const bool kEnableFrameRecording
= false;
On 2016/04/13 22:01:14, pbos-webrtc wrote:
> This (= false) part should go in the cc file I think. And if it's only used in
> the .cc file, then keep it inside the .cc file.

Done.

[sprang_webrtc, 2016-04-14 14:28:28]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-04-14 14:28:28]

The patchset sent to the CQ was uploaded after l-g-t-m from mflodman@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1853813002/#ps320001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-04-14 14:28:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1853813002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1853813002/320001

[commit-bot: I haz the power, 2016-04-14 16:29:18]

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

[commit-bot: I haz the power, 2016-04-14 16:29:20]

Try jobs failed on following builders:
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[sprang_webrtc, 2016-04-15 08:11:32]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2016-04-15 08:11:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1853813002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1853813002/320001

[commit-bot: I haz the power, 2016-04-15 08:24:19]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-04-15 08:24:28]

Description was changed from

==========
Add support for writing raw encoder output to .ivf files.

Also refactor GenericEncoder to use these file writers, and remove use
of preprocessor to enable file writing.

BUG=
==========

to

==========
Add support for writing raw encoder output to .ivf files.

Also refactor GenericEncoder to use these file writers, and remove use
of preprocessor to enable file writing.

BUG=

Committed: https://crrev.com/3911c26bc01fa4d5236c6d84b6d7a1e3443a79ba
Cr-Commit-Position: refs/heads/master@{#12372}
==========

[commit-bot: I haz the power, 2016-04-15 08:24:29]

Patchset 17 (id:??) landed as
https://crrev.com/3911c26bc01fa4d5236c6d84b6d7a1e3443a79ba
Cr-Commit-Position: refs/heads/master@{#12372}