Delete method webrtc::VideoFrame::native_handle.

Delete method webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the process,
reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

The cl also refactors VideoFrame::CopyFrame to use I420Buffer::Copy. This
has possibly undesired side effects of never reusing the frame buffer of
the destination frame, and producing a frame buffer which may use different
stride than the source frame.

BUG=webrtc:5682
Committed: https://crrev.com/26acec4772a426f46388cacd315a6af2affe4679
Cr-Commit-Position: refs/heads/master@{#12373}

[nisse-webrtc, 2016-04-12 08:50:44]

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

[commit-bot: I haz the power, 2016-04-12 08:50:51]

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

[commit-bot: I haz the power, 2016-04-12 08:54:22]

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

[commit-bot: I haz the power, 2016-04-12 08:54:23]

Dry run: Try jobs failed on following builders:
  linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[nisse-webrtc, 2016-04-12 09:05:14]

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

[commit-bot: I haz the power, 2016-04-12 09:05:21]

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

[commit-bot: I haz the power, 2016-04-12 09:08:51]

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

[commit-bot: I haz the power, 2016-04-12 09:08:52]

Dry run: Try jobs failed on following builders:
  android_compile_mips_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...)

[nisse-webrtc, 2016-04-12 11:41:17]

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

[commit-bot: I haz the power, 2016-04-12 11:41:26]

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

[commit-bot: I haz the power, 2016-04-12 11:45:20]

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

[commit-bot: I haz the power, 2016-04-12 11:45:21]

Dry run: Try jobs failed on following builders:
  android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)

[nisse-webrtc, 2016-04-12 12:07:12]

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

[commit-bot: I haz the power, 2016-04-12 12:07:17]

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

[nisse-webrtc, 2016-04-12 12:20:51]

Description was changed from

==========
Stop using webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the
process, reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

BUG=webrtc:5682
==========

to

==========
Stop using webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the process, 
reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

The cl also refactors VideoFrame::CopyFrame to use I420Buffer::Copy. This 
has possibly undesired side effects of never reusing the frame buffer of 
the destination frame, and producing a frame buffer which may use different
stride than the source frame.

BUG=webrtc:5682
==========

[nisse-webrtc, 2016-04-12 12:21:42]

nisse@webrtc.org changed reviewers:
+ magjed@webrtc.org, pbos@webrtc.org, perkj@webrtc.org

[nisse-webrtc, 2016-04-12 12:21:43]

I also have a question on video_frame_buffer(). When does it make
sense to have a webrtc::VideoFrame with out an associatd frame
buffer? In this case, video_frame_buffer() returns a nullptr and
IsZeroSize returns true.

It would make the code less cluttered if one could call

frame.video_frame_buffer()->native_handle 

(and later on, ->data, ->stride, etc), without having to check at
each call site if video_frame_buffer returns nullptr. And it
would be nice to get rid of IsZeroSize too.

[commit-bot: I haz the power, 2016-04-12 14:07:57]

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

[commit-bot: I haz the power, 2016-04-12 14:07:58]

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

[perkj_webrtc, 2016-04-13 05:44:08]

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/common_video/video...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/common_video/video...
webrtc/common_video/video_frame.cc:170: #if
ENABLE_WEBRTC_VIDEOFRAME_NATIVE_HANDLE
?

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:303: // TODO(nisse): Is it allowed
to have video_frame_buffer() return a
I don't think video_frame_buffer() ever should be allowed to be null.

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

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:377: // TODO(nisse): Is it allowed to have
video_frame_buffer() return a
no

[nisse-webrtc, 2016-04-13 07:28:50]

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/common_video/video...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/common_video/video...
webrtc/common_video/video_frame.cc:170: #if
ENABLE_WEBRTC_VIDEOFRAME_NATIVE_HANDLE
On 2016/04/13 05:44:08, perkj_webrtc wrote:
> ?

I wanted to disable the code temporarily for a cq dry run. Now that I've seen
that android and ios bots are happy, I'll delete this preprocessor constant.

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/1881953002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:303: // TODO(nisse): Is it allowed
to have video_frame_buffer() return a
On 2016/04/13 05:44:08, perkj_webrtc wrote:
> I don't think video_frame_buffer() ever should be allowed to be null.

Good. Should I add some RTC_CHECKS to that effect at the places where
video_frame_buffer_ is assigned? BTW, it seems we don't have any macros
expanding to __attribute__((nonnull)), should we? (But maybe nonnull it can't be
used with scoped_refptr anyway).

But I think I need to investigate in a separate cl. Problem is that the default
constructor, as well as the public Reset method, sets video_frame_buffer_ to
nullptr.

[nisse-webrtc, 2016-04-13 07:38:20]

On 2016/04/13 07:28:50, nisse-webrtc wrote:
> But I think I need to investigate in a separate cl. Problem is that the
default
> constructor, as well as the public Reset method, sets video_frame_buffer_ to
> nullptr.

And there's one piece of code which depends on the behaviour of Reset and
IsZeroSize(): 
VideoCaptureInput::GetVideoFrame.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Can this be refactored? It would be easy to add a bool frame_pending flag (or
rtc::Optional<webrtc::VideoFrame>), but maybe it's important to drop the
reference to the frame buffer here, so it can be discarded or reused? I't would
be nice if we could eliminate both Reset and the (strangely named) IsZeroSize.

[magjed_webrtc, 2016-04-13 08:53:06]

> And there's one piece of code which depends on the behaviour of Reset and
> IsZeroSize(): 
> VideoCaptureInput::GetVideoFrame.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> Can this be refactored? It would be easy to add a bool frame_pending flag (or
> rtc::Optional<webrtc::VideoFrame>), but maybe it's important to drop the
> reference to the frame buffer here, so it can be discarded or reused? I't
would
> be nice if we could eliminate both Reset and the (strangely named) IsZeroSize.

You can refactor |captured_frame_| to be a scoped_ptr<VideoFrame> instead of a
VideoFrame, and use reset on the scoped_ptr. I agree it would be nice if we can
remove Reset and IsZeroSize. I don't think we should remove the null checks in
e.g. native_handle() before Reset is removed and we check in the ctor that the
buffer is non-null.

BTW, can we remove VideoFrame::CopyFrame completely? I don't see any uses.

[magjed_webrtc, 2016-04-13 08:53:23]

https://codereview.webrtc.org/1881953002/diff/100001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/1881953002/diff/100001/webrtc/video_frame.h#new...
webrtc/video_frame.h:128: // TODO(nisse): Should be deleted, in favor of using
the
Can't you update the place in Chrome first, and remove this immediately?

[nisse-webrtc, 2016-04-13 10:09:19]

On 2016/04/13 08:53:06, magjed_webrtc wrote:
> You can refactor |captured_frame_| to be a scoped_ptr<VideoFrame> instead of a
> VideoFrame, and use reset on the scoped_ptr. I agree it would be nice if we
can
> remove Reset and IsZeroSize.

I'll give it a try.

> BTW, can we remove VideoFrame::CopyFrame completely? I don't see any uses.

It appears to be used in a few places in IncomingVideoStream and one place in
ViEEncoder.

[nisse-webrtc, 2016-04-13 13:31:58]

On 2016/04/13 08:53:23, magjed_webrtc wrote:

> Can't you update the place in Chrome first, and remove this immediately?

Good idea. Part of https://codereview.chromium.org/1882583004/

[magjed_webrtc, 2016-04-14 07:32:07]

lgtm

[nisse-webrtc, 2016-04-14 07:37:09]

Description was changed from

==========
Stop using webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the process, 
reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

The cl also refactors VideoFrame::CopyFrame to use I420Buffer::Copy. This 
has possibly undesired side effects of never reusing the frame buffer of 
the destination frame, and producing a frame buffer which may use different
stride than the source frame.

BUG=webrtc:5682
==========

to

==========
Delete method webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the process, 
reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

The cl also refactors VideoFrame::CopyFrame to use I420Buffer::Copy. This 
has possibly undesired side effects of never reusing the frame buffer of 
the destination frame, and producing a frame buffer which may use different
stride than the source frame.

BUG=webrtc:5682
==========

[nisse-webrtc, 2016-04-14 07:47:07]

Deleted the method, now that a cl eliminating it in chrome is landed
(https://codereview.chromium.org/1882583004/).

What about the CopyFrame changes, do you see any problems? It is used in two
places:

ViEEncoder, if pre_encode_callback is used. No idea if this feature is used, and
why it needs a deep copy. Anyway, this call never has a framebuffer to reuse, so
no change.

And for the SetStartImage and SetTimeOutImage methods in IncomingVideoStream.
Unlikely to be performance critical. I also can't find any use of these methods,
can they be deleted?

[pbos-webrtc, 2016-04-14 14:50:54]

lgtm

https://codereview.webrtc.org/1881953002/diff/120001/webrtc/video/video_captu...
File webrtc/video/video_capture_input_unittest.cc (right):

https://codereview.webrtc.org/1881953002/diff/120001/webrtc/video/video_captu...
webrtc/video/video_capture_input_unittest.cc:236:
frames2[i]->video_frame_buffer()))
{}s

[nisse-webrtc, 2016-04-15 07:15:09]

https://codereview.webrtc.org/1881953002/diff/120001/webrtc/video/video_captu...
File webrtc/video/video_capture_input_unittest.cc (right):

https://codereview.webrtc.org/1881953002/diff/120001/webrtc/video/video_captu...
webrtc/video/video_capture_input_unittest.cc:236:
frames2[i]->video_frame_buffer()))
On 2016/04/14 14:50:54, pbos-webrtc wrote:
> {}s

Done.

[perkj_webrtc, 2016-04-15 08:25:31]

lgtm

[nisse-webrtc, 2016-04-15 08:35:35]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-15 08:35:36]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1881953002/#ps140001 (title:
"Add missing braces.")

[commit-bot: I haz the power, 2016-04-15 08:35:48]

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

[commit-bot: I haz the power, 2016-04-15 10:43:42]

Description was changed from

==========
Delete method webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the process, 
reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

The cl also refactors VideoFrame::CopyFrame to use I420Buffer::Copy. This 
has possibly undesired side effects of never reusing the frame buffer of 
the destination frame, and producing a frame buffer which may use different
stride than the source frame.

BUG=webrtc:5682
==========

to

==========
Delete method webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the process, 
reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

The cl also refactors VideoFrame::CopyFrame to use I420Buffer::Copy. This 
has possibly undesired side effects of never reusing the frame buffer of 
the destination frame, and producing a frame buffer which may use different
stride than the source frame.

BUG=webrtc:5682
==========

[commit-bot: I haz the power, 2016-04-15 10:43:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-15 10:43:52]

Description was changed from

==========
Delete method webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the process, 
reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

The cl also refactors VideoFrame::CopyFrame to use I420Buffer::Copy. This 
has possibly undesired side effects of never reusing the frame buffer of 
the destination frame, and producing a frame buffer which may use different
stride than the source frame.

BUG=webrtc:5682
==========

to

==========
Delete method webrtc::VideoFrame::native_handle.

Instead, use the corresponding method on VideoFrameBuffer. In the process,
reduce code duplication in frame comparison functions used in
the test code.

Make FramesEqual use FrameBufsEqual. Make the latter support texture frames.

The cl also refactors VideoFrame::CopyFrame to use I420Buffer::Copy. This
has possibly undesired side effects of never reusing the frame buffer of
the destination frame, and producing a frame buffer which may use different
stride than the source frame.

BUG=webrtc:5682

Committed: https://crrev.com/26acec4772a426f46388cacd315a6af2affe4679
Cr-Commit-Position: refs/heads/master@{#12373}
==========

[commit-bot: I haz the power, 2016-04-15 10:43:53]

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