Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1813173002:
Deletes the class VideoRendererCallback. (Closed)
|
Created:
4 years, 9 months ago by nisse-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, sdk-team_agora.io, 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. |
DescriptionThis cl deletes the class webrtc::VideoRendererCallback.
Replaced by VideoSinkInterface instead.
Also delete stream_id property of IncomingVideoStream.
BUG=webrtc:5426
Committed: https://crrev.com/30f118effd6521e76fe603aa58f5e81988d5aaff
Cr-Commit-Position: refs/heads/master@{#12602}
Patch Set 1 #Patch Set 2 : Fixes to mac, android and windows rendererers. #Patch Set 3 : Delete unused variable, affecting mac builds. #Patch Set 4 : Fix remaining RenderFrame call. #
Total comments: 6
Patch Set 5 : Rebase, and tweak of DEPS files. #
Total comments: 20
Patch Set 6 : Break long lines. Add explicit keyword on IncomingVideoStream constructor. #
Total comments: 6
Patch Set 7 : Rebased. #
Total comments: 3
Patch Set 8 : Delete unused method IncomingVideoStream::ModuleCallback. #
Messages
Total messages: 39 (16 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/1813173002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813173002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13249) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/1709) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/7938) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13628)
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/1813173002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813173002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/1711) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/7940) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13630)
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/1813173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813173002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
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/1813173002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813173002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4257)
Description was changed from ========== RFC: This cl deletes the class VideoRendererCallback. Replaced by VideoSinkInterface instead. Delete unused stream_id argument for RenderFrame. Delete streamId from IncomingVideoStream. Renaming RenderFrame --> OnFrame. Delete VideoRenderCallback. BUG= ========== to ========== This cl deletes the class webrtc::VideoRendererCallback. Replaced by VideoSinkInterface instead. Delete unused stream_id argument for RenderFrame. Delete streamId from IncomingVideoStream. Renaming RenderFrame --> OnFrame. Delete VideoRenderCallback. BUG=webrtc:5426 ==========
nisse@webrtc.org changed reviewers: + pbos@webrtc.org, perkj@webrtc.org
Nice to get rid of am interface. But can you ask pbos and mflodman if the render implementations are actually used or if they can all be deleted? https://codereview.webrtc.org/1813173002/diff/60001/webrtc/DEPS File webrtc/DEPS (right): https://codereview.webrtc.org/1813173002/diff/60001/webrtc/DEPS#newcode34 webrtc/DEPS:34: "+webrtc/media", webrtc/media/base ? https://codereview.webrtc.org/1813173002/diff/60001/webrtc/common_video/inclu... File webrtc/common_video/include/incoming_video_stream.h (right): https://codereview.webrtc.org/1813173002/diff/60001/webrtc/common_video/inclu... webrtc/common_video/include/incoming_video_stream.h:26: class IncomingVideoStream : public rtc::VideoSinkInterface<VideoFrame> { I suspect this whole class can be removed... https://codereview.webrtc.org/1813173002/diff/60001/webrtc/modules/video_rend... File webrtc/modules/video_render/android/video_render_android_impl.h (right): https://codereview.webrtc.org/1813173002/diff/60001/webrtc/modules/video_rend... webrtc/modules/video_render/android/video_render_android_impl.h:33: class AndroidStream : public rtc::VideoSinkInterface<VideoFrame> { also unused and can be removed?
nisse@webrtc.org changed reviewers: + mflodman@webrtc.org
@mflodman: This cl touches a lot of renderers under modules/video_render. Do you know if they are all used, or if some can be deleted rather than updated? https://codereview.webrtc.org/1813173002/diff/60001/webrtc/DEPS File webrtc/DEPS (right): https://codereview.webrtc.org/1813173002/diff/60001/webrtc/DEPS#newcode34 webrtc/DEPS:34: "+webrtc/media", On 2016/03/22 09:06:59, perkj_webrtc wrote: > webrtc/media/base ? Moved to common_video/DEPS. https://codereview.webrtc.org/1813173002/diff/60001/webrtc/common_video/inclu... File webrtc/common_video/include/incoming_video_stream.h (right): https://codereview.webrtc.org/1813173002/diff/60001/webrtc/common_video/inclu... webrtc/common_video/include/incoming_video_stream.h:26: class IncomingVideoStream : public rtc::VideoSinkInterface<VideoFrame> { On 2016/03/22 09:06:59, perkj_webrtc wrote: > I suspect this whole class can be removed... Maybe, but not trivially. It's the place where the disable_prerenderer_smoothing is ultimately used (and if false, it sets up a separate delivery thread). https://codereview.webrtc.org/1813173002/diff/60001/webrtc/modules/video_rend... File webrtc/modules/video_render/android/video_render_android_impl.h (right): https://codereview.webrtc.org/1813173002/diff/60001/webrtc/modules/video_rend... webrtc/modules/video_render/android/video_render_android_impl.h:33: class AndroidStream : public rtc::VideoSinkInterface<VideoFrame> { On 2016/03/22 09:06:59, perkj_webrtc wrote: > also unused and can be removed? It is used in some way by the VideoRenderAndroid class, just below.
Ping?
Regardless of if more of the code should be deleted this looks good. lgtm with lots of nits. https://codereview.webrtc.org/1813173002/diff/80001/webrtc/common_video/inclu... File webrtc/common_video/include/incoming_video_stream.h (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/common_video/inclu... webrtc/common_video/include/incoming_video_stream.h:81: rtc::VideoSinkInterface<VideoFrame>* external_callback_ GUARDED_BY(thread_critsect_); git cl format. Line length... https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/external/video_render_external_impl.h (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/external/video_render_external_impl.h:21: class VideoRenderExternalImpl: IVideoRender, public rtc::VideoSinkInterface<VideoFrame> line length. https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/test/testAPI/testAPI.cc (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/test/testAPI/testAPI.cc:289: rtc::VideoSinkInterface<VideoFrame>* renderCallback0 = renderModule->AddIncomingRenderStream(streamId0, 0, 0.0f, 0.0f, 1.0f, 1.0f); line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/test/testAPI/testAPI.cc:353: rtc::VideoSinkInterface<VideoFrame>* renderCallback0 = renderModule->AddIncomingRenderStream(streamId0, 0, 0.0f, 0.0f, 1.0f, 1.0f); line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/test/testAPI/testAPI.cc:421: rtc::VideoSinkInterface<VideoFrame>* renderCallback0 = line length here and below https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/video_render.h (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render.h:110: * streamID - id of the stream the callback is used for line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render.h:111: * renderObject - the rtc::VideoSinkInterface<VideoFrame> to use for this stream, NULL to remove line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render.h:117: rtc::VideoSinkInterface<VideoFrame>* renderObject) = 0; line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render.h:157: rtc::VideoSinkInterface<VideoFrame>* callbackObj) = 0; line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/video_render_impl.cc (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render_impl.cc:199: rtc::VideoSinkInterface<VideoFrame>* moduleCallback = ptrIncomingStream->ModuleCallback(); line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/video_render_impl.h (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render_impl.h:80: rtc::VideoSinkInterface<VideoFrame>* renderObject); line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render_impl.h:110: rtc::VideoSinkInterface<VideoFrame>* callbackObj); line length https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/video_render_internal_impl.cc (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render_internal_impl.cc:422: rtc::VideoSinkInterface<VideoFrame>* moduleCallback = ptrIncomingStream->ModuleCallback(); line length
New version. Only non-cosmetic change is an added explicit on a constructor which used to take two args but now only takes one. I haven't done a a full cl format, since it produces way to much unrelated changes. But I've hunted down all overlong long lines I introduced in the previous patch set. https://codereview.webrtc.org/1813173002/diff/80001/webrtc/common_video/inclu... File webrtc/common_video/include/incoming_video_stream.h (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/common_video/inclu... webrtc/common_video/include/incoming_video_stream.h:81: rtc::VideoSinkInterface<VideoFrame>* external_callback_ GUARDED_BY(thread_critsect_); On 2016/03/31 05:11:58, perkj_webrtc wrote: > git cl format. Line length... Done. https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/external/video_render_external_impl.h (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/external/video_render_external_impl.h:21: class VideoRenderExternalImpl: IVideoRender, public rtc::VideoSinkInterface<VideoFrame> On 2016/03/31 05:11:58, perkj_webrtc wrote: > line length. Done. https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/test/testAPI/testAPI.cc (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/test/testAPI/testAPI.cc:289: rtc::VideoSinkInterface<VideoFrame>* renderCallback0 = renderModule->AddIncomingRenderStream(streamId0, 0, 0.0f, 0.0f, 1.0f, 1.0f); On 2016/03/31 05:11:58, perkj_webrtc wrote: > line length Done. https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/test/testAPI/testAPI.cc:421: rtc::VideoSinkInterface<VideoFrame>* renderCallback0 = On 2016/03/31 05:11:58, perkj_webrtc wrote: > line length here and below Not my lines... But I'll break them anyway. https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/video_render.h (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render.h:111: * renderObject - the rtc::VideoSinkInterface<VideoFrame> to use for this stream, NULL to remove On 2016/03/31 05:11:58, perkj_webrtc wrote: > line length Done. (All in this file). https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/video_render_impl.cc (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render_impl.cc:199: rtc::VideoSinkInterface<VideoFrame>* moduleCallback = ptrIncomingStream->ModuleCallback(); On 2016/03/31 05:11:59, perkj_webrtc wrote: > line length Done. https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... File webrtc/modules/video_render/video_render_internal_impl.cc (right): https://codereview.webrtc.org/1813173002/diff/80001/webrtc/modules/video_rend... webrtc/modules/video_render/video_render_internal_impl.cc:422: rtc::VideoSinkInterface<VideoFrame>* moduleCallback = ptrIncomingStream->ModuleCallback(); On 2016/03/31 05:11:59, perkj_webrtc wrote: > line length Done.
Is this CL obsolete now, or should it be rebased? If not, please close.
On 2016/05/02 02:13:43, pbos-webrtc wrote: > Is this CL obsolete now, or should it be rebased? If not, please close. I'll rebase it, and see what's left.
Nice cleanup! A few nits, then LGTM. https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... File webrtc/modules/video_render/android/video_render_android_native_opengl2.h (right): https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... webrtc/modules/video_render/android/video_render_android_native_opengl2.h:35: //Implement rtc::VideoSinkInterface<VideoFrame> Old nit, but since changing: space after '//' and end with a '.'. https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... File webrtc/modules/video_render/external/video_render_external_impl.cc (right): https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... webrtc/modules/video_render/external/video_render_external_impl.cc:190: // rtc::VideoSinkInterface<VideoFrame> Remove this comments. https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... File webrtc/modules/video_render/external/video_render_external_impl.h (right): https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... webrtc/modules/video_render/external/video_render_external_impl.h:117: // rtc::VideoSinkInterface<VideoFrame> Add 'Implements'. https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... File webrtc/modules/video_render/linux/video_render_linux_impl.h (right): https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... webrtc/modules/video_render/linux/video_render_linux_impl.h:46: * AddIncomingRenderStream(const uint32_t streamId, Move '*' to line above. https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... File webrtc/modules/video_render/mac/video_render_agl.h (right): https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... webrtc/modules/video_render/mac/video_render_agl.h:55: virtual void OnFrame(const VideoFrame& videoFrame); Add comment: // Implements rtc::VideoSinkInterface<VideoFrame>. https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... File webrtc/modules/video_render/windows/video_render_windows_impl.h (right): https://codereview.webrtc.org/1813173002/diff/100001/webrtc/modules/video_ren... webrtc/modules/video_render/windows/video_render_windows_impl.h:54: * AddIncomingRenderStream(const uint32_t streamId, * to line above.
Rebased. (Un)fortunately, all mflodman's nits were on code which was deleted the other day (video_render module).
lgtm, if the cl desc isn't updated yet please do that :) https://codereview.webrtc.org/1813173002/diff/120001/webrtc/common_video/inco... File webrtc/common_video/incoming_video_stream.cc (right): https://codereview.webrtc.org/1813173002/diff/120001/webrtc/common_video/inco... webrtc/common_video/incoming_video_stream.cc:50: rtc::VideoSinkInterface<VideoFrame>* IncomingVideoStream::ModuleCallback() { Can we remove this? Just use the stream directly.
Description was changed from ========== This cl deletes the class webrtc::VideoRendererCallback. Replaced by VideoSinkInterface instead. Delete unused stream_id argument for RenderFrame. Delete streamId from IncomingVideoStream. Renaming RenderFrame --> OnFrame. Delete VideoRenderCallback. BUG=webrtc:5426 ========== to ========== This cl deletes the class webrtc::VideoRendererCallback. Replaced by VideoSinkInterface instead. Also delete stream_id property of IncomingVideoStream. BUG=webrtc:5426 ==========
Anything in particular you want changed in the description? I've cut it down a bit now. https://codereview.webrtc.org/1813173002/diff/120001/webrtc/common_video/inco... File webrtc/common_video/incoming_video_stream.cc (right): https://codereview.webrtc.org/1813173002/diff/120001/webrtc/common_video/inco... webrtc/common_video/incoming_video_stream.cc:50: rtc::VideoSinkInterface<VideoFrame>* IncomingVideoStream::ModuleCallback() { On 2016/05/02 13:08:17, pbos-webrtc wrote: > Can we remove this? Just use the stream directly. It appears unused. I'll delete it. What was it intended for?
https://codereview.webrtc.org/1813173002/diff/120001/webrtc/common_video/inco... File webrtc/common_video/incoming_video_stream.cc (right): https://codereview.webrtc.org/1813173002/diff/120001/webrtc/common_video/inco... webrtc/common_video/incoming_video_stream.cc:50: rtc::VideoSinkInterface<VideoFrame>* IncomingVideoStream::ModuleCallback() { On 2016/05/02 13:31:23, nisse-webrtc wrote: > On 2016/05/02 13:08:17, pbos-webrtc wrote: > > Can we remove this? Just use the stream directly. > > It appears unused. I'll delete it. > > What was it intended for? This was probably used as a module sometime in the past, then it got retyped etc. Long story short, nuke it. :)
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org, mflodman@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1813173002/#ps140001 (title: "Delete unused method IncomingVideoStream::ModuleCallback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1813173002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813173002/140001
Message was sent while issue was closed.
Description was changed from ========== This cl deletes the class webrtc::VideoRendererCallback. Replaced by VideoSinkInterface instead. Also delete stream_id property of IncomingVideoStream. BUG=webrtc:5426 ========== to ========== This cl deletes the class webrtc::VideoRendererCallback. Replaced by VideoSinkInterface instead. Also delete stream_id property of IncomingVideoStream. BUG=webrtc:5426 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== This cl deletes the class webrtc::VideoRendererCallback. Replaced by VideoSinkInterface instead. Also delete stream_id property of IncomingVideoStream. BUG=webrtc:5426 ========== to ========== This cl deletes the class webrtc::VideoRendererCallback. Replaced by VideoSinkInterface instead. Also delete stream_id property of IncomingVideoStream. BUG=webrtc:5426 Committed: https://crrev.com/30f118effd6521e76fe603aa58f5e81988d5aaff Cr-Commit-Position: refs/heads/master@{#12602} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/30f118effd6521e76fe603aa58f5e81988d5aaff Cr-Commit-Position: refs/heads/master@{#12602} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
