43cb716e55eab1cf1ae8afd2ac79b51a604d0fa5
12 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
| 55d932b331 |
Add logging statements to places where the frame might be dropped in WebRTC pipeline.
BUG=b/31645554 Review-Url: https://codereview.webrtc.org/2361803003 Cr-Commit-Position: refs/heads/master@{#14457} |
|||
| 2e82f3821f |
Reland of Split IncomingVideoStream into two implementations, with smoothing and without. (patchset #1 id:1 of https://codereview.webrtc.org/2084873002/ )
Reason for revert: Reverting the revert. This change is not related to the failure on the Windows FYI bots. The cause of the failure has been reverted in Chromium: https://codereview.chromium.org/2081653004/ Original issue's description: > Revert of Split IncomingVideoStream into two implementations, with smoothing and without. (patchset #5 id:340001 of https://codereview.webrtc.org/2078873002/ ) > > Reason for revert: > Breaks chromium.webrtc.fyi > > https://uberchromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Win7%20Tester/builds/4719 > https://uberchromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Win10%20Tester/builds/3120 > > Original issue's description: > > Reland of IncomingVideoStream refactoring. > > This reland does not contain the non-smoothing part of the original implementation. Instead, when smoothing is turned off, frame callbacks run on the decoder thread, as they did before. This code path is used in Chrome. As far as Chrome goes, the difference now is that there won't be an instance of IncomingVideoStream in between the decoder and the callback (i.e. fewer locks). Other than that, no change for Chrome. > > > > Original issue's description (with non-smoothing references removed): > > > > Split IncomingVideoStream into two implementations, with smoothing and without. > > > > * Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 6 locks. > > > > * Removed the Start/Stop methods from the IncomingVideoStream implementations. Now, when an instance is created, it should be considered to be "running" and when it is deleted, it's "not running". This saves on resources and also reduces the amount of locking required and I could remove one critical section altogether. > > > > * Changed the VideoStreamDecoder class to not depend on IncomingVideoStream but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface. This means that any implementation of that interface can be used and the decoder can be made to just use the 'renderer' from the config. Once we do that, we can decouple the IncomingVideoStream implementations from the decoder and VideoReceiveStream implementations and leave it up to the application for how to do smoothing. The app can choose to use the Incoming* classes or roll its own (which may be preferable since applications often have their own scheduling mechanisms). > > > > * The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in VideoReceiveStream and not all of the lifetime of VideoReceiveStream. > > > > * Fixed VideoStreamDecoder to unregister callbacks in the dtor that were registered in the ctor. (this was open to a use-after-free regression) > > > > * Delay and callback pointers are now passed via the ctors to the IncomingVideoStream classes. The thread primitives in the IncomingVideoStream classes are also constructed/destructed at the same time as the owning object, which allowed me to remove one more lock. > > > > * Removed code in the VideoStreamDecoder that could overwrite the VideoReceiveStream render delay with a fixed value of 10ms on construction. This wasn't a problem with the previous implementation (it would be now though) but seemed to me like the wrong place to be setting that value. > > > > * Made the render delay value in VideoRenderFrames, const. > > > > BUG=chromium:620232 > > R=mflodman@webrtc.org, nisse@webrtc.org > > > > Committed: https://crrev.com/884c336c345d988974c2a69cea402b0fb8b07a63 > > Cr-Commit-Position: refs/heads/master@{#13219} > > TBR=nisse@webrtc.org,philipel@webrtc.org,mflodman@webrtc.org,tommi@webrtc.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=chromium:620232 > > Committed: https://crrev.com/a536bfe70de38fe877245317a7f0b00bcf69cbd0 > Cr-Commit-Position: refs/heads/master@{#13229} TBR=nisse@webrtc.org,philipel@webrtc.org,mflodman@webrtc.org,sakal@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=chromium:620232 Review-Url: https://codereview.webrtc.org/2089613002 Cr-Commit-Position: refs/heads/master@{#13230} |
|||
| a536bfe70d |
Revert of Split IncomingVideoStream into two implementations, with smoothing and without. (patchset #5 id:340001 of https://codereview.webrtc.org/2078873002/ )
Reason for revert: Breaks chromium.webrtc.fyi https://uberchromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Win7%20Tester/builds/4719 https://uberchromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Win10%20Tester/builds/3120 Original issue's description: > Reland of IncomingVideoStream refactoring. > This reland does not contain the non-smoothing part of the original implementation. Instead, when smoothing is turned off, frame callbacks run on the decoder thread, as they did before. This code path is used in Chrome. As far as Chrome goes, the difference now is that there won't be an instance of IncomingVideoStream in between the decoder and the callback (i.e. fewer locks). Other than that, no change for Chrome. > > Original issue's description (with non-smoothing references removed): > > Split IncomingVideoStream into two implementations, with smoothing and without. > > * Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 6 locks. > > * Removed the Start/Stop methods from the IncomingVideoStream implementations. Now, when an instance is created, it should be considered to be "running" and when it is deleted, it's "not running". This saves on resources and also reduces the amount of locking required and I could remove one critical section altogether. > > * Changed the VideoStreamDecoder class to not depend on IncomingVideoStream but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface. This means that any implementation of that interface can be used and the decoder can be made to just use the 'renderer' from the config. Once we do that, we can decouple the IncomingVideoStream implementations from the decoder and VideoReceiveStream implementations and leave it up to the application for how to do smoothing. The app can choose to use the Incoming* classes or roll its own (which may be preferable since applications often have their own scheduling mechanisms). > > * The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in VideoReceiveStream and not all of the lifetime of VideoReceiveStream. > > * Fixed VideoStreamDecoder to unregister callbacks in the dtor that were registered in the ctor. (this was open to a use-after-free regression) > > * Delay and callback pointers are now passed via the ctors to the IncomingVideoStream classes. The thread primitives in the IncomingVideoStream classes are also constructed/destructed at the same time as the owning object, which allowed me to remove one more lock. > > * Removed code in the VideoStreamDecoder that could overwrite the VideoReceiveStream render delay with a fixed value of 10ms on construction. This wasn't a problem with the previous implementation (it would be now though) but seemed to me like the wrong place to be setting that value. > > * Made the render delay value in VideoRenderFrames, const. > > BUG=chromium:620232 > R=mflodman@webrtc.org, nisse@webrtc.org > > Committed: https://crrev.com/884c336c345d988974c2a69cea402b0fb8b07a63 > Cr-Commit-Position: refs/heads/master@{#13219} TBR=nisse@webrtc.org,philipel@webrtc.org,mflodman@webrtc.org,tommi@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=chromium:620232 Review-Url: https://codereview.webrtc.org/2084873002 Cr-Commit-Position: refs/heads/master@{#13229} |
|||
| 884c336c34 |
Reland of IncomingVideoStream refactoring.
This reland does not contain the non-smoothing part of the original implementation. Instead, when smoothing is turned off, frame callbacks run on the decoder thread, as they did before. This code path is used in Chrome. As far as Chrome goes, the difference now is that there won't be an instance of IncomingVideoStream in between the decoder and the callback (i.e. fewer locks). Other than that, no change for Chrome. Original issue's description (with non-smoothing references removed): Split IncomingVideoStream into two implementations, with smoothing and without. * Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 6 locks. * Removed the Start/Stop methods from the IncomingVideoStream implementations. Now, when an instance is created, it should be considered to be "running" and when it is deleted, it's "not running". This saves on resources and also reduces the amount of locking required and I could remove one critical section altogether. * Changed the VideoStreamDecoder class to not depend on IncomingVideoStream but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface. This means that any implementation of that interface can be used and the decoder can be made to just use the 'renderer' from the config. Once we do that, we can decouple the IncomingVideoStream implementations from the decoder and VideoReceiveStream implementations and leave it up to the application for how to do smoothing. The app can choose to use the Incoming* classes or roll its own (which may be preferable since applications often have their own scheduling mechanisms). * The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in VideoReceiveStream and not all of the lifetime of VideoReceiveStream. * Fixed VideoStreamDecoder to unregister callbacks in the dtor that were registered in the ctor. (this was open to a use-after-free regression) * Delay and callback pointers are now passed via the ctors to the IncomingVideoStream classes. The thread primitives in the IncomingVideoStream classes are also constructed/destructed at the same time as the owning object, which allowed me to remove one more lock. * Removed code in the VideoStreamDecoder that could overwrite the VideoReceiveStream render delay with a fixed value of 10ms on construction. This wasn't a problem with the previous implementation (it would be now though) but seemed to me like the wrong place to be setting that value. * Made the render delay value in VideoRenderFrames, const. BUG=chromium:620232 R=mflodman@webrtc.org, nisse@webrtc.org Review URL: https://codereview.webrtc.org/2078873002 . Cr-Commit-Position: refs/heads/master@{#13219} |
|||
| 8e8222d0d2 |
Revert of Split IncomingVideoStream into two implementations, with smoothing and without. (patchset #4 id:290001 of https://codereview.webrtc.org/2071473002/ )
Reason for revert: Reverting again. The perf regression does not seem to be related to dropping frames. Original issue's description: > Reland of Split IncomingVideoStream into two implementations, with smoothing and without. > > Original issue's description: > > Split IncomingVideoStream into two implementations, with smoothing and without. > > This CL fixes an issue with the non-smoothing implementation where frames were delivered on the decoder thread. No-smoothing is now done in a separate class that uses a TaskQueue. The implementation may drop frames if the renderer doesn't keep up and it doesn't block the decoder thread. > > Further work done: > > * I added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 5 locks. > > * I removed the Start/Stop methods from the IncomingVideoStream implementations. Now, when an instance is created, it should be considered to be "running" and when it is deleted, it's "not running". This saves on resources and also reduces the amount of locking required and I could remove one critical section altogether. > > * I changed the VideoStreamDecoder class to not depend on IncomingVideoStream but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface. This means that any implementation of that interface can be used and the decoder can be made to just use the 'renderer' from the config. Once we do that, we can decouple the IncomingVideoStream implementations from the decoder and VideoReceiveStream implementations and leave it up to the application for how to do smoothing. The app can choose to use the Incoming* classes or roll its own (which may be preferable since applications often have their own scheduling mechanisms). > > * The non-smoothing IncomingVideoStream implementation currently allows only 1 outstanding pending frame. If we exceed that, the current frame won't be delivered to the renderer and instead we deliver the next one (since when this happens, the renderer is falling behind). > > * The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in VideoReceiveStream and not all of the lifetime of VideoReceiveStream. > > * Fixed VideoStreamDecoder to unregister callbacks in the dtor that were registered in the ctor. (this was open to a use-after-free regression) > > * Delay and callback pointers are now passed via the ctors to the IncomingVideoStream classes. The thread primitives in the IncomingVideoStream classes are also constructed/destructed at the same time as the owning object, which allowed me to remove one more lock. > > * Removed code in the VideoStreamDecoder that could overwrite the VideoReceiveStream render delay with a fixed value of 10ms on construction. This wasn't a problem with the previous implementation (it would be now though) but seemed to me like the wrong place to be setting that value. > > * Made the render delay value in VideoRenderFrames, const. > > BUG=chromium:620232 > TBR=mflodman > > Committed: https://crrev.com/e03f8787377bbc03a4e00184bb14b7561b108cbb > Cr-Commit-Position: refs/heads/master@{#13175} TBR=mflodman@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=chromium:620232 Review-Url: https://codereview.webrtc.org/2071093002 Cr-Commit-Position: refs/heads/master@{#13176} |
|||
| e03f878737 |
Reland of Split IncomingVideoStream into two implementations, with smoothing and without.
Original issue's description: Split IncomingVideoStream into two implementations, with smoothing and without. This CL fixes an issue with the non-smoothing implementation where frames were delivered on the decoder thread. No-smoothing is now done in a separate class that uses a TaskQueue. The implementation may drop frames if the renderer doesn't keep up and it doesn't block the decoder thread. Further work done: * I added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 5 locks. * I removed the Start/Stop methods from the IncomingVideoStream implementations. Now, when an instance is created, it should be considered to be "running" and when it is deleted, it's "not running". This saves on resources and also reduces the amount of locking required and I could remove one critical section altogether. * I changed the VideoStreamDecoder class to not depend on IncomingVideoStream but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface. This means that any implementation of that interface can be used and the decoder can be made to just use the 'renderer' from the config. Once we do that, we can decouple the IncomingVideoStream implementations from the decoder and VideoReceiveStream implementations and leave it up to the application for how to do smoothing. The app can choose to use the Incoming* classes or roll its own (which may be preferable since applications often have their own scheduling mechanisms). * The non-smoothing IncomingVideoStream implementation currently allows only 1 outstanding pending frame. If we exceed that, the current frame won't be delivered to the renderer and instead we deliver the next one (since when this happens, the renderer is falling behind). * The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in VideoReceiveStream and not all of the lifetime of VideoReceiveStream. * Fixed VideoStreamDecoder to unregister callbacks in the dtor that were registered in the ctor. (this was open to a use-after-free regression) * Delay and callback pointers are now passed via the ctors to the IncomingVideoStream classes. The thread primitives in the IncomingVideoStream classes are also constructed/destructed at the same time as the owning object, which allowed me to remove one more lock. * Removed code in the VideoStreamDecoder that could overwrite the VideoReceiveStream render delay with a fixed value of 10ms on construction. This wasn't a problem with the previous implementation (it would be now though) but seemed to me like the wrong place to be setting that value. * Made the render delay value in VideoRenderFrames, const. BUG=chromium:620232 TBR=mflodman Review-Url: https://codereview.webrtc.org/2071473002 Cr-Commit-Position: refs/heads/master@{#13175} |
|||
| 17c3cddf9d |
Revert of Split IncomingVideoStream into two implementations, with smoothing and without. (patchset #23 id:430001 of https://codereview.webrtc.org/2035173002/ )
Reason for revert: Reverting while we track down the issue on the Win10 bot. Original issue's description: > Split IncomingVideoStream into two implementations, with smoothing and without. > > This CL fixes an issue with the non-smoothing implementation where frames were delivered on the decoder thread. No-smoothing is now done in a separate class that uses a TaskQueue. The implementation may drop frames if the renderer doesn't keep up and it doesn't block the decoder thread. > > Further work done: > > * I added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 5 locks. > > * I removed the Start/Stop methods from the IncomingVideoStream implementations. Now, when an instance is created, it should be considered to be "running" and when it is deleted, it's "not running". This saves on resources and also reduces the amount of locking required and I could remove one critical section altogether. > > * I changed the VideoStreamDecoder class to not depend on IncomingVideoStream but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface. This means that any implementation of that interface can be used and the decoder can be made to just use the 'renderer' from the config. Once we do that, we can decouple the IncomingVideoStream implementations from the decoder and VideoReceiveStream implementations and leave it up to the application for how to do smoothing. The app can choose to use the Incoming* classes or roll its own (which may be preferable since applications often have their own scheduling mechanisms). > > * The non-smoothing IncomingVideoStream implementation currently allows only 1 outstanding pending frame. If we exceed that, the current frame won't be delivered to the renderer and instead we deliver the next one (since when this happens, the renderer is falling behind). > > * The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in VideoReceiveStream and not all of the lifetime of VideoReceiveStream. > > * Fixed VideoStreamDecoder to unregister callbacks in the dtor that were registered in the ctor. (this was open to a use-after-free regression) > > * Delay and callback pointers are now passed via the ctors to the IncomingVideoStream classes. The thread primitives in the IncomingVideoStream classes are also constructed/destructed at the same time as the owning object, which allowed me to remove one more lock. > > * Removed code in the VideoStreamDecoder that could overwrite the VideoReceiveStream render delay with a fixed value of 10ms on construction. This wasn't a problem with the previous implementation (it would be now though) but seemed to me like the wrong place to be setting that value. > > * Made the render delay value in VideoRenderFrames, const. > > BUG= > > Committed: https://crrev.com/1c7eef652b0aa22d8ebb0bfe2b547094a794be22 > Cr-Commit-Position: refs/heads/master@{#13129} TBR=mflodman@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Review-Url: https://codereview.webrtc.org/2061363002 Cr-Commit-Position: refs/heads/master@{#13146} |
|||
| 1c7eef652b |
Split IncomingVideoStream into two implementations, with smoothing and without.
This CL fixes an issue with the non-smoothing implementation where frames were delivered on the decoder thread. No-smoothing is now done in a separate class that uses a TaskQueue. The implementation may drop frames if the renderer doesn't keep up and it doesn't block the decoder thread. Further work done: * I added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 5 locks. * I removed the Start/Stop methods from the IncomingVideoStream implementations. Now, when an instance is created, it should be considered to be "running" and when it is deleted, it's "not running". This saves on resources and also reduces the amount of locking required and I could remove one critical section altogether. * I changed the VideoStreamDecoder class to not depend on IncomingVideoStream but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface. This means that any implementation of that interface can be used and the decoder can be made to just use the 'renderer' from the config. Once we do that, we can decouple the IncomingVideoStream implementations from the decoder and VideoReceiveStream implementations and leave it up to the application for how to do smoothing. The app can choose to use the Incoming* classes or roll its own (which may be preferable since applications often have their own scheduling mechanisms). * The non-smoothing IncomingVideoStream implementation currently allows only 1 outstanding pending frame. If we exceed that, the current frame won't be delivered to the renderer and instead we deliver the next one (since when this happens, the renderer is falling behind). * The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in VideoReceiveStream and not all of the lifetime of VideoReceiveStream. * Fixed VideoStreamDecoder to unregister callbacks in the dtor that were registered in the ctor. (this was open to a use-after-free regression) * Delay and callback pointers are now passed via the ctors to the IncomingVideoStream classes. The thread primitives in the IncomingVideoStream classes are also constructed/destructed at the same time as the owning object, which allowed me to remove one more lock. * Removed code in the VideoStreamDecoder that could overwrite the VideoReceiveStream render delay with a fixed value of 10ms on construction. This wasn't a problem with the previous implementation (it would be now though) but seemed to me like the wrong place to be setting that value. * Made the render delay value in VideoRenderFrames, const. BUG= Review-Url: https://codereview.webrtc.org/2035173002 Cr-Commit-Position: refs/heads/master@{#13129} |
|||
| bd3380ff7e |
Make VideoReceiveStream not inherit from I420FrameCallback.
There's no need for it as the current implementation only exists as a middle layer between the decoder and the eventual callback. R=mflodman@webrtc.org, pbos@webrtc.org Review URL: https://codereview.webrtc.org/2039053002 . Cr-Commit-Position: refs/heads/master@{#13101} |
|||
| ae284089cc |
Jitter delay now depend on protection mode (FEC/NACK).
R=stefan@webrtc.org Review URL: https://codereview.webrtc.org/1942683003 . Cr-Commit-Position: refs/heads/master@{#12661} |
|||
| dc7d0d2ef0 |
Move, almost, all receive side references to RTP to RtpStreamReceiver.
There are still a few places in VideoReceiveStream where the RTP module is explicitly used, e.g. setting up a/v sync, but it's a bigger task to change and that will be done in a follow up instead of in this CL. BUG=webrtc:5838 Review-Url: https://codereview.webrtc.org/1947913002 Cr-Commit-Position: refs/heads/master@{#12642} |
|||
| cfc8e3b9ef |
Removed all RTP dependencies from ViEChannel and renamed class.
ViEChannel is now called VideoStreamReceiver. There will be a follow up CL removing all rtp references from VideoReceiveStream, but that made this CL to big and it will be done separately. BUG=webrtc:5079 Review-Url: https://codereview.webrtc.org/1929313002 Cr-Commit-Position: refs/heads/master@{#12619} |
