Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1684423002:
Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. (Closed)
|
Created:
4 years, 10 months ago by nisse-webrtc Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface.
This patch tries to only change the interface to VideoTrack, with
minimal changes to the implementation. Some points worth noting:
VideoTrackRenderers should ultimately be deleted, but it is kept for
now since we need an object implementing webrtc::VideoRenderer, and
that shouldn't be VideoTrack.
BUG=webrtc:5426
TBR=glaznev@webrtc.org // please look at examples
Committed: https://crrev.com/db25d2e8c57b14b14111ee6ab4a5cb6372142155
Cr-Commit-Position: refs/heads/master@{#11775}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed Per's comments. #Patch Set 3 : Rebase on master, copy VideoSourceInterface. #
Total comments: 17
Patch Set 4 : Rebase. #Patch Set 5 : Additional test. Addressed nits. #
Total comments: 2
Patch Set 6 : Fix comment on namespace-confusion. #
Total comments: 10
Patch Set 7 : Delete unused last_frame method and corresponding member. #
Total comments: 1
Messages
Total messages: 34 (12 generated)
nisse@webrtc.org changed reviewers: + perkj@webrtc.org, pthatcher@webrtc.org
Next, I'll try to #if out VideoTrackInterface::AddRenderer, to find out how much it takes to switch from webrtc::VideoRendererInterface to rtc::VideoSinkInterface.
https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/mediastreaminterfa... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/mediastreaminterfa... webrtc/api/mediastreaminterface.h:131: virtual void AddRenderer(VideoRendererInterface* renderer) { Comment that these are deprecated and the new methods should be used please https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/mediastreamtrackpr... File webrtc/api/mediastreamtrackproxy.h (right): https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/mediastreamtrackpr... webrtc/api/mediastreamtrackproxy.h:52: AddOrUpdateSink, did git cl format produce this? https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrack.h File webrtc/api/videotrack.h (right): https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrack.h#newco... webrtc/api/videotrack.h:27: // VideoSourceInterface??? I think you can use using=webrtc::VideoSourceInterface ie class VideoTrack : public MediaStreamTrack<VideoTrackInterface> { public: using=webrtc::VideoSourceInterface; ..... https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrack.h#newco... webrtc/api/videotrack.h:48: // TODO(nisse): And here. dito https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrackrenderer... File webrtc/api/videotrackrenderers.cc (right): https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrackrenderer... webrtc/api/videotrackrenderers.cc:26: broadcaster_.AddOrUpdateSink(sink, wants); broadcaster_ is single threaded. I am afraid remote video frames are delivered on a separate thread. So maybe do that later when we consider how to handle a received stream.
I didn't see anything that Per didn't, so it's good as long as you address all the things Per found.
Description was changed from ========== Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. Based on https://codereview.webrtc.org/1655793003/ This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. VideoTrackRenderers keeps a VideoBroadcaster as a member. It uses a CriticalSection to protect access. If this is a typical use, I think the ThreadChecker should be removed from VideoBroadcaster and left to users who need it. I have to write webrtc::VideoSourceInterface in several places to pick up the old type. I really don't understand why that is necessary within "namespace webrtc {" code. BUG=webrtc:5426 ========== to ========== Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. VideoTrackRenderers keeps a VideoBroadcaster as a member. It uses a CriticalSection to protect access. If this is a typical use, I think the ThreadChecker should be removed from VideoBroadcaster and left to users who need it. I have to write webrtc::VideoSourceInterface in several places to pick up the old type. I really don't understand why that is necessary within "namespace webrtc {" code. BUG=webrtc:5426 ==========
I've addressed Per's comments. Also rebased it on master, now that it no longer uses VideoBroadcaster. It was just the VideoSourceInterface and the fix for the VideoSinkInterface constructor that I needed to copy over. git grep AddRenderer gives 11 matches, should I try to eliminate them all? https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/mediastreaminterfa... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/mediastreaminterfa... webrtc/api/mediastreaminterface.h:131: virtual void AddRenderer(VideoRendererInterface* renderer) { On 2016/02/11 15:30:43, perkj_webrtc wrote: > Comment that these are deprecated and the new methods should be used please Done. https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/mediastreamtrackpr... File webrtc/api/mediastreamtrackproxy.h (right): https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/mediastreamtrackpr... webrtc/api/mediastreamtrackproxy.h:52: AddOrUpdateSink, On 2016/02/11 15:30:43, perkj_webrtc wrote: > did git cl format produce this? Yes. My manual formatting didn't have the first line break. https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrack.h File webrtc/api/videotrack.h (right): https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrack.h#newco... webrtc/api/videotrack.h:27: // VideoSourceInterface??? On 2016/02/11 15:30:43, perkj_webrtc wrote: > I think you can use using=webrtc::VideoSourceInterface > > ie > > class VideoTrack : public MediaStreamTrack<VideoTrackInterface> { > public: > using=webrtc::VideoSourceInterface; > ..... You mean using VideoSourceInterface = webrtc::VideoSourceInterface; ? I'm adding it to VideoTrackInterface (which also uses webrtc::VideoSourceInterface), with a TODO saying that I still don't understand why it's needed. https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrackrenderer... File webrtc/api/videotrackrenderers.cc (right): https://codereview.webrtc.org/1684423002/diff/1/webrtc/api/videotrackrenderer... webrtc/api/videotrackrenderers.cc:26: broadcaster_.AddOrUpdateSink(sink, wants); On 2016/02/11 15:30:44, perkj_webrtc wrote: > broadcaster_ is single threaded. I am afraid remote video frames are delivered > on a separate thread. So maybe do that later when we consider how to handle a > received stream. Ok, I'm going back to not using VideoBroadcaster. Still use VideoSourceInterface, but ignore the wants passed to AddOrUpdateSink.
https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... webrtc/api/mediastreaminterface.h:131: // webrtc::VideoSourceInterface? and why should it not resolve to rtc::VideoSourceInterface< ? https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... webrtc/api/mediastreaminterface.h:146: // mock implementations. TODO(nisse) remove please https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/test/fakevideo... File webrtc/api/test/fakevideotrackrenderer.h (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/test/fakevideo... webrtc/api/test/fakevideotrackrenderer.h:20: : public rtc::VideoSinkInterface<cricket::VideoFrame> { Se should continue testing both until all clients has been updated. ie - android, chrome etc. https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrack_uni... File webrtc/api/videotrack_unittest.cc (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrack_uni... webrtc/api/videotrack_unittest.cc:58: TEST_F(VideoTrackTest, RenderVideo) { please add a new test like this that use the new methods. https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrack_uni... webrtc/api/videotrack_unittest.cc:89: video_track_->RemoveSink(renderer_1.get()); This should not be necessary ? https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrackrend... File webrtc/api/videotrackrenderers.cc (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrackrend... webrtc/api/videotrackrenderers.cc:79: const cricket::VideoFrame& frame) { git cl format ?
https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... webrtc/api/mediastreaminterface.h:131: // webrtc::VideoSourceInterface? On 2016/02/12 14:40:14, perkj_webrtc wrote: > and why should it not resolve to rtc::VideoSourceInterface< ? Because the code is inside a namespace webrtc { ... } block, and there is a declaration in that namespace. And in this file, everything else in rtc seems to need qualification to resolve at all, e.g, dropping rtc:: from nearby use of rtc::VideoSinkInterface gives a compile time error. There's either something about namespaces that I misunderstand (perhaps inherit pulls in some namespace state?), or there's some leaking rtc namespace-related declaration in some header file. So I'd really like to understand what's going on before I delete/improve that comment. https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... webrtc/api/mediastreaminterface.h:146: // mock implementations. On 2016/02/12 14:40:14, perkj_webrtc wrote: > TODO(nisse) remove please Sorry, I don't understand. Are you suggesting that I 1. Remove the comment? 2. Remove the dummy implementation? 3. Add a TODO(nisse) comment about removal? https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/test/fakevideo... File webrtc/api/test/fakevideotrackrenderer.h (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/test/fakevideo... webrtc/api/test/fakevideotrackrenderer.h:20: : public rtc::VideoSinkInterface<cricket::VideoFrame> { On 2016/02/12 14:40:14, perkj_webrtc wrote: > Se should continue testing both until all clients has been updated. ie - > android, chrome etc. Unclear what you suggest... I think it should be possible to leave this class unchanged, until the backwards compatibility methods AddSink/RemoveSink are deleted (VideoRendererInterface inherits VideoSinkInterface). Would that help clients? https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrack_uni... File webrtc/api/videotrack_unittest.cc (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrack_uni... webrtc/api/videotrack_unittest.cc:58: TEST_F(VideoTrackTest, RenderVideo) { On 2016/02/12 14:40:14, perkj_webrtc wrote: > please add a new test like this that use the new methods. Hmm. The only VideoTrack method called directly here is the RemoveSink at the end (which could be RemoveRenderer instead, I don't think it matters much). So then we'd need two different FakeVideoTrackRenderer-style classes, using the old and new interface, and one testcase using each. Right? https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrack_uni... webrtc/api/videotrack_unittest.cc:89: video_track_->RemoveSink(renderer_1.get()); On 2016/02/12 14:40:14, perkj_webrtc wrote: > This should not be necessary ? I tried to purge all calls to the deprecated RemoveRenderer method from the webrtc code. But such changes can be postponed to some later cl, if you prefer. https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrackrend... File webrtc/api/videotrackrenderers.cc (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrackrend... webrtc/api/videotrackrenderers.cc:79: const cricket::VideoFrame& frame) { On 2016/02/12 14:40:14, perkj_webrtc wrote: > git cl format ? I kept the linebreaks here unchanged. I'll run git cl format before the next upload. But it annoys me a little that it wants to format the prototypes for AddOrUpdateSink and RemoveSink differently, I'd prefer consistency.
https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... webrtc/api/mediastreaminterface.h:146: // mock implementations. On 2016/02/15 08:54:48, nisse-webrtc wrote: > On 2016/02/12 14:40:14, perkj_webrtc wrote: > > TODO(nisse) remove please > > Sorry, I don't understand. Are you suggesting that I > > 1. Remove the comment? > > 2. Remove the dummy implementation? > > 3. Add a TODO(nisse) comment about removal? Remove the comment / Dummy default implementations, to not get compilation errors on 146 // mock implementations. https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/test/fakevideo... File webrtc/api/test/fakevideotrackrenderer.h (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/test/fakevideo... webrtc/api/test/fakevideotrackrenderer.h:20: : public rtc::VideoSinkInterface<cricket::VideoFrame> { On 2016/02/15 08:54:48, nisse-webrtc wrote: > On 2016/02/12 14:40:14, perkj_webrtc wrote: > > Se should continue testing both until all clients has been updated. ie - > > android, chrome etc. > > Unclear what you suggest... > > I think it should be possible to leave this class unchanged, until the backwards > compatibility methods AddSink/RemoveSink are deleted (VideoRendererInterface > inherits VideoSinkInterface). Would that help clients? Sorry- I meant that currently clients such as chrome uses AddRenderer - RemoveRenderer - therefor- as long as those methods has not been removed, we should keep the old tests that tests AddRenderer and RemoveRenderer. If we add new methods, AddSink and RemoveSink, there should be tests for them too.
https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/mediastreamint... webrtc/api/mediastreaminterface.h:146: // mock implementations. On 2016/02/15 12:36:27, perkj_webrtc wrote: > On 2016/02/15 08:54:48, nisse-webrtc wrote: > > On 2016/02/12 14:40:14, perkj_webrtc wrote: > > > TODO(nisse) remove please > > > > Sorry, I don't understand. Are you suggesting that I > > > > 1. Remove the comment? > > > > 2. Remove the dummy implementation? > > > > 3. Add a TODO(nisse) comment about removal? > > Remove the comment / Dummy default implementations, to not get compilation > errors on > 146 // mock implementations. Done. https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/test/fakevideo... File webrtc/api/test/fakevideotrackrenderer.h (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/test/fakevideo... webrtc/api/test/fakevideotrackrenderer.h:20: : public rtc::VideoSinkInterface<cricket::VideoFrame> { On 2016/02/15 12:36:27, perkj_webrtc wrote: > On 2016/02/15 08:54:48, nisse-webrtc wrote: > > On 2016/02/12 14:40:14, perkj_webrtc wrote: > > > Se should continue testing both until all clients has been updated. ie - > > > android, chrome etc. > > > > Unclear what you suggest... > > > > I think it should be possible to leave this class unchanged, until the > backwards > > compatibility methods AddSink/RemoveSink are deleted (VideoRendererInterface > > inherits VideoSinkInterface). Would that help clients? > > Sorry- I meant that currently clients such as chrome uses AddRenderer - > RemoveRenderer - therefor- as long as those methods has not been removed, we > should keep the old tests that tests AddRenderer and RemoveRenderer. > If we add new methods, AddSink and RemoveSink, there should be tests for them > too. Made two variants of the VideoTrack.RenderVideo test case and the FakeVideoTrackRenderer class. https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrackrend... File webrtc/api/videotrackrenderers.cc (right): https://codereview.webrtc.org/1684423002/diff/40001/webrtc/api/videotrackrend... webrtc/api/videotrackrenderers.cc:79: const cricket::VideoFrame& frame) { On 2016/02/12 14:40:14, perkj_webrtc wrote: > git cl format ? Done.
lgtm if the comment // TODO(nisse): is resolved in mediastreaminterface.h https://codereview.webrtc.org/1684423002/diff/80001/webrtc/api/mediastreamint... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1684423002/diff/80001/webrtc/api/mediastreamint... webrtc/api/mediastreaminterface.h:129: // TODO(nisse): I don't understand why this is needed, why doesn't You need to resolve this before landing.
https://codereview.webrtc.org/1684423002/diff/80001/webrtc/api/mediastreamint... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1684423002/diff/80001/webrtc/api/mediastreamint... webrtc/api/mediastreaminterface.h:129: // TODO(nisse): I don't understand why this is needed, why doesn't On 2016/02/21 18:16:47, perkj_webrtc wrote: > You need to resolve this before landing. Done, I think I have a rough understanding now. Basically, names of base classes are searched early in the resolution process.
https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/test/fakevide... File webrtc/api/test/fakevideotrackrenderer.h (right): https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/test/fakevide... webrtc/api/test/fakevideotrackrenderer.h:29: last_frame_ = const_cast<cricket::VideoFrame*>(&video_frame); Why do we need a const_cast? Why not just make the type of last_frame_ be const? If it's for comparison only, shouldn't a const value still work? https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/test/fakevide... webrtc/api/test/fakevideotrackrenderer.h:52: class FakeVideoTrackRendererOld : public VideoRendererInterface { When are we going to remove this? It could use a TODO explaining when. https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrack.h File webrtc/api/videotrack.h (right): https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrack.h#... webrtc/api/videotrack.h:26: VideoSourceInterface* source); BTW, can we rename this other VideoSourceInterface to something else so that it doesn't conflict with our newer VideoSourceInterface? I can't think of a good name, though. This is why I wanted to call them producers and consumers :). https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrack_un... File webrtc/api/videotrack_unittest.cc (right): https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrack_un... webrtc/api/videotrack_unittest.cc:98: // providing frames to the source. Uses the old VideoTrack interface When can we switch to the new one and remove the old one? https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrackren... File webrtc/api/videotrackrenderers.cc (right): https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrackren... webrtc/api/videotrackrenderers.cc:27: // first. It seems like the code here is exactly the same as for VideoBroadcaster, except for the ignoring of wants. What are the threading issues?
https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/test/fakevide... File webrtc/api/test/fakevideotrackrenderer.h (right): https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/test/fakevide... webrtc/api/test/fakevideotrackrenderer.h:29: last_frame_ = const_cast<cricket::VideoFrame*>(&video_frame); On 2016/02/25 07:07:30, pthatcher1 wrote: > Why do we need a const_cast? Why not just make the type of last_frame_ be > const? If it's for comparison only, shouldn't a const value still work? I can change last_frame_ to a const pointer in both classes. No idea why it wasn't const earlier. No, it's even better, I can delete it! It was introduced for the test case VideoTrackTest.RenderVideoWithPendingRotation (commit 00c509ad by guoweis a year ago), which was removed by magjed in c2db810b8958588771 half a year later. https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/test/fakevide... webrtc/api/test/fakevideotrackrenderer.h:52: class FakeVideoTrackRendererOld : public VideoRendererInterface { On 2016/02/25 07:07:30, pthatcher1 wrote: > When are we going to remove this? It could use a TODO explaining when. The test should clearly be deleted together with the feature. https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrack.h File webrtc/api/videotrack.h (right): https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrack.h#... webrtc/api/videotrack.h:26: VideoSourceInterface* source); On 2016/02/25 07:07:30, pthatcher1 wrote: > BTW, can we rename this other VideoSourceInterface to something else so that it > doesn't conflict with our newer VideoSourceInterface? I can't think of a good > name, though. This is why I wanted to call them producers and consumers :). I'm afraid the name is in the public API, so no idea if and how to change that. I'd be happy to rename rtc::VideoSourceInterface to rtc::VideoSource when the time comes to drop its template argument. https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrackren... File webrtc/api/videotrackrenderers.cc (right): https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrackren... webrtc/api/videotrackrenderers.cc:27: // first. On 2016/02/25 07:07:30, pthatcher1 wrote: > It seems like the code here is exactly the same as for VideoBroadcaster, except > for the ignoring of wants. > > What are the threading issues? The VideoBroadcaster was written to be single-threaded, which I don't think works here (at least it's a different approach than VideoTrackRenderers implement). I'm not too worried about code duplication now, because (1) this cl is mainly about the interface, and (2) we're discussing pushing the broadcast functionally back to the track's source. See https://docs.google.com/document/d/1mEIw_0uDzyHjL3l8a82WKp6AvgR8Tlwn9JGvhbUjV... https://codereview.webrtc.org/1684423002/diff/120001/webrtc/api/test/fakevide... File webrtc/api/test/fakevideotrackrenderer.h (right): https://codereview.webrtc.org/1684423002/diff/120001/webrtc/api/test/fakevide... webrtc/api/test/fakevideotrackrenderer.h:19: class FakeVideoTrackRenderer Actually, I'm not sure this class adds anything useful over FakeVideoRenderer, all it really does is to automatically remove itself from the track on destruction. Maybe delete? But that's for a different cl, I think.
lgtm https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrack.h File webrtc/api/videotrack.h (right): https://codereview.webrtc.org/1684423002/diff/100001/webrtc/api/videotrack.h#... webrtc/api/videotrack.h:26: VideoSourceInterface* source); It's not really part of the API. It's just an implementation detail. Chrome uses it, so we'd need to carefully update both at the same time. But other than that, I don't see an issue. And having looked at that class, at I figured out what the name should be: VideoTrackSourceInterface, because it's so specific to a VideoTrack. I think we should keep our VideoSourceInterface named VideoSourceInterface. On 2016/02/25 11:42:28, nisse-webrtc wrote: > On 2016/02/25 07:07:30, pthatcher1 wrote: > > BTW, can we rename this other VideoSourceInterface to something else so that > it > > doesn't conflict with our newer VideoSourceInterface? I can't think of a good > > name, though. This is why I wanted to call them producers and consumers :). > > I'm afraid the name is in the public API, so no idea if and how to change that. > I'd be happy to rename rtc::VideoSourceInterface to rtc::VideoSource when the > time comes to drop its template argument.
Description was changed from
==========
Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface.
This patch tries to only change the interface to VideoTrack, with
minimal changes to the implementation. Some points worth noting:
VideoTrackRenderers should ultimately be deleted, but it is kept for
now since we need an object implementing webrtc::VideoRenderer, and
that shouldn't be VideoTrack.
VideoTrackRenderers keeps a VideoBroadcaster as a member. It uses a
CriticalSection to protect access. If this is a typical use, I think
the ThreadChecker should be removed from VideoBroadcaster and left to
users who need it.
I have to write webrtc::VideoSourceInterface in several places to pick
up the old type. I really don't understand why that is necessary
within "namespace webrtc {" code.
BUG=webrtc:5426
==========
to
==========
Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface.
This patch tries to only change the interface to VideoTrack, with
minimal changes to the implementation. Some points worth noting:
VideoTrackRenderers should ultimately be deleted, but it is kept for
now since we need an object implementing webrtc::VideoRenderer, and
that shouldn't be VideoTrack.
BUG=webrtc:5426
==========
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 Link to the patchset: https://codereview.webrtc.org/1684423002/#ps120001 (title: "Delete unused last_frame method and corresponding member.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684423002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3681)
Please have a look at the small changes to webrtc/examples. TBR=glaznev
Please have a look at the small changes to webrtc/examples. TBR=glaznev@webrtc.org (Second attempt, sorry, I'm quite new to this...)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684423002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3685)
Description was changed from ========== Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. BUG=webrtc:5426 ========== to ========== Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. BUG=webrtc:5426 TBR=glaznev@webrtc.org // please look at examples ==========
The CQ bit was checked by perkj@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684423002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684423002/120001
Message was sent while issue was closed.
Description was changed from ========== Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. BUG=webrtc:5426 TBR=glaznev@webrtc.org // please look at examples ========== to ========== Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. BUG=webrtc:5426 TBR=glaznev@webrtc.org // please look at examples ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. BUG=webrtc:5426 TBR=glaznev@webrtc.org // please look at examples ========== to ========== Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. BUG=webrtc:5426 TBR=glaznev@webrtc.org // please look at examples Committed: https://crrev.com/db25d2e8c57b14b14111ee6ab4a5cb6372142155 Cr-Commit-Position: refs/heads/master@{#11775} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/db25d2e8c57b14b14111ee6ab4a5cb6372142155 Cr-Commit-Position: refs/heads/master@{#11775} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
