Initial VideoProcessing refactoring.

Initial VideoProcessing refactoring.

This CL is the first in a series of CLs to refactor
VideoProcessing(Module) to follow Google C++ style guide and make the
code more readable.

This CL removed inheritance from Module, renames variables and makes
VideoProcessingImpl::PreprocessFrame return a frame pointer if there
is a frame to send, nullptr otherwise. The affected CLs also passes git
cl lint.

BUG=webrtc:5259
Committed: https://crrev.com/a8565425bc6b1d3d7b5a0b0e0a49bc4e57c64f21
Cr-Commit-Position: refs/heads/master@{#10907}

[mflodman, 2015-11-30 10:31:19]

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

[mflodman, 2015-11-30 10:31:20]

Peter,
The video processing changes we discussed Friday. It grew a bit large when
removing Module from the name and making it lint warning free.

[pbos-webrtc, 2015-11-30 11:51:58]

I understand if you want to do some of these in a follow-up but I think they'd
be good. PTAL

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/frame_preprocessor.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:58: if ((width == 0) ||
(height == 0) || (frame_rate == 0)) {
remove extra parenthesis on every expression

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:113: if (denoiser_.get())
{
if (denoiser_)

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:118: if
(spatial_resampler_->ApplyResample(frame.width(), frame.height()))  {
current_frame (imo since we're working on current_frame, even though this
shouldn't have changed)

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/frame_preprocessor.h (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:68: // All pointers/members
in this class are assumed to be protected by the class
Maybe put a note on the class itself rather than here saying that it's not
thread safe and assumed to be protected from the outside?

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/include/video_processing.h (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/include/video_processing.h:38: struct FrameStats
{
Q: Should these members even be initialized? Isn't it enough that GetFrameStats
initializes it? This might hide msan errors, that we're reading it before we're
actually initializing it.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/include/video_processing.h:43: uint32_t
hist[256];  // Frame histogram.
I think you can do = {}; ?

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/include/video_processing.h:65: static bool
ValidFrameStats(const FrameStats& stats);
Do we really need this? :\

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/include/video_processing.h:67: static void
ClearFrameStats(FrameStats* stats);
Same here? If we need it (I don't see why), we should do stats = FrameStats()
instead?

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/brightness_detection_test.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/brightness_detection_test.cc:25:
EXPECT_EQ(0, ConvertToI420(kI420, video_buffer.get(), 0, 0, width_,
If you wanna, can you clang format the entire file? This indentation is off and
the total file diff won't be huge.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/content_metrics_test.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/content_metrics_test.cc:30: ==
frame_length_) {
clang-format the entire file :)

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/video_processing_unittest.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/video_processing_unittest.cc:81:
source_file_ = fopen(video_file.c_str(),  "rb");
double space, please run git cl format

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/video_processing_unittest.cc:92: if (vp_ !=
NULL)  {
Can you make this a scoped_ptr? Or otherwise always delete (null delete ==
no-op)

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/video_processing_impl.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_processing_impl.cc:44: int32_t
VideoProcessing::GetFrameStats(const VideoFrame& frame,
Make void.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_processing_impl.cc:46: if
(frame.IsZeroSize()) {
DCHECK here?

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/video_engine/vie_e...
File webrtc/video_engine/vie_encoder.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/video_engine/vie_e...
webrtc/video_engine/vie_encoder.cc:365: if (frame_to_send == nullptr) {
!frame_to_send pref

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/video_engine/vie_e...
webrtc/video_engine/vie_encoder.cc:375: copied_frame.CopyFrame(video_frame);
Base this on the preprocessed frame?

[mflodman, 2015-12-04 15:01:08]

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/frame_preprocessor.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:58: if ((width == 0) ||
(height == 0) || (frame_rate == 0)) {
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> remove extra parenthesis on every expression

That's a lot to change if changing everywhere here, so I'll follow up with that.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:113: if (denoiser_.get())
{
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> if (denoiser_)

Done.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:118: if
(spatial_resampler_->ApplyResample(frame.width(), frame.height()))  {
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> current_frame (imo since we're working on current_frame, even though this
> shouldn't have changed)

Done.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/frame_preprocessor.h (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:68: // All pointers/members
in this class are assumed to be protected by the class
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> Maybe put a note on the class itself rather than here saying that it's not
> thread safe and assumed to be protected from the outside?

Done.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/include/video_processing.h (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/include/video_processing.h:38: struct FrameStats
{
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> Q: Should these members even be initialized? Isn't it enough that
GetFrameStats
> initializes it? This might hide msan errors, that we're reading it before
we're
> actually initializing it.

Init removed.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/include/video_processing.h:65: static bool
ValidFrameStats(const FrameStats& stats);
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> Do we really need this? :\

Maybe not, but I'd prefer to go at this depth in a follow up rather than now,
this has gotten big enough.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/include/video_processing.h:67: static void
ClearFrameStats(FrameStats* stats);
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> Same here? If we need it (I don't see why), we should do stats = FrameStats()
> instead?

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/video_processing_unittest.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/video_processing_unittest.cc:92: if (vp_ !=
NULL)  {
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> Can you make this a scoped_ptr? Or otherwise always delete (null delete ==
> no-op)

Done.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/video_processing_impl.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_processing_impl.cc:44: int32_t
VideoProcessing::GetFrameStats(const VideoFrame& frame,
On 2015/11/30 11:51:58, pbos-webrtc wrote:
> Make void.

Done.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_processing_impl.cc:46: if
(frame.IsZeroSize()) {
On 2015/11/30 11:51:57, pbos-webrtc wrote:
> DCHECK here?

I don't know if that is ok, I'll follow up.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/video_engine/vie_e...
File webrtc/video_engine/vie_encoder.cc (right):

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/video_engine/vie_e...
webrtc/video_engine/vie_encoder.cc:365: if (frame_to_send == nullptr) {
On 2015/11/30 11:51:58, pbos-webrtc wrote:
> !frame_to_send pref

Done.

https://codereview.webrtc.org/1482913003/diff/20001/webrtc/video_engine/vie_e...
webrtc/video_engine/vie_encoder.cc:375: copied_frame.CopyFrame(video_frame);
On 2015/11/30 11:51:58, pbos-webrtc wrote:
> Base this on the preprocessed frame?

Done.

[pbos-webrtc, 2015-12-04 15:04:59]

lgtm with one question

https://codereview.webrtc.org/1482913003/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/video_processing_impl.cc (right):

https://codereview.webrtc.org/1482913003/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_processing_impl.cc:46:
ClearFrameStats(stats);  // The histogram needs to be zeroed out.
If this is only true when frame.IsZeroSize() move this into the if.

[mflodman, 2015-12-04 15:06:22]

https://codereview.webrtc.org/1482913003/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/video_processing_impl.cc (right):

https://codereview.webrtc.org/1482913003/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_processing_impl.cc:46:
ClearFrameStats(stats);  // The histogram needs to be zeroed out.
On 2015/12/04 15:04:59, pbos-webrtc wrote:
> If this is only true when frame.IsZeroSize() move this into the if.

We should zero out the histogram anyway.

[mflodman, 2015-12-07 08:06:05]

The CQ bit was checked by mflodman@webrtc.org

[commit-bot: I haz the power, 2015-12-07 08:06:10]

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

[commit-bot: I haz the power, 2015-12-07 09:09:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-07 09:10:10]

Description was changed from

==========
Initial VideoProcessing refactoring.

This CL is the first in a series of CLs to refactor
VideoProcessing(Module) to follow Google C++ style guide and make the
code more readable.

This CL removed inheritance from Module, renames variables and makes
VideoProcessingImpl::PreprocessFrame return a frame pointer if there
is a frame to send, nullptr otherwise. The affected CLs also passes git
cl lint.

BUG=webrtc:5259
==========

to

==========
Initial VideoProcessing refactoring.

This CL is the first in a series of CLs to refactor
VideoProcessing(Module) to follow Google C++ style guide and make the
code more readable.

This CL removed inheritance from Module, renames variables and makes
VideoProcessingImpl::PreprocessFrame return a frame pointer if there
is a frame to send, nullptr otherwise. The affected CLs also passes git
cl lint.

BUG=webrtc:5259

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

[commit-bot: I haz the power, 2015-12-07 09:10:10]

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