Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1574963002:
Deleted renderer-related SetSize methods, and all uses. (Closed)
|
Created:
4 years, 11 months ago by nisse-webrtc Modified:
4 years, 11 months ago CC:
webrtc-reviews_webrtc.org, guoweis_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDeleted renderer-related SetSize methods, and all uses.
webrtc::VideoRendererInterface::SetSize was completely unused.
cricket::VideoRenderer::SetSize only had dummy implementations
returning true and doing nothing.
BUG=webrtc:5426
Committed: https://crrev.com/c4c84856628c70fada1ada6c4112f5497d993aa6
Cr-Commit-Position: refs/heads/master@{#11298}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Delete also FakeVideoRenderer::SetSize, and update unit tests. #Patch Set 3 : Deleted WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetSize. #Patch Set 4 : Added static cast when converting width and height from size_t to int. #
Total comments: 4
Patch Set 5 : Addressed Peter's comments. #Patch Set 6 : Rebase. #Patch Set 7 : Rebase #
Messages
Total messages: 49 (19 generated)
Description was changed from ========== Deleted renderer-related SetSize methods, and all uses. webrtc::VideoRendererInterface::SetSize was completely unused. cricket::VideoRenderer::SetSize only had dummy implementations returning true and doing nothing. ========== to ========== Deleted renderer-related SetSize methods, and all uses. webrtc::VideoRendererInterface::SetSize was completely unused. cricket::VideoRenderer::SetSize only had dummy implementations returning true and doing nothing. ==========
nisse@webrtc.org changed reviewers: + perkj@webrtc.org, pthatcher@webrtc.org, tommi@webrtc.org
nisse@webrtc.org changed reviewers: + magjed@webrtc.org, pbos@webrtc.org
https://codereview.webrtc.org/1574963002/diff/1/talk/media/base/videorenderer.h File talk/media/base/videorenderer.h (left): https://codereview.webrtc.org/1574963002/diff/1/talk/media/base/videorenderer... talk/media/base/videorenderer.h:42: class VideoRenderer { Except for the RenderFrame return value, and the protection of the destructor, this class declaration is identical to webrtc::VideoRendererInterface. Can we simply delete one of the classes (and if so, which one to keep)? I'd also prefer to rename it to something like VideoFrameSink, but that's less important.
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/1574963002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574963002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/8357)
lgtm assuming the build errors are a minor glitch https://codereview.webrtc.org/1574963002/diff/1/talk/media/base/videorenderer.h File talk/media/base/videorenderer.h (left): https://codereview.webrtc.org/1574963002/diff/1/talk/media/base/videorenderer... talk/media/base/videorenderer.h:42: class VideoRenderer { On 2016/01/11 08:30:52, nisse-webrtc wrote: > Except for the RenderFrame return value, and the protection of the destructor, > this class declaration is identical to webrtc::VideoRendererInterface. Can we > simply delete one of the classes (and if so, which one to keep)? > > I'd also prefer to rename it to something like VideoFrameSink, but that's less > important. keep the webrtc:: one. It's a part of the public interface but cricket:: is an implementation detail.
https://codereview.webrtc.org/1574963002/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1574963002/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:2494: void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetSize(int width, I think you should be able to remove this method completely, as well as last_width_ and last_height_.
I had to update unit tests using SetSize. And I've now deleted FakeVideoRenderer::SetSize, and, by pbos' suggestion, WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetSize. I get a presubmit warning saying that I'm modifying "native API files" talk/app/webrtc/mediastreaminterface.h and talk/app/webrtc/videotrackrenderers.h. Are these really public interfaces? I could send a message to discuss-webrtc@googlegroups.com and webrtc-users@google.com to explain that SetSize is being deleted. https://codereview.webrtc.org/1574963002/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1574963002/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:2494: void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetSize(int width, On 2016/01/11 10:25:01, pbos-webrtc wrote: > I think you should be able to remove this method completely, as well as > last_width_ and last_height_. I'm deleting the SetSize method. But last_width_ and last_height_ are used to populate the VideoReceiverInfo struct, so it's not obvious to me that they are unused.
lgtm
lgtm
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/1574963002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574963002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/11753)
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/1574963002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574963002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Be careful about not breaking the Chrome FYI bots. I noticed that Chrome overrides/implements SetSize in a few places. So you might break the build if you don't do it carefully. https://codereview.webrtc.org/1574963002/diff/60001/talk/media/base/videocapt... File talk/media/base/videocapturer_unittest.cc (right): https://codereview.webrtc.org/1574963002/diff/60001/talk/media/base/videocapt... talk/media/base/videocapturer_unittest.cc:337: // factor of 2. I think you're right, but I don't think your comment adds much. I'd prefer it just be removed. https://codereview.webrtc.org/1574963002/diff/60001/talk/media/base/videoengi... File talk/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/1574963002/diff/60001/talk/media/base/videoengi... talk/media/base/videoengine_unittest.h:459: // TODO(nisse): Delete this test, since we no longer have SetSize? Yes, just remove this.
On 2016/01/11 18:42:08, pthatcher1 wrote:
> Be careful about not breaking the Chrome FYI bots. I noticed that Chrome
> overrides/implements SetSize in a few places. So you might break the build if
> you don't do it carefully.
Codesearch finds the following two chrome matches for
cricket::VideoRenderer::SetSize:
 src/content/renderer/media/webrtc/mock_peer_connection_dependency_factory.cc
(1 occurrence)
167 bool MockVideoRenderer::SetSize(int width, int height, int reserved) {
 src/content/renderer/media/webrtc/mock_peer_connection_dependency_factory.h
(1 occurrence)
27 bool SetSize(int width, int height, int reserved) override;
How do you suggest dealing with that? One way to do it safely could be to
1. Make a chrome cl to drop "override" from the declaration, and land that
first.
2. Land this cl.
3. Make another chrome cl to delete the method from MockVideoRenderer.
But maybe there's some simpler way?
https://codereview.webrtc.org/1574963002/diff/60001/talk/media/base/videocapt... File talk/media/base/videocapturer_unittest.cc (right): https://codereview.webrtc.org/1574963002/diff/60001/talk/media/base/videocapt... talk/media/base/videocapturer_unittest.cc:337: // factor of 2. On 2016/01/11 18:42:08, pthatcher1 wrote: > I think you're right, but I don't think your comment adds much. I'd prefer it > just be removed. Done. https://codereview.webrtc.org/1574963002/diff/60001/talk/media/base/videoengi... File talk/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/1574963002/diff/60001/talk/media/base/videoengi... talk/media/base/videoengine_unittest.h:459: // TODO(nisse): Delete this test, since we no longer have SetSize? On 2016/01/11 18:42:08, pthatcher1 wrote: > Yes, just remove this. Done.
On 2016/01/12 08:00:11, nisse-webrtc wrote: > How do you suggest dealing with that? One way to do it safely could be to > > 1. Make a chrome cl to drop "override" from the declaration, and land that > first. Tried it. Doesn't work. Reason is that it generates a warning about missing "override", and for some reason chrome is built using -Werror. In my not very humble opinion, building with -Werror by default is a very bad idea. Sure, one should pay close attention to warnings, but the fine distinction between an error and a warning is pretty important. :-( Any other ideas on how to make this transition? I guess one could change the plan to 0. Add a default do nothing implementation of cricket::VideoRenderer::SetSize. 1. Delete the method from chrome's MockVideoRenderer. 2. Go on with this cl. I'll prepare cls for (0) and (1).
On 2016/01/12 09:34:22, nisse-webrtc wrote: > On 2016/01/12 08:00:11, nisse-webrtc wrote: > > How do you suggest dealing with that? One way to do it safely could be to > > > > 1. Make a chrome cl to drop "override" from the declaration, and land that > > first. > > Tried it. Doesn't work. Reason is that it generates a warning about missing > "override", and for some reason chrome is built using -Werror. Purpose is to only enable warnings we want and then make them impossible to ignore, e.g. be warning-clean, to keep warnings enforced by style checkers, prevent unsafe casts etc. :) > In my not very humble opinion, building with -Werror by default is a very bad > idea. Sure, one should pay close attention to warnings, but the fine distinction > between an error and a warning is pretty important. :-( > > Any other ideas on how to make this transition? I guess one could change the > plan to > > 0. Add a default do nothing implementation of cricket::VideoRenderer::SetSize. > > 1. Delete the method from chrome's MockVideoRenderer. > > 2. Go on with this cl. > > I'll prepare cls for (0) and (1). Yes, 0, 1, 2 is the sensible approach. Or you can buy someone who rolls webrtc into chromium, but this is the best way to go.
On 2016/01/12 09:59:57, pbos-webrtc wrote: > On 2016/01/12 09:34:22, nisse-webrtc wrote: > > On 2016/01/12 08:00:11, nisse-webrtc wrote: > > > How do you suggest dealing with that? One way to do it safely could be to > > > > > > 1. Make a chrome cl to drop "override" from the declaration, and land that > > > first. > > > > Tried it. Doesn't work. Reason is that it generates a warning about missing > > "override", and for some reason chrome is built using -Werror. > > Purpose is to only enable warnings we want and then make them impossible to > ignore, e.g. be warning-clean, to keep warnings enforced by style checkers, > prevent unsafe casts etc. :) > > > In my not very humble opinion, building with -Werror by default is a very bad > > idea. Sure, one should pay close attention to warnings, but the fine > distinction > > between an error and a warning is pretty important. :-( > > > > Any other ideas on how to make this transition? I guess one could change the > > plan to > > > > 0. Add a default do nothing implementation of cricket::VideoRenderer::SetSize. > > > > 1. Delete the method from chrome's MockVideoRenderer. > > > > 2. Go on with this cl. > > > > I'll prepare cls for (0) and (1). > > Yes, 0, 1, 2 is the sensible approach. Or you can buy someone who rolls webrtc > into chromium, but this is the best way to go. Sorry, buy some candy for someone...
On 2016/01/12 10:00:12, pbos-webrtc wrote: > On 2016/01/12 09:59:57, pbos-webrtc wrote: > > On 2016/01/12 09:34:22, nisse-webrtc wrote: > > > On 2016/01/12 08:00:11, nisse-webrtc wrote: > > > > How do you suggest dealing with that? One way to do it safely could be to > > > > > > > > 1. Make a chrome cl to drop "override" from the declaration, and land that > > > > first. > > > > > > Tried it. Doesn't work. Reason is that it generates a warning about missing > > > "override", and for some reason chrome is built using -Werror. > > > > Purpose is to only enable warnings we want and then make them impossible to > > ignore, e.g. be warning-clean, to keep warnings enforced by style checkers, > > prevent unsafe casts etc. :) > > > > > In my not very humble opinion, building with -Werror by default is a very > bad > > > idea. Sure, one should pay close attention to warnings, but the fine > > distinction > > > between an error and a warning is pretty important. :-( > > > > > > Any other ideas on how to make this transition? I guess one could change the > > > plan to > > > > > > 0. Add a default do nothing implementation of > cricket::VideoRenderer::SetSize. > > > > > > 1. Delete the method from chrome's MockVideoRenderer. > > > > > > 2. Go on with this cl. > > > > > > I'll prepare cls for (0) and (1). > > > > Yes, 0, 1, 2 is the sensible approach. Or you can buy someone who rolls webrtc > > into chromium, but this is the best way to go. > > Sorry, buy some candy for someone... lgtm
Description was changed from ========== Deleted renderer-related SetSize methods, and all uses. webrtc::VideoRendererInterface::SetSize was completely unused. cricket::VideoRenderer::SetSize only had dummy implementations returning true and doing nothing. ========== to ========== Deleted renderer-related SetSize methods, and all uses. webrtc::VideoRendererInterface::SetSize was completely unused. cricket::VideoRenderer::SetSize only had dummy implementations returning true and doing nothing. BUG=webrtc:5426 ==========
On 2016/01/12 10:00:12, pbos-webrtc wrote: > On 2016/01/12 09:59:57, pbos-webrtc wrote: > > On 2016/01/12 09:34:22, nisse-webrtc wrote: > > > On 2016/01/12 08:00:11, nisse-webrtc wrote: > > > > How do you suggest dealing with that? One way to do it safely could be to > > > > > > > > 1. Make a chrome cl to drop "override" from the declaration, and land that > > > > first. > > > > > > Tried it. Doesn't work. Reason is that it generates a warning about missing > > > "override", and for some reason chrome is built using -Werror. > > > > Purpose is to only enable warnings we want and then make them impossible to > > ignore, e.g. be warning-clean, to keep warnings enforced by style checkers, > > prevent unsafe casts etc. :) > > > > > In my not very humble opinion, building with -Werror by default is a very > bad > > > idea. Sure, one should pay close attention to warnings, but the fine > > distinction > > > between an error and a warning is pretty important. :-( > > > > > > Any other ideas on how to make this transition? I guess one could change the > > > plan to > > > > > > 0. Add a default do nothing implementation of > cricket::VideoRenderer::SetSize. > > > > > > 1. Delete the method from chrome's MockVideoRenderer. > > > > > > 2. Go on with this cl. > > > > > > I'll prepare cls for (0) and (1). > > > > Yes, 0, 1, 2 is the sensible approach. Or you can buy someone who rolls webrtc > > into chromium, but this is the best way to go. > > Sorry, buy some candy for someone... Too late :-/ after an unexpected sequence of events, it seems I'm now the property of Nisse. lgtm++
On 2016/01/11 18:42:08, pthatcher1 wrote: > Be careful about not breaking the Chrome FYI bots. I noticed that Chrome > overrides/implements SetSize in a few places. So you might break the build if > you don't do it carefully. Now https://codereview.chromium.org/1584433002/ is landed, deleting chrome's use of SetSize. (Following the little dance where I first added a default implementation in webrtc, https://codereview.webrtc.org/1581583002/). I think there's a reasonable chance that landing this cl won't break chrome.
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/1574963002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574963002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/01/13 08:44:24, nisse-webrtc wrote: > On 2016/01/11 18:42:08, pthatcher1 wrote: > > Be careful about not breaking the Chrome FYI bots. I noticed that Chrome > > overrides/implements SetSize in a few places. So you might break the build if > > you don't do it carefully. > > Now https://codereview.chromium.org/1584433002/ is landed, deleting chrome's use > of SetSize. (Following the little dance where I first added a default > implementation in webrtc, https://codereview.webrtc.org/1581583002/). > > I think there's a reasonable chance that landing this cl won't break chrome. Ping. pthatcher, are you ok with this, or are there any remaining concerns?
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, tommi@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1574963002/#ps90001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574963002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574963002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/4578) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/11861) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/254) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6489)
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, tommi@webrtc.org, pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1574963002/#ps110001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574963002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574963002/110001
Message was sent while issue was closed.
Description was changed from ========== Deleted renderer-related SetSize methods, and all uses. webrtc::VideoRendererInterface::SetSize was completely unused. cricket::VideoRenderer::SetSize only had dummy implementations returning true and doing nothing. BUG=webrtc:5426 ========== to ========== Deleted renderer-related SetSize methods, and all uses. webrtc::VideoRendererInterface::SetSize was completely unused. cricket::VideoRenderer::SetSize only had dummy implementations returning true and doing nothing. BUG=webrtc:5426 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Deleted renderer-related SetSize methods, and all uses. webrtc::VideoRendererInterface::SetSize was completely unused. cricket::VideoRenderer::SetSize only had dummy implementations returning true and doing nothing. BUG=webrtc:5426 ========== to ========== Deleted renderer-related SetSize methods, and all uses. webrtc::VideoRendererInterface::SetSize was completely unused. cricket::VideoRenderer::SetSize only had dummy implementations returning true and doing nothing. BUG=webrtc:5426 Committed: https://crrev.com/c4c84856628c70fada1ada6c4112f5497d993aa6 Cr-Commit-Position: refs/heads/master@{#11298} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c4c84856628c70fada1ada6c4112f5497d993aa6 Cr-Commit-Position: refs/heads/master@{#11298} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
