Add unit test for stand-alone denoiser and fixed some bugs.

Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

TBR=tommi@webrtc.org

BUG=webrtc:5255
Committed: https://crrev.com/67e94fb6f2a84cad97ba96abce3506c29f8ebb24
Cr-Commit-Position: refs/heads/master@{#11216}

[jackychen, 2015-12-03 00:40:01]

Description was changed from

==========
Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

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

to

==========
Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

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

[jackychen, 2015-12-03 00:40:01]

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

[stefan-webrtc, 2015-12-10 08:53:26]

Thanks for adding this!

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/denoiser_test.cc (right):

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:21: static int
CompareImage(const VideoFrame *const img1,
bool

Or maybe we can break out EqualFrames() in i420_video_frame_unittest.cc into a
new file in webrtc/test/ and use that function here too?

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:23: int match = 1;
bool

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:44:
TEST_F(VideoProcessingModuleTest, DISABLED_ON_IOS(Denoiser)) {
Comment on why we disable on iOS

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:51: denoiser_c.reset(new
VideoDenoiser(false));
You may be able to just create these denoisers on the stack, or?
VideoDenoiser denoiser_c(false);

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:32: new
DenoiserFilterSSE2();
Indentation, run git cl format.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.h:57: int
increase_denoising) = 0;
Would it be OK to request a few smaller unittests which test these methods
separately, just making sure that if we give some reasonable input (e.g. a a few
4x4 frames or similar), that the functions make the changes we expect them to? I
think that would be a great addition to the test you added.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_c.cc:59: return *sse -
((static_cast<int64_t>(sum) * sum) >> 7);
Was this a bug? Not entirely clear to me why it should be 7 and not 8. :)

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_sse2.cc:131:
VarianceSSE2(src, src_stride << 1, ref, ref_stride << 1, 16, 8, sse, &sum, 8);
Could you comment on these changes so I can follow why they were made?

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:82: metrics_.reset(new
DenoiseMetrics[mb_cols * mb_rows]());
Why was () added?

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.h:21: VideoDenoiser(bool
runtime_cpu_detection);
explicit

[jackychen, 2015-12-11 19:57:57]

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/denoiser_test.cc (right):

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:21: static int
CompareImage(const VideoFrame *const img1,
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> bool
> 
> Or maybe we can break out EqualFrames() in i420_video_frame_unittest.cc into a
> new file in webrtc/test/ and use that function here too?

I broke out EqualFrames() and put it in video_frame.cc, in modules.gyp added
dependency to common_video. Then I found one ugly issue, that in
video_capture_input_unittest.cc there is another function have the same name, I
don't want to touch that file, so I have to rename it to EqualFrame().
How about that?

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:23: int match = 1;
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> bool

Done.

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:44:
TEST_F(VideoProcessingModuleTest, DISABLED_ON_IOS(Denoiser)) {
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> Comment on why we disable on iOS

I don't know why, but in modules.gyp line 107, I saw
['OS!="ios" or target_arch=="arm" or target_arch=="arm64"',
And I found all other tests in the same folder with mine apply this.

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:51: denoiser_c.reset(new
VideoDenoiser(false));
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> You may be able to just create these denoisers on the stack, or?
> VideoDenoiser denoiser_c(false);

Done.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.cc:32: new
DenoiserFilterSSE2();
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> Indentation, run git cl format.

Done.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.h:57: int
increase_denoising) = 0;
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> Would it be OK to request a few smaller unittests which test these methods
> separately, just making sure that if we give some reasonable input (e.g. a a
few
> 4x4 frames or similar), that the functions make the changes we expect them to?
I
> think that would be a great addition to the test you added.

Do you mean we should check the input width and height before denoising?

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_c.cc:59: return *sse -
((static_cast<int64_t>(sum) * sum) >> 7);
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> Was this a bug? Not entirely clear to me why it should be 7 and not 8. :)

Yes. Previously, we used variance of 16x16, but we changed to 16x8 to speed up
on ARM neon, I forgot to change it here. Thanks to the unit test, we fixed it.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_sse2.cc:131:
VarianceSSE2(src, src_stride << 1, ref, ref_stride << 1, 16, 8, sse, &sum, 8);
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> Could you comment on these changes so I can follow why they were made?

This is a 16x8 variance instead of 16x16, so the stride should be doubled.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.cc:82: metrics_.reset(new
DenoiseMetrics[mb_cols * mb_rows]());
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> Why was () added?

To memset the buffer to 0 to pass msan checking, this only happen once, so the
speed will not be slow down.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.h:21: VideoDenoiser(bool
runtime_cpu_detection);
On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> explicit

Done.

[stefan-webrtc, 2015-12-15 21:00:30]

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/denoiser_test.cc (right):

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:21: static int
CompareImage(const VideoFrame *const img1,
On 2015/12/11 19:57:56, jackychen wrote:
> On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> > bool
> > 
> > Or maybe we can break out EqualFrames() in i420_video_frame_unittest.cc into
a
> > new file in webrtc/test/ and use that function here too?
> 
> I broke out EqualFrames() and put it in video_frame.cc, in modules.gyp added
> dependency to common_video. Then I found one ugly issue, that in
> video_capture_input_unittest.cc there is another function have the same name,
I
> don't want to touch that file, so I have to rename it to EqualFrame().
> How about that?

Maybe you could make it a method of the VideoFrame class instead?

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:44:
TEST_F(VideoProcessingModuleTest, DISABLED_ON_IOS(Denoiser)) {
On 2015/12/11 19:57:56, jackychen wrote:
> On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> > Comment on why we disable on iOS
> 
> I don't know why, but in modules.gyp line 107, I saw
> ['OS!="ios" or target_arch=="arm" or target_arch=="arm64"',
> And I found all other tests in the same folder with mine apply this.

Hm, ok. Could you try to do a tryjob without it? If it works it would be good to
also run on iOS, I think.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.h:57: int
increase_denoising) = 0;
On 2015/12/11 19:57:57, jackychen wrote:
> On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> > Would it be OK to request a few smaller unittests which test these methods
> > separately, just making sure that if we give some reasonable input (e.g. a a
> few
> > 4x4 frames or similar), that the functions make the changes we expect them
to?
> I
> > think that would be a great addition to the test you added.
> 
> Do you mean we should check the input width and height before denoising? 

I was thinking that you could have separate unittests which tests these methods.

For instance, test that the Variance16x8() method actually computes the correct
variance of a simple test block.

Similarly, verify that MbDenoise actually denoises a simple test block in the
way that you would expect given some known simple running_avg_y and
mc_running_avg_y inputs. It's good to test these methods separately with some
input based on which we can easily compute the expected output by hand.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter_c.cc:59: return *sse -
((static_cast<int64_t>(sum) * sum) >> 7);
On 2015/12/11 19:57:57, jackychen wrote:
> On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> > Was this a bug? Not entirely clear to me why it should be 7 and not 8. :)
> 
> Yes. Previously, we used variance of 16x16, but we changed to 16x8 to speed up
> on ARM neon, I forgot to change it here. Thanks to the unit test, we fixed it.

Great!

[jackychen, 2015-12-16 00:28:06]

Patchset #6 (id:100001) has been deleted

[jackychen, 2015-12-16 00:28:19]

Patchset #6 (id:120001) has been deleted

[jackychen, 2015-12-16 22:12:03]

Patchset #5 (id:80001) has been deleted

[jackychen, 2015-12-16 22:12:15]

Patchset #6 (id:160001) has been deleted

[jackychen, 2015-12-16 22:12:30]

Patchset #5 (id:140001) has been deleted

[jackychen, 2015-12-16 22:41:56]

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/denoiser_test.cc (right):

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:21: static int
CompareImage(const VideoFrame *const img1,
On 2015/12/15 21:00:30, stefan-webrtc (holmer) wrote:
> On 2015/12/11 19:57:56, jackychen wrote:
> > On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> > > bool
> > > 
> > > Or maybe we can break out EqualFrames() in i420_video_frame_unittest.cc
into
> a
> > > new file in webrtc/test/ and use that function here too?
> > 
> > I broke out EqualFrames() and put it in video_frame.cc, in modules.gyp added
> > dependency to common_video. Then I found one ugly issue, that in
> > video_capture_input_unittest.cc there is another function have the same
name,
> I
> > don't want to touch that file, so I have to rename it to EqualFrame().
> > How about that?
> 
> Maybe you could make it a method of the VideoFrame class instead?

Done.

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/denoiser_test.cc:44:
TEST_F(VideoProcessingModuleTest, DISABLED_ON_IOS(Denoiser)) {
On 2015/12/15 21:00:30, stefan-webrtc (holmer) wrote:
> On 2015/12/11 19:57:56, jackychen wrote:
> > On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> > > Comment on why we disable on iOS
> > 
> > I don't know why, but in modules.gyp line 107, I saw
> > ['OS!="ios" or target_arch=="arm" or target_arch=="arm64"',
> > And I found all other tests in the same folder with mine apply this.
> 
> Hm, ok. Could you try to do a tryjob without it? If it works it would be good
to
> also run on iOS, I think.

Seems OK to run on iOS. Done.

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

https://codereview.webrtc.org/1492053003/diff/60001/webrtc/modules/video_proc...
webrtc/modules/video_processing/util/denoiser_filter.h:57: int
increase_denoising) = 0;
On 2015/12/15 21:00:30, stefan-webrtc (holmer) wrote:
> On 2015/12/11 19:57:57, jackychen wrote:
> > On 2015/12/10 08:53:25, stefan-webrtc (holmer) wrote:
> > > Would it be OK to request a few smaller unittests which test these methods
> > > separately, just making sure that if we give some reasonable input (e.g. a
a
> > few
> > > 4x4 frames or similar), that the functions make the changes we expect them
> to?
> > I
> > > think that would be a great addition to the test you added.
> > 
> > Do you mean we should check the input width and height before denoising? 
> 
> I was thinking that you could have separate unittests which tests these
methods.
> 
> For instance, test that the Variance16x8() method actually computes the
correct
> variance of a simple test block.
> 
> Similarly, verify that MbDenoise actually denoises a simple test block in the
> way that you would expect given some known simple running_avg_y and
> mc_running_avg_y inputs. It's good to test these methods separately with some
> input based on which we can easily compute the expected output by hand.

Done. Add unit tests for all the functions.

[stefan-webrtc, 2015-12-17 10:07:47]

Two nits, otherwise I'm very happy with this!

lgtm

https://codereview.webrtc.org/1492053003/diff/180001/webrtc/common_video/vide...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/1492053003/diff/180001/webrtc/common_video/vide...
webrtc/common_video/video_frame.cc:228: bool VideoFrame::EqualFrames(const
VideoFrame& frame) const {
Rename to just Equals() or EqualsFrame()

https://codereview.webrtc.org/1492053003/diff/180001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/test/denoiser_test.cc (right):

https://codereview.webrtc.org/1492053003/diff/180001/webrtc/modules/video_pro...
webrtc/modules/video_processing/test/denoiser_test.cc:60: EXPECT_EQ(690848,
(int)df_sse_neon->Variance16x8(src, 16, dst, 16, &sse));
static_cast<int>, or 690848u.

Could you maybe even name the constant and write the expression to compute it?
const uint32_t kExpectedVariance = 16*....;

If the expression is easily computed, which I guess it is. :)

[jackychen, 2015-12-18 07:32:01]

https://codereview.webrtc.org/1492053003/diff/180001/webrtc/common_video/vide...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/1492053003/diff/180001/webrtc/common_video/vide...
webrtc/common_video/video_frame.cc:228: bool VideoFrame::EqualFrames(const
VideoFrame& frame) const {
On 2015/12/17 10:07:47, stefan-webrtc (holmer) wrote:
> Rename to just Equals() or EqualsFrame()

Done.

https://codereview.webrtc.org/1492053003/diff/180001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/test/denoiser_test.cc (right):

https://codereview.webrtc.org/1492053003/diff/180001/webrtc/modules/video_pro...
webrtc/modules/video_processing/test/denoiser_test.cc:60: EXPECT_EQ(690848,
(int)df_sse_neon->Variance16x8(src, 16, dst, 16, &sse));
On 2015/12/17 10:07:47, stefan-webrtc (holmer) wrote:
> static_cast<int>, or 690848u.
> 
> Could you maybe even name the constant and write the expression to compute it?
> const uint32_t kExpectedVariance = 16*....;
> 
> If the expression is easily computed, which I guess it is. :)

Done.

[jackychen_, 2016-01-11 10:09:43]

Description was changed from

==========
Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

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

to

==========
Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

TBR=tommi@webrtc.org

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

[jackychen_, 2016-01-11 10:27:05]

Patchset #7 (id:220001) has been deleted

[marpan, 2016-01-12 04:58:36]

The CQ bit was checked by marpan@google.com

[marpan, 2016-01-12 04:58:36]

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/1492053003/#ps240001 (title:
"remove gtest_disabled.h")

[commit-bot: I haz the power, 2016-01-12 04:58:49]

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

[commit-bot: I haz the power, 2016-01-12 05:34:10]

Description was changed from

==========
Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

TBR=tommi@webrtc.org

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

to

==========
Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

TBR=tommi@webrtc.org

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

[commit-bot: I haz the power, 2016-01-12 05:34:10]

Committed patchset #7 (id:240001)

[commit-bot: I haz the power, 2016-01-12 05:34:21]

Description was changed from

==========
Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

TBR=tommi@webrtc.org

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

to

==========
Add unit test for stand-alone denoiser and fixed some bugs.

The unit test will run the pure C denoiser and SSE2/NEON denoiser (based
on the CPU detection) and compare the denoised frames to ensure the bit
exact.

TBR=tommi@webrtc.org

BUG=webrtc:5255

Committed: https://crrev.com/67e94fb6f2a84cad97ba96abce3506c29f8ebb24
Cr-Commit-Position: refs/heads/master@{#11216}
==========

[commit-bot: I haz the power, 2016-01-12 05:34:21]

Patchset 7 (id:??) landed as
https://crrev.com/67e94fb6f2a84cad97ba96abce3506c29f8ebb24
Cr-Commit-Position: refs/heads/master@{#11216}

[tommi (sloooow) - chröme, 2016-01-12 12:01:20]

tommi@chromium.org changed reviewers:
+ tommi@chromium.org

[tommi (sloooow) - chröme, 2016-01-12 12:01:21]

Hi - I just noticed this change by coincidence and that I was TBRed on it.
Please add me to the list of reviewers when you do that. Otherwise I won't be
notified.

Some small requests below, otherwise lgtm.

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/common_video/vide...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/common_video/vide...
webrtc/common_video/video_frame.cc:39: } else {
nit: no need for else

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/common_video/vide...
webrtc/common_video/video_frame.cc:229: if ((this->width() != frame.width()) ||
(this->height() != frame.height()) ||
no need for extra parenthesis for each of the expressions and if could remove
use of |this->|, that'd be good too.

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/util/denoiser_filter.cc (right):

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
webrtc/modules/video_processing/util/denoiser_filter.cc:48: return filter;
nit: first RTC_DCHECK(filter)?

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/util/denoiser_filter.h (right):

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
webrtc/modules/video_processing/util/denoiser_filter.h:33: static
DenoiserFilter* Create(bool runtime_cpu_detection);
it would be good to change the return type in these cases to be
scoped_ptr<DenoiseFilter> to explicitly declare ownership passing and require
the caller to use scoped_ptr.

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/video_denoiser.cc (right):

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
webrtc/modules/video_processing/video_denoiser.cc:83: metrics_.reset(new
DenoiseMetrics[mb_cols * mb_rows]());
I'm curious about what this change does?

[jackychen_, 2016-01-13 03:07:49]

jackychen@google.com changed reviewers:
+ jackychen@google.com

[jackychen_, 2016-01-13 03:07:51]

Hi Tommi, since I can't log with my webrtc account now in HK, I fixed the issues
in https://codereview.webrtc.org/1578373003/ for convenience and added you as
reviewer.

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/common_video/vide...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/common_video/vide...
webrtc/common_video/video_frame.cc:39: } else {
On 2016/01/12 12:01:21, tommi-chromium wrote:
> nit: no need for else

Done.

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/common_video/vide...
webrtc/common_video/video_frame.cc:229: if ((this->width() != frame.width()) ||
(this->height() != frame.height()) ||
On 2016/01/12 12:01:21, tommi-chromium wrote:
> no need for extra parenthesis for each of the expressions and if could remove
> use of |this->|, that'd be good too.

Done.

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/util/denoiser_filter.cc (right):

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
webrtc/modules/video_processing/util/denoiser_filter.cc:48: return filter;
On 2016/01/12 12:01:21, tommi-chromium wrote:
> nit: first RTC_DCHECK(filter)?

Done.

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/util/denoiser_filter.h (right):

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
webrtc/modules/video_processing/util/denoiser_filter.h:33: static
DenoiserFilter* Create(bool runtime_cpu_detection);
On 2016/01/12 12:01:21, tommi-chromium wrote:
> it would be good to change the return type in these cases to be
> scoped_ptr<DenoiseFilter> to explicitly declare ownership passing and require
> the caller to use scoped_ptr.

Done.

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
File webrtc/modules/video_processing/video_denoiser.cc (right):

https://codereview.webrtc.org/1492053003/diff/240001/webrtc/modules/video_pro...
webrtc/modules/video_processing/video_denoiser.cc:83: metrics_.reset(new
DenoiseMetrics[mb_cols * mb_rows]());
On 2016/01/12 12:01:21, tommi-chromium wrote:
> I'm curious about what this change does?

This is to initialize the array, since the msan test will check this and give a
warning, though it is not necessary to init it.
This will only do once when the denoiser starts, so not expensive.