Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by nisse-webrtc Modified:
4 years, 8 months ago Reviewers:
magjed_webrtc, pbos-webrtc, perkj_webrtc CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, pbos-webrtc, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete 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}
Patch Set 1 #Patch Set 2 : Use preprocessor hack instead of RTC_DEPRECATED, for testing. #Patch Set 3 : Update android encoder. Fix FrameEquals. #Patch Set 4 : Another android fix. #Patch Set 5 : Make EqualFramesVector and ExpectEqualFramesVector ignore timestamps. #
Total comments: 5
Patch Set 6 : Cleanup TODOs. #
Total comments: 1
Patch Set 7 : Delete the native_handle method. #
Total comments: 2
Patch Set 8 : Add missing braces. #
Created: 4 years, 8 months ago
Messages
Total messages: 39 (15 generated)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
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
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.org changed reviewers: + magjed@webrtc.org, pbos@webrtc.org, perkj@webrtc.org
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.
The CQ bit was unchecked by commit-bot@chromium.org
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)
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
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.
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.
> 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.
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?
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.
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/
lgtm
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 ==========
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?
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
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.
lgtm
The CQ bit was checked by nisse@webrtc.org
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.")
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
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/26acec4772a426f46388cacd315a6af2affe4679 Cr-Commit-Position: refs/heads/master@{#12373} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
