Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2137503003:
AVFoundation Video Capturer: Remove thread jump when delivering frames (Closed)
|
Created:
4 years, 5 months ago by magjed_webrtc Modified:
4 years, 3 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. |
DescriptionAVFoundation Video Capturer: Remove thread jump when delivering frames
WebRTC no longer has any restriction on what thread frames should be
delivered on. One possible problem with this CL is that NV21->I420
conversion and scaling is done on the thread that delivers frames, which
might cause fps regressions.
R=nisse@webrtc.org, perkj@webrtc.org, tkchin@webrtc.org
Committed: https://crrev.com/0bade0df3b45650c47074f9304e2ddd97b771f93
Cr-Commit-Position: refs/heads/master@{#14021}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Synchronize frame delivery #Patch Set 4 : Remove critical section #
Total comments: 2
Patch Set 5 : Add thread comment in TimestampAligner #
Messages
Total messages: 38 (14 generated)
Description was changed from ========== AVFoundation Video Capturer: Remove thread jump when delivering frames WebRTC no longer has any restriction on what thread frames should be delivered on. ========== to ========== AVFoundation Video Capturer: Remove thread jump when delivering frames WebRTC no longer has any restriction on what thread frames should be delivered on. One possible problem is that NV21->I420 conversion and scaling is done on the thread that delivers frames. ==========
Description was changed from ========== AVFoundation Video Capturer: Remove thread jump when delivering frames WebRTC no longer has any restriction on what thread frames should be delivered on. One possible problem is that NV21->I420 conversion and scaling is done on the thread that delivers frames. ========== to ========== AVFoundation Video Capturer: Remove thread jump when delivering frames WebRTC no longer has any restriction on what thread frames should be delivered on. One possible problem with this CL is that NV21->I420 conversion and scaling is done on the thread that delivers frames, which might cause fps regressions. ==========
magjed@webrtc.org changed reviewers: + tkchin@webrtc.org
tkchin - please take a look.
On 2016/07/12 14:05:02, magjed_webrtc wrote: > tkchin - please take a look. I'm not certain what the effect will be on fps. It is dependent on how the system is deciding to drop frames. If it is dropping frames because the calls to the dispatch queue aren't returning fast enough, then we want the thread hop. If it is dropping frames because we are holding on to CVPixelBuffer from the capturer's pool for too long, then whether or not we thread hop doesn't matter. The goldilocks solution might be to copy bytes and thread hop only if we know that conversions need to happen before it arrives at the encoder, but otherwise just deliver it without thread hopping since minimal work should occur before the frame is handed off async to the encoder. Can you check what the behavior is using VP8? You can do so by commenting out the relevant preferh264 bits in ARDAppClient. It would also be nice to see what difference this makes in terms of CPU performance via Instruments.
On 2016/07/12 22:19:06, tkchin_webrtc wrote: > On 2016/07/12 14:05:02, magjed_webrtc wrote: > > tkchin - please take a look. > > I'm not certain what the effect will be on fps. It is dependent on how the > system is deciding to drop frames. If it is dropping frames because the calls to > the dispatch queue aren't returning fast enough, then we want the thread hop. If > it is dropping frames because we are holding on to CVPixelBuffer from the > capturer's pool for too long, then whether or not we thread hop doesn't matter. > The goldilocks solution might be to copy bytes and thread hop only if we know > that conversions need to happen before it arrives at the encoder, but otherwise > just deliver it without thread hopping since minimal work should occur before > the frame is handed off async to the encoder. > > Can you check what the behavior is using VP8? You can do so by commenting out > the relevant preferh264 bits in ARDAppClient. It would also be nice to see what > difference this makes in terms of CPU performance via Instruments. The only reason to remove the thread hop is to reduce code complexity, no? I think the thread hop itself has negligible CPU performance impact, unless it affects cache hit ratio. I have verified on an iPhone 6+ that AVFoundationVideoCapturer has a pool of CVPixelBuffers and can have multiple outstanding frames at the same time, so it shouldn't drop frames for that reason. I will investigate fps and CPU impact of this change on h264 and vp8 when I have time over, but it's not on top of my priority list, because I don't think it will have a big impact.
On 2016/08/08 14:06:02, magjed_webrtc wrote: > On 2016/07/12 22:19:06, tkchin_webrtc wrote: > > On 2016/07/12 14:05:02, magjed_webrtc wrote: > > > tkchin - please take a look. > > > > I'm not certain what the effect will be on fps. It is dependent on how the > > system is deciding to drop frames. If it is dropping frames because the calls > to > > the dispatch queue aren't returning fast enough, then we want the thread hop. > If > > it is dropping frames because we are holding on to CVPixelBuffer from the > > capturer's pool for too long, then whether or not we thread hop doesn't > matter. > > The goldilocks solution might be to copy bytes and thread hop only if we know > > that conversions need to happen before it arrives at the encoder, but > otherwise > > just deliver it without thread hopping since minimal work should occur before > > the frame is handed off async to the encoder. > > > > Can you check what the behavior is using VP8? You can do so by commenting out > > the relevant preferh264 bits in ARDAppClient. It would also be nice to see > what > > difference this makes in terms of CPU performance via Instruments. > > The only reason to remove the thread hop is to reduce code complexity, no? I > think the thread hop itself has negligible CPU performance impact, unless it > affects cache hit ratio. I have verified on an iPhone 6+ that > AVFoundationVideoCapturer has a pool of CVPixelBuffers and can have multiple > outstanding frames at the same time, so it shouldn't drop frames for that > reason. I will investigate fps and CPU impact of this change on h264 and vp8 > when I have time over, but it's not on top of my priority list, because I don't > think it will have a big impact. lgtm
On 2016/08/08 22:48:41, tkchin_webrtc wrote: > On 2016/08/08 14:06:02, magjed_webrtc wrote: > > On 2016/07/12 22:19:06, tkchin_webrtc wrote: > > > On 2016/07/12 14:05:02, magjed_webrtc wrote: > > > > tkchin - please take a look. > > > > > > I'm not certain what the effect will be on fps. It is dependent on how the > > > system is deciding to drop frames. If it is dropping frames because the > calls > > to > > > the dispatch queue aren't returning fast enough, then we want the thread > hop. > > If > > > it is dropping frames because we are holding on to CVPixelBuffer from the > > > capturer's pool for too long, then whether or not we thread hop doesn't > > matter. > > > The goldilocks solution might be to copy bytes and thread hop only if we > know > > > that conversions need to happen before it arrives at the encoder, but > > otherwise > > > just deliver it without thread hopping since minimal work should occur > before > > > the frame is handed off async to the encoder. > > > > > > Can you check what the behavior is using VP8? You can do so by commenting > out > > > the relevant preferh264 bits in ARDAppClient. It would also be nice to see > > what > > > difference this makes in terms of CPU performance via Instruments. > > > > The only reason to remove the thread hop is to reduce code complexity, no? I > > think the thread hop itself has negligible CPU performance impact, unless it > > affects cache hit ratio. I have verified on an iPhone 6+ that > > AVFoundationVideoCapturer has a pool of CVPixelBuffers and can have multiple > > outstanding frames at the same time, so it shouldn't drop frames for that > > reason. I will investigate fps and CPU impact of this change on h264 and vp8 > > when I have time over, but it's not on top of my priority list, because I > don't > > think it will have a big impact. > > lgtm I would expect that thread hop has impact on old devices e.g 4S. Those do not handle having many threads very well.
any news on this CL?
On 2016/08/25 17:53:40, tkchin_webrtc wrote: > any news on this CL? I will measure the CPU on iPhone6+ with and without this patch, and if it's not worse, I will go ahead and land this. I will probably do this on Tuesday next week.
magjed@webrtc.org changed reviewers: + nisse@webrtc.org
It seems frames are delivered from the OS on multiple threads. I guess it's still one frame at a time, but I added a critical section just in case. nisse - you recently added a thread checker to TimeStampAligner. I had to remove those checks again.
On 2016/08/30 15:27:08, magjed_webrtc wrote: > It seems frames are delivered from the OS on multiple threads. I guess it's > still one frame at a time, but I added a critical section just in case. > > nisse - you recently added a thread checker to TimeStampAligner. I had to remove > those checks again. It is arriving on a serial dispatch queue so you have a guarantee that they will arrive one at a time. Probably don't need the crit section. You can assert on the name of the dispatch queue though. Serial dispatch queues can have multiple threads but will still execute tasks in order.
On 2016/08/30 15:27:08, magjed_webrtc wrote: > It seems frames are delivered from the OS on multiple threads. I guess it's > still one frame at a time, but I added a critical section just in case. > > nisse - you recently added a thread checker to TimeStampAligner. I had to remove > those checks again. Hmm. To be safe, do we need synchronization in each caller?
On 2016/08/30 18:08:00, tkchin_webrtc wrote: > It is arriving on a serial dispatch queue so you have a guarantee that they will > arrive one at a time. Probably don't need the crit section. > You can assert on the name of the dispatch queue though. Serial dispatch queues > can have multiple threads but will still execute tasks in order. I hope they also do whatever magic is needed with memory barriers so memory accesses of tasks are properly ordered?
Patchset #4 (id:60001) has been deleted
> Hmm. To be safe, do we need synchronization in each caller? Yes, the calls to cricket::VideoCapturer::AdaptFrame must be synchronized in each caller. > I hope they also do whatever magic is needed with > memory barriers so memory accesses of tasks are properly > ordered? Yes, they do.
On 2016/08/31 11:26:19, magjed_webrtc wrote: > > Hmm. To be safe, do we need synchronization in each caller? > Yes, the calls to cricket::VideoCapturer::AdaptFrame must be synchronized in > each caller. Do you intend to do that in this cl? There's also AndroidVideoTrackSource::AdaptFrame, which already has a thread checker. Also, if we do this, it might be possible to remove some locking in the VideoAdapter class?
On 2016/08/31 11:38:42, nisse-webrtc wrote: > On 2016/08/31 11:26:19, magjed_webrtc wrote: > > > Hmm. To be safe, do we need synchronization in each caller? > > Yes, the calls to cricket::VideoCapturer::AdaptFrame must be synchronized in > > each caller. > > Do you intend to do that in this cl? The other callers must be synchronized already, because otherwise your thread check wouldn't have worked? And for this caller, we concluded that it's synchronized because it's on a dispatch queue. I need to remove the thread check in TimeStampAligner, because requiring single thread use is too restrictive because of the dispatch queue here. So I need your approval for removing the thread check. Synchronization for other callers is outside the scope of this CL.
lgtm
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2137503003/#ps80001 (title: "Remove critical section")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, no build URL)
magjed@webrtc.org changed reviewers: + perkj@webrtc.org
Per - please take a look. I need your approval for TimeStampAligner.
lgtm if you just add a comment instead. https://codereview.webrtc.org/2137503003/diff/80001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2137503003/diff/80001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:20: class TimestampAligner { Can you please add a comment about the thread assumptions. ie, this class is not thread safe.
https://codereview.webrtc.org/2137503003/diff/80001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2137503003/diff/80001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:20: class TimestampAligner { On 2016/09/01 10:10:22, perkj_webrtc wrote: > Can you please add a comment about the thread assumptions. ie, this class is not > thread safe. Done.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, perkj@webrtc.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2137503003/#ps100001 (title: "Add thread comment in TimestampAligner")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== AVFoundation Video Capturer: Remove thread jump when delivering frames WebRTC no longer has any restriction on what thread frames should be delivered on. One possible problem with this CL is that NV21->I420 conversion and scaling is done on the thread that delivers frames, which might cause fps regressions. ========== to ========== AVFoundation Video Capturer: Remove thread jump when delivering frames WebRTC no longer has any restriction on what thread frames should be delivered on. One possible problem with this CL is that NV21->I420 conversion and scaling is done on the thread that delivers frames, which might cause fps regressions. R=nisse@webrtc.org, perkj@webrtc.org, tkchin@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/0bade0df3b45650c47074f930... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as 0bade0df3b45650c47074f9304e2ddd97b771f93 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== AVFoundation Video Capturer: Remove thread jump when delivering frames WebRTC no longer has any restriction on what thread frames should be delivered on. One possible problem with this CL is that NV21->I420 conversion and scaling is done on the thread that delivers frames, which might cause fps regressions. R=nisse@webrtc.org, perkj@webrtc.org, tkchin@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/0bade0df3b45650c47074f930... ========== to ========== AVFoundation Video Capturer: Remove thread jump when delivering frames WebRTC no longer has any restriction on what thread frames should be delivered on. One possible problem with this CL is that NV21->I420 conversion and scaling is done on the thread that delivers frames, which might cause fps regressions. R=nisse@webrtc.org, perkj@webrtc.org, tkchin@webrtc.org Committed: https://crrev.com/0bade0df3b45650c47074f9304e2ddd97b771f93 Cr-Commit-Position: refs/heads/master@{#14021} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
