Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1575223003:
Added mute logic to VideoTrackRenderers. (Closed)
|
Created:
4 years, 11 months ago by nisse-webrtc Modified:
4 years, 11 months ago CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded mute logic to VideoTrackRenderers.
If the track is disabled, replace incoming frames by black frames. Affects local rendering of disabled tracks.
Also intended to replace the similar logic in
WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame, once
VideoRtpSender is hooked up as a renderer for the track.
BUG=webrtc:5426
Committed: https://crrev.com/81354f5ef6b8250c6b370bb17d8447fdf24da00b
Cr-Commit-Position: refs/heads/master@{#11297}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Stack-allocate the black frame. #Patch Set 3 : Added unit test. #
Total comments: 3
Patch Set 4 : Leave critical section while generating the black frame. #
Total comments: 1
Patch Set 5 : Comment on potential frame reorder #Patch Set 6 : Updated PeerConnectionFactoryTest.LocalRendering #Patch Set 7 : Added size_t to int cast, needed for windows compile #Patch Set 8 : Rebase #
Messages
Total messages: 50 (21 generated)
nisse@webrtc.org changed reviewers: + perkj@webrtc.org, pthatcher@webrtc.org
I think this should go into the VideoTrack instead if a track is disabled. Similar to Chrome. That will lead to that black frames can still be send on a PC and rendered and recorded... https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
On 2016/01/13 08:28:25, perkj_webrtc wrote: > I think this should go into the VideoTrack instead if a track is disabled. > Similar to Chrome. That will lead to that black frames can still be send on a PC > and rendered and recorded... > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... I'd prefer to merge VideoTrack and VideoTrackRenderers, and also make VideoTrack implement webrtc::VideoRendererInterface, but that's a later step. Currently, it's VideoTrackRenderers which implements the cricket::VideoRenderer interface, and VideoTrack never gets to see any frames. So I don't think it's possible to move the black frame logic to VideoTrack now.
Please ignore my previous comments. I was confused by the name VideoRenderers. This do what I suggested. https://codereview.webrtc.org/1575223003/diff/1/talk/app/webrtc/videotrackren... File talk/app/webrtc/videotrackrenderers.cc (right): https://codereview.webrtc.org/1575223003/diff/1/talk/app/webrtc/videotrackren... talk/app/webrtc/videotrackrenderers.cc:58: static cricket::VideoFrame* CreateBlackFrame(const cricket::VideoFrame* frame) { static methods should be at the top of the file. And prefer anonymouse ns to static. https://codereview.webrtc.org/1575223003/diff/1/talk/app/webrtc/videotrackren... talk/app/webrtc/videotrackrenderers.cc:59: cricket::VideoFrame* black = new cricket::WebRtcVideoFrame( This looks like a memory leak. Ownership is not transfered to the renderers. You will have to have this black frame as a member I think.
https://codereview.webrtc.org/1575223003/diff/1/talk/app/webrtc/videotrackren... File talk/app/webrtc/videotrackrenderers.cc (right): https://codereview.webrtc.org/1575223003/diff/1/talk/app/webrtc/videotrackren... talk/app/webrtc/videotrackrenderers.cc:59: cricket::VideoFrame* black = new cricket::WebRtcVideoFrame( On 2016/01/13 09:44:40, perkj_webrtc wrote: > This looks like a memory leak. Ownership is not transfered to the renderers. > You will have to have this black frame as a member I think. The VideoFrameBuffer is reference counted, but I have to delete the cricket::VideoFrame? When is that safe? As soon as the downstream RenderFrame calls return? Then I can just allocate the object on the stack instead. I was also thinking about caching the VideoFrameBuffer and keep using it as long as the size is unchanged. But that might be a premature optimzation.
On 2016/01/13 10:33:16, nisse-webrtc wrote: > https://codereview.webrtc.org/1575223003/diff/1/talk/app/webrtc/videotrackren... > File talk/app/webrtc/videotrackrenderers.cc (right): > > https://codereview.webrtc.org/1575223003/diff/1/talk/app/webrtc/videotrackren... > talk/app/webrtc/videotrackrenderers.cc:59: cricket::VideoFrame* black = new > cricket::WebRtcVideoFrame( > On 2016/01/13 09:44:40, perkj_webrtc wrote: > > This looks like a memory leak. Ownership is not transfered to the renderers. > > You will have to have this black frame as a member I think. > > The VideoFrameBuffer is reference counted, but I have to delete the > cricket::VideoFrame? When is that safe? As soon as the downstream RenderFrame > calls return? Then I can just allocate the object on the stack instead. > > I was also thinking about caching the VideoFrameBuffer and keep using it as long > as the size is unchanged. But that might be a premature optimzation. Changed to use stack allocation. I'll look into unit tests next.
lgtm https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... File talk/app/webrtc/videotrack_unittest.cc (right): https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... talk/app/webrtc/videotrack_unittest.cc:119: video_track_->GetSource()->FrameInput(); FrameInput it pretty horrible. But nice for testing. Anyway , something to remove one day...
I think this is the wrong place to do this. Whether we send black or something real is really up to the VideoRtpSender (currently in VideoRtpSender::SetVideoSend), not the track. It's just the case that right now it uses track_->enabled(). But that may change. Similarly, WebRtcVideoChannel2::WebRtcVideoSendStream::SetCapturer also produces black frames when the capturer goes to a NULL capturer. That would also make sense to move to VideoRtpSender. I also think it would make more sense to produce one black frame when it goes to null/inactive, and not at 30hz. We could implement this in VideoRtpSender in one of three ways: 1. Have the VideoRtpSender create black video frames and send them via the new SendFrame (the one you implemented). 2. Have a SendBlackFrame method on WebrtcMediaChannel2. I think I like 2 > 1. Another problem with putting this on VideoRtpReceives is that it affects not just video senders, but also video receivers. https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... File talk/app/webrtc/videotrackrenderers.cc (right): https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... talk/app/webrtc/videotrackrenderers.cc:65: frame->GetVideoRotation()); Why does frame generation have to happen in the critical_section_? Wouldn't it make more sense to do it outside?
On 2016/01/13 17:13:38, pthatcher1 wrote: > I think this is the wrong place to do this. Whether we send black or something > real is really up to the VideoRtpSender (currently in > VideoRtpSender::SetVideoSend), not the track. It's just the case that right now > it uses track_->enabled(). But that may change. > > Similarly, WebRtcVideoChannel2::WebRtcVideoSendStream::SetCapturer also produces > black frames when the capturer goes to a NULL capturer. That would also make > sense to move to VideoRtpSender. > > I also think it would make more sense to produce one black frame when it goes to > null/inactive, and not at 30hz. > > We could implement this in VideoRtpSender in one of three ways: > 1. Have the VideoRtpSender create black video frames and send them via the new > SendFrame (the one you implemented). > 2. Have a SendBlackFrame method on WebrtcMediaChannel2. > > I think I like 2 > 1. > > > Another problem with putting this on VideoRtpReceives is that it affects not > just video senders, but also video receivers. > > https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... > File talk/app/webrtc/videotrackrenderers.cc (right): > > https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... > talk/app/webrtc/videotrackrenderers.cc:65: frame->GetVideoRotation()); > Why does frame generation have to happen in the critical_section_? Wouldn't it > make more sense to do it outside? Sorry, by "Another problem with putting this on VideoRtpReceives ...", I meant "Another problem with putting this on VideoTrackRenderers ...".
On 2016/01/13 17:14:31, pthatcher1 wrote: > On 2016/01/13 17:13:38, pthatcher1 wrote: > > I think this is the wrong place to do this. Whether we send black or > something > > real is really up to the VideoRtpSender (currently in > > VideoRtpSender::SetVideoSend), not the track. It's just the case that right > now > > it uses track_->enabled(). But that may change. > > > > Similarly, WebRtcVideoChannel2::WebRtcVideoSendStream::SetCapturer also > produces > > black frames when the capturer goes to a NULL capturer. That would also make > > sense to move to VideoRtpSender. > > > > I also think it would make more sense to produce one black frame when it goes > to > > null/inactive, and not at 30hz. > > > > We could implement this in VideoRtpSender in one of three ways: > > 1. Have the VideoRtpSender create black video frames and send them via the > new > > SendFrame (the one you implemented). > > 2. Have a SendBlackFrame method on WebrtcMediaChannel2. > > > > I think I like 2 > 1. > > > > > > Another problem with putting this on VideoRtpReceives is that it affects not > > just video senders, but also video receivers. > > > > > https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... > > File talk/app/webrtc/videotrackrenderers.cc (right): > > > > > https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... > > talk/app/webrtc/videotrackrenderers.cc:65: frame->GetVideoRotation()); > > Why does frame generation have to happen in the critical_section_? Wouldn't > it > > make more sense to do it outside? > > Sorry, by "Another problem with putting this on VideoRtpReceives ...", I meant > "Another problem with putting this on VideoTrackRenderers ...". Peter - The spec kind of say that a disabled track should produce black frames. http://www.w3.org/TR/mediacapture-streams/#track-enabled and that is what we do in chrome and it would also make sure that local preview would render black frames. But if you prefer, I have never understood why one frame is not enough. Do we still want to send a black frame if a track is removed from VideoRtpSender? It seems like we should create a black frame in VideoRtpSender as well in that case.
> Peter - The spec kind of say that a disabled track should produce black frames. > http://www.w3.org/TR/mediacapture-streams/#track-enabled and that is what we do > in chrome and it would also make sure that local preview would render black > frames. > But if you prefer, I have never understood why one frame is not enough. I think the key question is what the enabled state on a VideoTrack really means. I have a very fuzzy understanding of that. But it's VideoTrack's responsibility to implement that state, whatever it means.
https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... File talk/app/webrtc/videotrackrenderers.cc (right): https://codereview.webrtc.org/1575223003/diff/40001/talk/app/webrtc/videotrac... talk/app/webrtc/videotrackrenderers.cc:65: frame->GetVideoRotation()); On 2016/01/13 17:13:38, pthatcher1 wrote: > Why does frame generation have to happen in the critical_section_? Wouldn't it > make more sense to do it outside? Not sure, but I think it's desirable to have it consistent with the enabled state. I.e., rule out the sequence 0. Start with enabled_ == false 1. Enter SetEnabled(true) 2. Enter RenderFrame. Still enabled_ == false, so start generating a black frame. 3. The SetEnabled call sets enabled_ = true, and returns. 4. RenderFrame passes on the black frame to the renderers, even though the track is supposedly enabled now. But I see it's desirable to do the frame generation outside of the critical section. It just needs a little more logic (after the frame is ready, enter the critical section again, and recheck enabled_) to ensure consistency.
Now leaving the critical section while generating the black frame. RenderFrame isn't quite reentrant, frames might be reordered in corner cases if it is called again before the first call returns, and also the track is enabled at the wrong time. (Most likely no problem, but this change made me aware of it).
pthatcher@google.com changed reviewers: + pthatcher@google.com
lgtm https://codereview.webrtc.org/1575223003/diff/60001/talk/app/webrtc/videotrac... File talk/app/webrtc/videotrackrenderers.cc (right): https://codereview.webrtc.org/1575223003/diff/60001/talk/app/webrtc/videotrac... talk/app/webrtc/videotrackrenderers.cc:59: rtc::CritScope cs(&critical_section_); Can you put a comment that this may result in out-of-order frames if RenderFrame is called on different threads and SetEnabled(true) is called right in the middle of two RenderFrame calls? It likely doesn't matter at all since it's highly unlikely we'll be producing frames on different threads, but it's probably worth leaving a warning/comment.
On 2016/01/14 19:23:09, pthatcher wrote: > lgtm > > https://codereview.webrtc.org/1575223003/diff/60001/talk/app/webrtc/videotrac... > File talk/app/webrtc/videotrackrenderers.cc (right): > > https://codereview.webrtc.org/1575223003/diff/60001/talk/app/webrtc/videotrac... > talk/app/webrtc/videotrackrenderers.cc:59: rtc::CritScope > cs(&critical_section_); > Can you put a comment that this may result in out-of-order frames if RenderFrame > is called on different threads and SetEnabled(true) is called right in the > middle of two RenderFrame calls? > > It likely doesn't matter at all since it's highly unlikely we'll be producing > frames on different threads, but it's probably worth leaving a warning/comment. I'll can add a brief comment on this. But on second thought, my understanding is that if RenderFrame is called on different frames, then the only way to get a well defined frame order is for the callers to coordinate and serialize their calls, so that when thread A has calls RenderFrame, thread B doesn't call it until the call on thread A has returned. Right? We can have a thread switch not only when we release the critical section in the middle of RenderFrame, we could also have a thread switch directly at entry to the function, before we grab the critical section. Even more unlikely to happen, but possible and with the same reordering effect.
Description was changed from ========== Added mute logic to VideoTrackRenderers. If the track is disabled, replace incoming frames by black frames. Intended to replace the similar logic in WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame. BUG=webrtc:5426 ========== to ========== Added mute logic to VideoTrackRenderers. If the track is disabled, replace incoming frames by black frames. Affects local rendering of disabled tracks. Also intended to replace the similar logic in WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame, once VideoRtpSender is hooked up as a renderer for the track. 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, pthatcher@google.com Link to the patchset: https://codereview.webrtc.org/1575223003/#ps80001 (title: "Comment on potential frame reorder")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575223003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575223003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/11893) linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/12119)
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, pthatcher@google.com Link to the patchset: https://codereview.webrtc.org/1575223003/#ps100001 (title: "Updated PeerConnectionFactoryTest.LocalRendering")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575223003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575223003/100001
The CQ bit was unchecked by commit-bot@chromium.org
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/11918)
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, pthatcher@google.com Link to the patchset: https://codereview.webrtc.org/1575223003/#ps120001 (title: "Added size_t to int cast, needed for windows compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575223003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575223003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/6792) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12036)
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/1575223003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575223003/120001
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, pthatcher@google.com Link to the patchset: https://codereview.webrtc.org/1575223003/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575223003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575223003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/10387)
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/1575223003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575223003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kjellander@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1575223003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1575223003/140001
Message was sent while issue was closed.
Description was changed from ========== Added mute logic to VideoTrackRenderers. If the track is disabled, replace incoming frames by black frames. Affects local rendering of disabled tracks. Also intended to replace the similar logic in WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame, once VideoRtpSender is hooked up as a renderer for the track. BUG=webrtc:5426 ========== to ========== Added mute logic to VideoTrackRenderers. If the track is disabled, replace incoming frames by black frames. Affects local rendering of disabled tracks. Also intended to replace the similar logic in WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame, once VideoRtpSender is hooked up as a renderer for the track. 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 ========== Added mute logic to VideoTrackRenderers. If the track is disabled, replace incoming frames by black frames. Affects local rendering of disabled tracks. Also intended to replace the similar logic in WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame, once VideoRtpSender is hooked up as a renderer for the track. BUG=webrtc:5426 ========== to ========== Added mute logic to VideoTrackRenderers. If the track is disabled, replace incoming frames by black frames. Affects local rendering of disabled tracks. Also intended to replace the similar logic in WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame, once VideoRtpSender is hooked up as a renderer for the track. BUG=webrtc:5426 Committed: https://crrev.com/81354f5ef6b8250c6b370bb17d8447fdf24da00b Cr-Commit-Position: refs/heads/master@{#11297} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/81354f5ef6b8250c6b370bb17d8447fdf24da00b Cr-Commit-Position: refs/heads/master@{#11297} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
