Standalone denoiser (off by default).

Standalone denoiser (off by default).

BUG=webrtc:5255
Committed: https://crrev.com/8f9902a0ff2c4673b3e470026017e858f2992838
Cr-Commit-Position: refs/heads/master@{#10800}

[jackychen, 2015-11-20 22:35:44]

Description was changed from

==========
Standalone denoiser (off by default).

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

to

==========
Standalone denoiser (off by default).

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

[jackychen, 2015-11-20 22:35:44]

jackychen@webrtc.org changed reviewers:
+ marpan@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2015-11-20 22:39:37]

take a look at the previous cl for comments. if possible it would be good if the
answers were posted in the new cl... :)

[jackychen, 2015-11-21 00:14:26]

On 2015/11/20 22:39:37, stefan-webrtc (holmer) wrote:
> take a look at the previous cl for comments. if possible it would be good if
the
> answers were posted in the new cl... :)

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/main/source/frame_preprocessor.h (right):

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/frame_preprocessor.h:74:
VPMVideoDenoiser* vdn_;
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> scoped_ptr

Just keep the style here, we can refactor the whole frame_preprocessor in
another cl.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/frame_preprocessor.h:74:
VPMVideoDenoiser* vdn_;
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> I think we should stop using the VPM prefix, so please remove that. And call
> this member denoiser_ instead.

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/frame_preprocessor.h:76: bool
enable_denoise_;
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> Can we remove this and instead only create the video denoiser if it is
enabled?

Just keep the style here and need to refactor the whole file later.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/main/source/video_denoiser.cc (right):

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:17: filter_ =
(scoped_ptr<DenoiserFilter>)DenoiserFilter::Create();
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> filter_.reset(DenoiserFilter::Create());

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:18: dm_ = NULL;
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> Both of these members can be initialized in the initializer list instead.

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:35: // If the
number of denoised neighbor is less than a threshold,
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> neighbors?

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:40: if
(dm_[mb_index].denoise &&
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> Maybe comment on why fewer neighbors are compared here than for the non-skin
> case?

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:57:
filter_->CopyMem16x16(mb_src, stride_y, mb_dst, stride_y);
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> Shouldn't this set dm_[mb_index].denoise = 0?

Not doing this to avoid denoising status propagate as comment says.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:63: int32_t
VPMVideoDenoiser::VideoDenoise(const VideoFrame& frame,
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> This method can return void instead.

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:63: int32_t
VPMVideoDenoiser::VideoDenoise(const VideoFrame& frame,
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> This method can return void instead.

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:91: // Temporal
buffer to store denoising result.
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> Temporal -> temporary? 

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:94: dm_ = new
DenoiseMetrics[mb_cols * mb_rows];
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> Seems like the delete above is wrong since it doesn't have [].

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.cc:125: }
On 2015/11/20 22:30:47, stefan-webrtc (holmer) wrote:
> If I understand this correctly, we may copy a macroblock here, and then do
> another copy again in TrailingReduction() after this. Wouldn't it be better to
> just mark macroblocks as "denoise" or not here here, and then do all the
copying
> once at the end based on "denoise"?

If y_src block is copied here, it will not be copied again in TrailingReduction.
But if y_tmp is copied here, then y_src might be copied in TrailingReduction to
avoid improper denoise.
It doesn't happen often and I don't want to keep y_tmp to TrailingReduction,
because it will use another frame buffer.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/main/source/video_denoiser.h (right):

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.h:17: using
rtc::scoped_ptr;
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> Just use rtc::scoped_ptr below instead.

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.h:21: class
VPMVideoDenoiser {
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> VideoDenoiser

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.h:29: int height_;
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> I think we are using size_t for width/height

In video_denoiser.cc, there is a compare width_ != frame.width() where
frame.width() returns a int, so I use int to avoid cast.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.h:30: DenoiseMetrics*
dm_;
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> scoped_ptr

I thought it should be scoped_array, but there is no rtc::scoped_array.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.h:30: DenoiseMetrics*
dm_;
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> Maybe simply call it metrics_?

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/source/video_denoiser.h:32: void
TrailingReduction(int mb_rows, int mb_cols, const uint8_t* y_src,
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> Move this method to directly under private:

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/main/util/denoiser_filter.h (right):

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/util/denoiser_filter.h:22: #define
SUM_DIFF_THRESHOLD_HIGH (600)
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> Make these regular constants instead, e.g., extern int
kMotionMagnitudeThreshold
> if it needs to be accessed in multiple files.

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/util/denoiser_filter.h:23: enum
denoiser_decision { COPY_BLOCK, FILTER_BLOCK };
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> DenoiserDecision, or move it to the public section of DenoiserFilter and call
it
> Decision instead?

Done.

https://codereview.webrtc.org/1411563006/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/main/util/denoiser_filter.h:37: virtual int32_t
MbDenoise(uint8_t* mc_running_avg_y,
On 2015/11/20 22:30:48, stefan-webrtc (holmer) wrote:
> DenoiseMb
> 
> I think this should return DenoiserDecision?

Done.

[stefan-webrtc, 2015-11-23 10:12:53]

I think we should add some unittests for VideoDenoiser.

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:121:
vdn_->DenoiseFrame(frame, &denoised_frame_);
What if we have resampled the frame above? Then I'd assume we denoise
*processed_frame  instead?

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:133: content_metrics_ =
ca_->ComputeContentMetrics(resampled_frame_);
Should this use *processed_frame instead?

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:74: VideoDenoiser* vdn_;
I would prefer to call this video_denoiser_ instead.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:76: bool enable_denoise_;
enable_denoising_

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter.cc (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:123: return *sse -
(((int64_t)sum * sum) >> 8);
static_cast<int64_t>

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter.h (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.h:28: uint8_t denoise;
bool

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter_neon.cc (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_neon.cc:61: *sse =
(unsigned int)HorizontalAddS32x4(vaddq_s32(v_sse_lo, v_sse_hi));
static_cast

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_neon.cc:98: return *sse -
(((int64_t)sum * sum) >> 7);
static_cast

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter_sse2.cc (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_sse2.cc:132: return *sse -
(((unsigned int)sum * sum) >> 7);
static_cast

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/skin_detection.h (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/skin_detection.h:17: bool
video_skin_pixel(const uint8_t* y_src,
VideoSkinPixel or maybe PixelHasSkinColor?

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/skin_detection.h:25: int
evaluate_skin_color_difference(int cb, int cr);
EvaluateSkinColorDifference

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/skin_detection.h:25: int
evaluate_skin_color_difference(int cb, int cr);
Is this function used outside of skin_detection.cc? Otherwise make it static
within the .cc file and remove from here.

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:17:
filter_.reset(DenoiserFilter::Create());
Assign in the initializer list instead.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:102: // Denoise each MB at the
very start and save the result to a temporal
temporary buffer

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.h:30: DenoiseMetrics* metrics_;
I do believe scoped_ptr<DenoiseMetrics[]> metrics_; should work

[jackychen, 2015-11-23 19:45:00]

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:121:
vdn_->DenoiseFrame(frame, &denoised_frame_);
On 2015/11/23 10:12:52, stefan-webrtc (holmer) wrote:
> What if we have resampled the frame above? Then I'd assume we denoise
> *processed_frame  instead?

I reordered the code, so that resample will be working on the denoised frame. I
think this might be better.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:133: content_metrics_ =
ca_->ComputeContentMetrics(resampled_frame_);
On 2015/11/23 10:12:52, stefan-webrtc (holmer) wrote:
> Should this use *processed_frame instead?

Maybe Marco can change it in another cl, but for my part, resampled_frame_ and
*processed_frame should be the same.

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:74: VideoDenoiser* vdn_;
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> I would prefer to call this video_denoiser_ instead.

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:76: bool enable_denoise_;
On 2015/11/23 10:12:52, stefan-webrtc (holmer) wrote:
> enable_denoising_

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter.cc (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:123: return *sse -
(((int64_t)sum * sum) >> 8);
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> static_cast<int64_t>

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter.h (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.h:28: uint8_t denoise;
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> bool

I use this as integer to conveniently count the total number of denoised
neighboring blocks like following:
"metrics_[mb_index + 1].denoise +
 metrics_[mb_index - 1].denoise +
 metrics_[mb_index + mb_cols].denoise +
 metrics_[mb_index - mb_cols].denoise <= 2"

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter_neon.cc (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_neon.cc:61: *sse =
(unsigned int)HorizontalAddS32x4(vaddq_s32(v_sse_lo, v_sse_hi));
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> static_cast

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_neon.cc:98: return *sse -
(((int64_t)sum * sum) >> 7);
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> static_cast

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter_sse2.cc (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_sse2.cc:132: return *sse -
(((unsigned int)sum * sum) >> 7);
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> static_cast

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/skin_detection.h (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/skin_detection.h:17: bool
video_skin_pixel(const uint8_t* y_src,
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> VideoSkinPixel or maybe PixelHasSkinColor?

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/skin_detection.h:25: int
evaluate_skin_color_difference(int cb, int cr);
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> EvaluateSkinColorDifference

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/skin_detection.h:25: int
evaluate_skin_color_difference(int cb, int cr);
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> EvaluateSkinColorDifference

Done.

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:17:
filter_.reset(DenoiserFilter::Create());
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> Assign in the initializer list instead.

Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:102: // Denoise each MB at the
very start and save the result to a temporal
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> temporary buffer

Done.

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.h:30: DenoiseMetrics* metrics_;
On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> I do believe scoped_ptr<DenoiseMetrics[]> metrics_; should work

Done.

[stefan-webrtc, 2015-11-24 08:56:18]

This is starting to look good to me, besides no unittest. I agreed to let jacky
submit a unittest in a separate CL as long as this isn't enabled.

Magnus, do you have any comments?

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:133: content_metrics_ =
ca_->ComputeContentMetrics(resampled_frame_);
On 2015/11/23 19:44:59, jackychen wrote:
> On 2015/11/23 10:12:52, stefan-webrtc (holmer) wrote:
> > Should this use *processed_frame instead?
> 
> Maybe Marco can change it in another cl, but for my part, resampled_frame_ and
> *processed_frame should be the same.

But if denoising is on, but no resampling is done, then *processed_frame will be
pointing at &denoised_frame_, but the metrics will be computed on the
uninitialized resampled_frame_. That must be wrong, or am I missing something?

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter.h (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.h:28: uint8_t denoise;
On 2015/11/23 19:44:59, jackychen wrote:
> On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> > bool
> 
> I use this as integer to conveniently count the total number of denoised
> neighboring blocks like following:
> "metrics_[mb_index + 1].denoise +
>  metrics_[mb_index - 1].denoise +
>  metrics_[mb_index + mb_cols].denoise +
>  metrics_[mb_index - mb_cols].denoise <= 2"

Ok. I think you can make it int at least

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

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:121: if
(enable_denoising_)
{}

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:124: else
{}

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter_neon.cc (right):

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_neon.cc:61: *sse =
static_cast<unsigned int>(
Shouldn't this cast to uint32_t?

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_neon.cc:97: int sum = 0;
Make this an int64_t and remove the cast below.

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter_sse2.cc (right):

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_sse2.cc:50: *sum =
static_cast<int16_t>(_mm_extract_epi16(vsum, 0));
Shouldn't this cast to an int since that's the type of *sum?

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_sse2.cc:130: int sum = 0;
Make this unsigned int and remove the cast below.

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/skin_detection.h (right):

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/skin_detection.h:17: bool
MBHasSkinColor(const uint8_t* y_src,
MB->Mb

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

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:91: metrics_.reset(new
DenoiseMetrics[mb_cols * mb_rows]);
Move this to right after mb_cols and mb_rows are set.

[jackychen, 2015-11-24 22:33:06]

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

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:133: content_metrics_ =
ca_->ComputeContentMetrics(resampled_frame_);
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> On 2015/11/23 19:44:59, jackychen wrote:
> > On 2015/11/23 10:12:52, stefan-webrtc (holmer) wrote:
> > > Should this use *processed_frame instead?
> > 
> > Maybe Marco can change it in another cl, but for my part, resampled_frame_
and
> > *processed_frame should be the same.
> 
> But if denoising is on, but no resampling is done, then *processed_frame will
be
> pointing at &denoised_frame_, but the metrics will be computed on the
> uninitialized resampled_frame_. That must be wrong, or am I missing something?

You are right. Done.

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter.h (right):

https://codereview.webrtc.org/1466763002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.h:28: uint8_t denoise;
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> On 2015/11/23 19:44:59, jackychen wrote:
> > On 2015/11/23 10:12:53, stefan-webrtc (holmer) wrote:
> > > bool
> > 
> > I use this as integer to conveniently count the total number of denoised
> > neighboring blocks like following:
> > "metrics_[mb_index + 1].denoise +
> >  metrics_[mb_index - 1].denoise +
> >  metrics_[mb_index + mb_cols].denoise +
> >  metrics_[mb_index - mb_cols].denoise <= 2"
> 
> Ok. I think you can make it int at least

Acknowledged.

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

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:121: if
(enable_denoising_)
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> {}

Done.

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:124: else
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> {}

Done.

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter_neon.cc (right):

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_neon.cc:61: *sse =
static_cast<unsigned int>(
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> Shouldn't this cast to uint32_t?

Done.

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_neon.cc:97: int sum = 0;
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> Make this an int64_t and remove the cast below.

Done.

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter_sse2.cc (right):

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_sse2.cc:50: *sum =
static_cast<int16_t>(_mm_extract_epi16(vsum, 0));
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> Shouldn't this cast to an int since that's the type of *sum?

Done.

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_sse2.cc:130: int sum = 0;
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> Make this unsigned int and remove the cast below.

Done.

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/skin_detection.h (right):

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/skin_detection.h:17: bool
MBHasSkinColor(const uint8_t* y_src,
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> MB->Mb

Done.

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

https://codereview.webrtc.org/1466763002/diff/40001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:91: metrics_.reset(new
DenoiseMetrics[mb_cols * mb_rows]);
On 2015/11/24 08:56:18, stefan-webrtc (holmer) wrote:
> Move this to right after mb_cols and mb_rows are set.

Done.

[stefan-webrtc, 2015-11-25 10:12:59]

lgtm, but please follow up with a unittest, and wait for lg from mflodman and
marpan.

[mflodman, 2015-11-25 12:15:19]

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

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:114: if
(enable_denoising_) {
I'd prefer to skip this bool and instead check if (video_denoiser_ != nullptr)
instead. Then only create video_denoiser_ when it's actually going to be used.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:121: if
(enable_denoising_) {
I think we can avoid this check by refactor this method, but that can be done as
a follow up since this is the way it's been done previously.

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

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:69: VideoFrame
denoised_frame_;
Already from earlier, but we should have #include "webrtc/video_frame.h" in this
file.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:69: VideoFrame
denoised_frame_;
And I'd prefer to call this just 'denoiser_', since it's already implied this
code is for video.

I know you said video_denoiser_ Stefan, but WDYT?

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter.cc (right):

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:53: #if
defined(__SSE2__)
Can't we skip this? I.e. creating the denoiser is only done once and if in use,
if we change as I propose in my other comment, and it shouldn't be that
expensive, so can't we just skip this step and always run
'WebRtc_GetCPUInfo(kSSE2)'?

That would simplify the code.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:67: new
DenoiserFilterNEON();
And same goes for neon detection.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:81: void
DenoiserFilterC::CopyMem16x16(const uint8_t* src,
I'd prefer to put 'DenoiserFilterC' in its own file, in the same way as it's
done for the other versions. This would make this code cleaner, right now we mix
class declaration, Create() and class definition.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/video_denoiser.cc (right):

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:19:
filter_((rtc::scoped_ptr<DenoiserFilter>)DenoiserFilter::Create()) {}
'(rtc::scoped_ptr<DenoiserFilter>)' shouldn't be needed here, right?

[stefan-webrtc, 2015-11-25 12:16:53]

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

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:69: VideoFrame
denoised_frame_;
On 2015/11/25 12:15:19, mflodman wrote:
> And I'd prefer to call this just 'denoiser_', since it's already implied this
> code is for video.
> 
> I know you said video_denoiser_ Stefan, but WDYT?

Sounds good to me.

[mflodman, 2015-11-25 14:06:46]

Description was changed from

==========
Standalone denoiser (off by default).

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

to

==========
Standalone denoiser (off by default).

BUG=webrtc:5255
==========

[mflodman, 2015-11-25 14:07:03]

Description was changed from

==========
Standalone denoiser (off by default).

BUG=webrtc:5255
==========

to

==========
Standalone denoiser (off by default).

BUG=5255
==========

[mflodman, 2015-11-25 14:07:26]

Description was changed from

==========
Standalone denoiser (off by default).

BUG=5255
==========

to

==========
Standalone denoiser (off by default).

BUG=webrtc:5255
==========

[jackychen, 2015-11-25 19:59:42]

Patchset #5 (id:80001) has been deleted

[jackychen, 2015-11-25 20:00:18]

Patchset #5 (id:100001) has been deleted

[jackychen, 2015-11-25 20:29:05]

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

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:114: if
(enable_denoising_) {
On 2015/11/25 12:15:19, mflodman wrote:
> I'd prefer to skip this bool and instead check if (video_denoiser_ != nullptr)
> instead. Then only create video_denoiser_ when it's actually going to be used.

Done.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:121: if
(enable_denoising_) {
On 2015/11/25 12:15:19, mflodman wrote:
> I think we can avoid this check by refactor this method, but that can be done
as
> a follow up since this is the way it's been done previously.

OK. Will do in a follow up cl.

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

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:69: VideoFrame
denoised_frame_;
On 2015/11/25 12:15:19, mflodman wrote:
> And I'd prefer to call this just 'denoiser_', since it's already implied this
> code is for video.
> 
> I know you said video_denoiser_ Stefan, but WDYT?

Done.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.h:69: VideoFrame
denoised_frame_;
On 2015/11/25 12:15:19, mflodman wrote:
> Already from earlier, but we should have #include "webrtc/video_frame.h" in
this
> file.

Done.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/util/denoiser_filter.cc (right):

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:53: #if
defined(__SSE2__)
On 2015/11/25 12:15:19, mflodman wrote:
> Can't we skip this? I.e. creating the denoiser is only done once and if in
use,
> if we change as I propose in my other comment, and it shouldn't be that
> expensive, so can't we just skip this step and always run
> 'WebRtc_GetCPUInfo(kSSE2)'?
> 
> That would simplify the code.

Done.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:67: new
DenoiserFilterNEON();
On 2015/11/25 12:15:19, mflodman wrote:
> And same goes for neon detection.

Done.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:81: void
DenoiserFilterC::CopyMem16x16(const uint8_t* src,
On 2015/11/25 12:15:19, mflodman wrote:
> I'd prefer to put 'DenoiserFilterC' in its own file, in the same way as it's
> done for the other versions. This would make this code cleaner, right now we
mix
> class declaration, Create() and class definition.

Done.

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/video_denoiser.cc (right):

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:19:
filter_((rtc::scoped_ptr<DenoiserFilter>)DenoiserFilter::Create()) {}
On 2015/11/25 12:15:19, mflodman wrote:
> '(rtc::scoped_ptr<DenoiserFilter>)' shouldn't be needed here, right?

Done.

[mflodman, 2015-11-26 10:50:00]

Nive, LGTM!

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

https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/frame_preprocessor.cc:121: if
(enable_denoising_) {
On 2015/11/25 20:29:05, jackychen wrote:
> On 2015/11/25 12:15:19, mflodman wrote:
> > I think we can avoid this check by refactor this method, but that can be
done
> as
> > a follow up since this is the way it's been done previously.
> 
> OK. Will do in a follow up cl.

I filed a bug to keep track of the coming clean-up work and assigned to me, so
I'll keep that as a side project the coming weeks (/months). I'll start when
this CL has been submitted to not interfere with this work.

https://bugs.chromium.org/p/webrtc/issues/detail?id=5259

https://codereview.webrtc.org/1466763002/diff/120001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/frame_preprocessor.cc (right):

https://codereview.webrtc.org/1466763002/diff/120001/webrtc/modules/video_pro...
webrtc/modules/video_processing/frame_preprocessor.cc:24: if (enable_denoising_)
{
This can later be changed to only be an input parameter to the constructor, but
we can leave it as is for now.

https://codereview.webrtc.org/1466763002/diff/120001/webrtc/modules/video_pro...
webrtc/modules/video_processing/frame_preprocessor.cc:125: if
(enable_denoising_) {
Change to denoiser != nullptr here too.

[mflodman, 2015-11-26 10:50:15]

On 2015/11/26 10:50:00, mflodman wrote:
> Nive, LGTM!
> 
>
https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
> File webrtc/modules/video_processing/frame_preprocessor.cc (right):
> 
>
https://codereview.webrtc.org/1466763002/diff/60001/webrtc/modules/video_proc...
> webrtc/modules/video_processing/frame_preprocessor.cc:121: if
> (enable_denoising_) {
> On 2015/11/25 20:29:05, jackychen wrote:
> > On 2015/11/25 12:15:19, mflodman wrote:
> > > I think we can avoid this check by refactor this method, but that can be
> done
> > as
> > > a follow up since this is the way it's been done previously.
> > 
> > OK. Will do in a follow up cl.
> 
> I filed a bug to keep track of the coming clean-up work and assigned to me, so
> I'll keep that as a side project the coming weeks (/months). I'll start when
> this CL has been submitted to not interfere with this work.
> 
> https://bugs.chromium.org/p/webrtc/issues/detail?id=5259
> 
>
https://codereview.webrtc.org/1466763002/diff/120001/webrtc/modules/video_pro...
> File webrtc/modules/video_processing/frame_preprocessor.cc (right):
> 
>
https://codereview.webrtc.org/1466763002/diff/120001/webrtc/modules/video_pro...
> webrtc/modules/video_processing/frame_preprocessor.cc:24: if
(enable_denoising_)
> {
> This can later be changed to only be an input parameter to the constructor,
but
> we can leave it as is for now.
> 
>
https://codereview.webrtc.org/1466763002/diff/120001/webrtc/modules/video_pro...
> webrtc/modules/video_processing/frame_preprocessor.cc:125: if
> (enable_denoising_) {
> Change to denoiser != nullptr here too.

And Nive = Nice :/

[mflodman, 2015-11-26 10:50:26]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2015-11-26 10:50:26]

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

[commit-bot: I haz the power, 2015-11-26 10:50:37]

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

[commit-bot: I haz the power, 2015-11-26 10:59:51]

Description was changed from

==========
Standalone denoiser (off by default).

BUG=webrtc:5255
==========

to

==========
Standalone denoiser (off by default).

BUG=webrtc:5255
==========

[commit-bot: I haz the power, 2015-11-26 10:59:51]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-11-26 10:59:57]

Description was changed from

==========
Standalone denoiser (off by default).

BUG=webrtc:5255
==========

to

==========
Standalone denoiser (off by default).

BUG=webrtc:5255

Committed: https://crrev.com/8f9902a0ff2c4673b3e470026017e858f2992838
Cr-Commit-Position: refs/heads/master@{#10800}
==========

[commit-bot: I haz the power, 2015-11-26 10:59:58]

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

[kjellander_webrtc, 2015-11-26 16:28:35]

kjellander@webrtc.org changed reviewers:
+ kjellander@webrtc.org, magjed@webrtc.org, niklas.enbom@webrtc.org

[kjellander_webrtc, 2015-11-26 16:28:36]

Adding our WebRTC-in-Chrome sheriffs.
I think I know why this breaks the Android GN FYI bot, see comment below.

https://codereview.webrtc.org/1466763002/diff/120001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/BUILD.gn (right):

https://codereview.webrtc.org/1466763002/diff/120001/webrtc/modules/video_pro...
webrtc/modules/video_processing/BUILD.gn:100: }
Our FYI bots used for rolling are failing the Android GN build due to this:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20GN/builds...

I think it's because the include paths are not properly setup for the
build_in_chromium=True case. 
I believe you can solve that by adding:
    configs += [ "..:common_config" ]
    public_configs = [ "..:common_inherited_config" ]
similar to the common_audio_neon target in
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/common_audi...
but to be sure you need build from a Chromium checkout with the change applied.