Refactor VideoDenoiser to work with I420Buffer, not VideoFrame.

Refactor VideoDenoiser to work with I420Buffer, not VideoFrame.

BUG=webrtc:5921
R=jackychen@webrtc.org, marpan@webrtc.org

Committed: https://crrev.com/6af2e86b46a638f8a9309dffc3181053bd32974e
Cr-Commit-Position: refs/heads/master@{#13179}

[nisse-webrtc, 2016-05-23 10:10:14]

nisse@webrtc.org changed reviewers:
+ holmer@chromium.org, jackychen@webrtc.org

[nisse-webrtc, 2016-05-23 10:10:15]

Hi,

I'm attempting to convert code mutating pixel data to use I420Buffer rather than
VideoFrame, following the grand plan to make VideoFrame and the VideoFrameBuffer
base class always keep immutable pixels.

This cl converts the VideoDenoiser class in a simple-minded way, I haven't tried
to understand how it works in any detail.

I'm considering one additional change: To drop the denoised_frame and
denoised_frame_prev arguments to VideoDenoiser::DenoiseFrame. Instead keep
I420Buffers as member variables, and return the relevant one.

That way, the double-buffering logic can also be moved inside this class, so
callers don't need to bother.

Which unit tests are relevant at this level? I had to update
VideoProcessingTest.Denoiser, but that only checks agreement between the
underlying C and asm denoising filters, and is unlikely to trigger if this cl
breaks things.

[nisse-webrtc, 2016-05-23 10:36:13]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 10:36:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005733003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2005733003/1

[commit-bot: I haz the power, 2016-05-23 12:36:52]

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

[commit-bot: I haz the power, 2016-05-23 12:36:56]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-06-09 07:42:41]

Ping?

[stefan-webrtc, 2016-06-09 09:45:23]

Jacky, can you take a look at this?

[jackychen, 2016-06-09 19:08:36]

Have you verified that this cl works without any behavior change for denoiser?
It looks ok for me, as it doesn't change the logic inside the denoiser.

https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
webrtc/common_video/video_frame_buffer.cc:227: return copy;
Could you use a better name instead of copy? At least, we should know it is a
buffer copy.

[nisse-webrtc, 2016-06-10 11:07:18]

On 2016/06/09 19:08:36, jackychen wrote:
> Have you verified that this cl works without any behavior change for denoiser?
> It looks ok for me, as it doesn't change the logic inside the denoiser.

How do you suggest I verify it, besides just running the test suite?
I'm not familiar with this code and I'm not sure where it is enabled (e.g., the
android AppRTCDemo).

[nisse-webrtc, 2016-06-10 11:09:20]

https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
webrtc/common_video/video_frame_buffer.cc:227: return copy;
On 2016/06/09 19:08:36, jackychen wrote:
> Could you use a better name instead of copy? At least, we should know it is a
> buffer copy.

To me, "copy" seems quite clear from the context, but I can try to improve the
naming. Would it be clearer if I rename buffer --> src (the input argument) and
copy --> buffer (the returned value)?

[jackychen, 2016-06-10 16:56:05]

On 2016/06/10 11:09:20, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
> File webrtc/common_video/video_frame_buffer.cc (right):
> 
>
https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
> webrtc/common_video/video_frame_buffer.cc:227: return copy;
> On 2016/06/09 19:08:36, jackychen wrote:
> > Could you use a better name instead of copy? At least, we should know it is
a
> > buffer copy.
> 
> To me, "copy" seems quite clear from the context, but I can try to improve the
> naming. Would it be clearer if I rename buffer --> src (the input argument)
and
> copy --> buffer (the returned value)?

What about "buffer" -> "buf_src", copy -> "buf_copy"?

[pbos-webrtc, 2016-06-10 16:57:17]

On 2016/06/10 16:56:05, jackychen wrote:
> On 2016/06/10 11:09:20, nisse-webrtc wrote:
> >
>
https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
> > File webrtc/common_video/video_frame_buffer.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
> > webrtc/common_video/video_frame_buffer.cc:227: return copy;
> > On 2016/06/09 19:08:36, jackychen wrote:
> > > Could you use a better name instead of copy? At least, we should know it
is
> a
> > > buffer copy.
> > 
> > To me, "copy" seems quite clear from the context, but I can try to improve
the
> > naming. Would it be clearer if I rename buffer --> src (the input argument)
> and
> > copy --> buffer (the returned value)?
> 
> What about "buffer" -> "buf_src", copy -> "buf_copy"?

buffer_source or buffer_copy in that case

[jackychen, 2016-06-10 17:07:04]

On 2016/06/10 11:07:18, nisse-webrtc wrote:
> On 2016/06/09 19:08:36, jackychen wrote:
> > Have you verified that this cl works without any behavior change for
denoiser?
> > It looks ok for me, as it doesn't change the logic inside the denoiser.
> 
> How do you suggest I verify it, besides just running the test suite?
> I'm not familiar with this code and I'm not sure where it is enabled (e.g.,
the
> android AppRTCDemo).

You can enable denoiser
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_process...,
just change "false" to "true".

You don't need to enable it for the testing. In the unit test, you can output
the result of the denoiser w/wo your cl and compare them which is pretty easy to
do.

[nisse-webrtc, 2016-06-15 09:04:21]

On 2016/06/10 17:07:04, jackychen wrote:
> On 2016/06/10 11:07:18, nisse-webrtc wrote:
> > On 2016/06/09 19:08:36, jackychen wrote:
> > > Have you verified that this cl works without any behavior change for
> denoiser?
> > > It looks ok for me, as it doesn't change the logic inside the denoiser.
> > 
> > How do you suggest I verify it, besides just running the test suite?
> > I'm not familiar with this code and I'm not sure where it is enabled (e.g.,
> the
> > android AppRTCDemo).
> 
> You can enable denoiser
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_process...,
> just change "false" to "true".
> 
> You don't need to enable it for the testing. In the unit test, you can output
> the result of the denoiser w/wo your cl and compare them which is pretty easy
to
> do.

I added code (locally) to VideoProcessingTest.Denoiser, to write out the
denoised frames to a file (webrtc::test::OutputPath() +
"denoiser_test_out.yuv"), and I get identical results on the master branch and
with this cl. So that seems fine.

Would it make sense to check the gen_files command line flag, similarly to
WriteProcessedFrameForVisualInspection? See

https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_process...
 ?

[jackychen, 2016-06-15 15:38:54]

On 2016/06/15 09:04:21, nisse-webrtc wrote:
> On 2016/06/10 17:07:04, jackychen wrote:
> > On 2016/06/10 11:07:18, nisse-webrtc wrote:
> > > On 2016/06/09 19:08:36, jackychen wrote:
> > > > Have you verified that this cl works without any behavior change for
> > denoiser?
> > > > It looks ok for me, as it doesn't change the logic inside the denoiser.
> > > 
> > > How do you suggest I verify it, besides just running the test suite?
> > > I'm not familiar with this code and I'm not sure where it is enabled
(e.g.,
> > the
> > > android AppRTCDemo).
> > 
> > You can enable denoiser
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_process...,
> > just change "false" to "true".
> > 
> > You don't need to enable it for the testing. In the unit test, you can
output
> > the result of the denoiser w/wo your cl and compare them which is pretty
easy
> to
> > do.
> 
> I added code (locally) to VideoProcessingTest.Denoiser, to write out the
> denoised frames to a file (webrtc::test::OutputPath() +
> "denoiser_test_out.yuv"), and I get identical results on the master branch and
> with this cl. So that seems fine.
> 
> Would it make sense to check the gen_files command line flag, similarly to
> WriteProcessedFrameForVisualInspection? See
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_process...
>  ?

LGTM. Stefan, do you have more comments?

[nisse-webrtc, 2016-06-16 07:44:07]

On 2016/06/10 16:57:17, pbos-webrtc wrote:
> On 2016/06/10 16:56:05, jackychen wrote:
> > On 2016/06/10 11:09:20, nisse-webrtc wrote:
> > >
> >
>
https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
> > > File webrtc/common_video/video_frame_buffer.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2005733003/diff/1/webrtc/common_video/video_fra...
> > > webrtc/common_video/video_frame_buffer.cc:227: return copy;
> > > On 2016/06/09 19:08:36, jackychen wrote:
> > > > Could you use a better name instead of copy? At least, we should know it
> is
> > a
> > > > buffer copy.
> > > 
> > > To me, "copy" seems quite clear from the context, but I can try to improve
> the
> > > naming. Would it be clearer if I rename buffer --> src (the input
argument)
> > and
> > > copy --> buffer (the returned value)?
> > 
> > What about "buffer" -> "buf_src", copy -> "buf_copy"?
> 
> buffer_source or buffer_copy in that case

Ok, I've changed to those names, both in the new method and the Copy method just
above.
But to my eyes, the "buffer_" prefix on names everywhere in these functions
makes the code *considerably* less readable.

In particular, since everything is the same type anyway, that really doesn't
make anything clearer.

Unless you have a strong objection to that, I'll change back to some shorter
names (is |source| and |target| better than the original |buffer|, |copy|?)
without any typing prefix or suffix.

[nisse-webrtc, 2016-06-16 08:12:41]

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

[nisse-webrtc, 2016-06-16 08:12:41]

+pbos, who expressed opinions on naming.

[nisse-webrtc, 2016-06-16 09:50:29]

nisse@webrtc.org changed reviewers:
+ marpan@webrtc.org

[nisse-webrtc, 2016-06-16 09:50:29]

Marco, can you have an owner's look? Stefan is on vacation.

[pbos-webrtc, 2016-06-16 11:33:10]

On 2016/06/16 08:12:41, nisse-webrtc wrote:
> +pbos, who expressed opinions on naming.

Just had an opinion on bfr vs buffer

[marpan2, 2016-06-16 14:16:00]

lgtm

[nisse-webrtc, 2016-06-16 14:49:47]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-06-16 14:49:47]

The patchset sent to the CQ was uploaded after l-g-t-m from jackychen@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2005733003/#ps40001 (title:
"Shorter names, for even better clarity.")

[commit-bot: I haz the power, 2016-06-16 14:49:52]

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

[commit-bot: I haz the power, 2016-06-16 14:51:23]

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

[commit-bot: I haz the power, 2016-06-16 14:51:24]

Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14256)
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14229)
  linux_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)

[nisse-webrtc, 2016-06-16 15:00:35]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-06-16 15:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
jackychen@webrtc.org, marpan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2005733003/#ps60001 (title:
"Rebased.")

[commit-bot: I haz the power, 2016-06-16 15:00:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005733003/60001

[commit-bot: I haz the power, 2016-06-16 15:17:30]

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

[commit-bot: I haz the power, 2016-06-16 15:17:32]

Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14258)

[nisse-webrtc, 2016-06-17 07:13:00]

Description was changed from

==========
Refactor VideoDenoiser to work with I420Buffer, not VideoFrame.

BUG=webrtc:5921
==========

to

==========
Refactor VideoDenoiser to work with I420Buffer, not VideoFrame.

BUG=webrtc:5921
R=jackychen@webrtc.org, marpan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/6af2e86b46a638f8a9309dffc...
==========

[nisse-webrtc, 2016-06-17 07:13:01]

Committed patchset #5 (id:80001) manually as
6af2e86b46a638f8a9309dffc3181053bd32974e (presubmit successful).

[commit-bot: I haz the power, 2016-06-17 07:13:03]

Description was changed from

==========
Refactor VideoDenoiser to work with I420Buffer, not VideoFrame.

BUG=webrtc:5921
R=jackychen@webrtc.org, marpan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/6af2e86b46a638f8a9309dffc...
==========

to

==========
Refactor VideoDenoiser to work with I420Buffer, not VideoFrame.

BUG=webrtc:5921
R=jackychen@webrtc.org, marpan@webrtc.org

Committed: https://crrev.com/6af2e86b46a638f8a9309dffc3181053bd32974e
Cr-Commit-Position: refs/heads/master@{#13179}
==========

[nisse-webrtc, 2016-06-17 07:34:47]

Landed now. Thanks for the review.

I have one remaining question, see below.

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

https://codereview.webrtc.org/2005733003/diff/80001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.h:29: // from the caller.
BTW, what do you think about this idea? It would simplify things for the caller
quite a lot.

[jackychen, 2016-06-17 23:56:50]

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

https://codereview.webrtc.org/2005733003/diff/80001/webrtc/modules/video_proc...
webrtc/modules/video_processing/video_denoiser.h:29: // from the caller.
On 2016/06/17 07:34:47, nisse-webrtc wrote:
> BTW, what do you think about this idea? It would simplify things for the
caller
> quite a lot.

SG for me. You can surely make this change.