Expose Ivf logging through the native API

Expose Ivf logging through the native API

BUG=webrtc:6300
Committed: https://crrev.com/e75f204b06d597203c7eefd583aeb7820f32344b
Cr-Commit-Position: refs/heads/master@{#14419}

[palmkvist, 2016-09-02 13:35:22]

Description was changed from

==========
Expose Ivf logging through the native API
==========

to

==========
Expose Ivf logging through the native API

BUG=webrtc:6300
==========

[palmkvist, 2016-09-02 13:35:54]

palmkvist@webrtc.org changed reviewers:
+ sprang@webrtc.org, stefan@webrtc.org

[sprang_webrtc, 2016-09-04 14:48:48]

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

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:44: // name (or get one)
Probably enough to add one of these TODO's at the top of the file.

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/ivf_file_writer_unittest.cc (right):

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/ivf_file_writer_unittest.cc:83:
ASSERT_EQ(static_cast<unsigned int>(kHeaderSize),
prefer uint32_t to unsigned int

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3760: #endif
Can we add this as a static delete method to File instead?

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3777: #endif
Maybe even add this to File itself?

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3784: video_send_stream_->SetLogFiles();
use brackets for if/else

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3837: EXPECT_LT(0u, receive_file.Read(out,
100));
Please add some comments about the intentions here.

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3843: EndToEndLogTest* fixture_;
EndToEndLogTest* const fixture_;

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3847: int recorded_frames_ GUARDED_BY(crit_) =
0;
prefer setting values in the initializer list

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:405: RequestKeyFrame();
Add comment why this is done

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:91: rtc::PlatformFile file =
rtc::kInvalidPlatformFileValue) override;
I think this needs a more descriptive name.
Something like EnableEncodedFrameRecording() ?

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:114: rtc::CriticalSection ivf_writer_crit_;
I personally prefer lock as name, rather than crit, but up to you.

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:115: std::unique_ptr<IvfFileWriter>
ivf_writer_;
add GUARDED_BY

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:88: 
Again, more descriptive name please.
Also need to comment on why there are multiple files.
Default values are unfortunately not allowed according to the style guide.
Maybe a vector of platform files could be used instead? If so comment on max
length (there is a "max simulcast streams" constant somewhere)

[palmkvist, 2016-09-05 11:57:37]

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3777: #endif
On 2016/09/04 14:48:47, språng wrote:
> Maybe even add this to File itself?

Thinking about it, platform_file seems like a more reasonable place to put it.
Adding it there, unless someone complains.

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:88: 
On 2016/09/04 14:48:48, språng wrote:
> Again, more descriptive name please.
> Also need to comment on why there are multiple files.
> Default values are unfortunately not allowed according to the style guide.
> Maybe a vector of platform files could be used instead? If so comment on max
> length (there is a "max simulcast streams" constant somewhere)

Does this seem reasonable?
// informative comments + streams with no corresponding file will not be logged
EnableEncodedFrameRecording(vector)
...and then in the actual interface put a helper
DisableEncodedFrameRecording()
that just calls the former method with a length 0 vector.

Threading two calls through the abstractions inbetween seems excessive, given
that one method necessarily has to support the behaviour of the other. At the
same time having an explicit Disable method makes things clearer, so I figure
this is a reasonable compromise.

[sprang_webrtc, 2016-09-05 12:23:53]

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2303273002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:88: 
On 2016/09/05 11:57:37, palmkvist wrote:
> On 2016/09/04 14:48:48, språng wrote:
> > Again, more descriptive name please.
> > Also need to comment on why there are multiple files.
> > Default values are unfortunately not allowed according to the style guide.
> > Maybe a vector of platform files could be used instead? If so comment on max
> > length (there is a "max simulcast streams" constant somewhere)
> 
> Does this seem reasonable?
> // informative comments + streams with no corresponding file will not be
logged
> EnableEncodedFrameRecording(vector)
> ...and then in the actual interface put a helper
> DisableEncodedFrameRecording()
> that just calls the former method with a length 0 vector.
> 
> Threading two calls through the abstractions inbetween seems excessive, given
> that one method necessarily has to support the behaviour of the other. At the
> same time having an explicit Disable method makes things clearer, so I figure
> this is a reasonable compromise.

sgtm

[palmkvist, 2016-09-12 10:25:40]

This CL now exposes the logging functionality, as well as using said logging in
video_loopback and screenshare_loopback.

Comments?

[stefan-webrtc, 2016-09-13 10:53:18]

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:183: size_t byte_limit) {
Why does this take a byte_limit?

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:184: for (auto it = files.begin(); it <
files.end(); ++it) {
Something like this instead:
for (rtc::PlatformFile* file : files) 
  rtc::ClosePlatformFile(*file);

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3763: #endif
Don't we have platform independent wrappers for this in webrtc/base/ or similar?
I don't like having this here.

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1329: #endif
Break out to platform independent function or class.

[palmkvist, 2016-09-13 11:59:37]

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:183: size_t byte_limit) {
On 2016/09/13 10:53:17, stefan-webrtc (holmer) wrote:
> Why does this take a byte_limit?

It's not used in the fake streams, but in the regular streams it's there to make
sure we don't create massive logs in case logging is left on.

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:184: for (auto it = files.begin(); it <
files.end(); ++it) {
On 2016/09/13 10:53:17, stefan-webrtc (holmer) wrote:
> Something like this instead:
> for (rtc::PlatformFile* file : files) 
>   rtc::ClosePlatformFile(*file);

Done.

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3763: #endif
On 2016/09/13 10:53:17, stefan-webrtc (holmer) wrote:
> Don't we have platform independent wrappers for this in webrtc/base/ or
similar?
> I don't like having this here.

Nothing that is approved. The idea is to put it in platform_file, but I figured
I'd put the changes to base in a separate cl.

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2303273002/diff/120001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1329: #endif
On 2016/09/13 10:53:17, stefan-webrtc (holmer) wrote:
> Break out to platform independent function or class.

Also intended to be put in base, but in a separate CL.

[sprang_webrtc, 2016-09-15 08:59:31]

Looks good, but I'd actually prefer if you could move the platform specific
parts out before landing this. I think we can fast-track that.

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

https://codereview.webrtc.org/2303273002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/ivf_file_writer.cc:175: bytes_written_ +
kFrameHeaderSize + encoded_image._length > byte_limit_) {
Perhaps add a log line about this?

[sprang_webrtc, 2016-09-16 12:32:00]

lgtm

[sprang_webrtc, 2016-09-21 11:03:08]

ping stefan?

[stefan-webrtc, 2016-09-28 09:15:57]

lgtm % nit

https://codereview.webrtc.org/2303273002/diff/160001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2303273002/diff/160001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:184: for (rtc::PlatformFile file : files)
{
Remove {}

[palmkvist, 2016-09-28 11:32:10]

The CQ bit was checked by palmkvist@webrtc.org

[palmkvist, 2016-09-28 11:32:11]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2303273002/#ps180001 (title:
"Nit")

[commit-bot: I haz the power, 2016-09-28 11:32:15]

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-28 13:19:52]

Description was changed from

==========
Expose Ivf logging through the native API

BUG=webrtc:6300
==========

to

==========
Expose Ivf logging through the native API

BUG=webrtc:6300
==========

[commit-bot: I haz the power, 2016-09-28 13:19:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-28 13:19:57]

Description was changed from

==========
Expose Ivf logging through the native API

BUG=webrtc:6300
==========

to

==========
Expose Ivf logging through the native API

BUG=webrtc:6300

Committed: https://crrev.com/e75f204b06d597203c7eefd583aeb7820f32344b
Cr-Commit-Position: refs/heads/master@{#14419}
==========

[commit-bot: I haz the power, 2016-09-28 13:19:58]

Patchset 10 (id:??) landed as
https://crrev.com/e75f204b06d597203c7eefd583aeb7820f32344b
Cr-Commit-Position: refs/heads/master@{#14419}