Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887673002:
Add RTCFileVideoCapturer class. (Closed)
|
Created:
3 years, 7 months ago by daniela-webrtc Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd RTCFileVideoCapturer class.
- Reads and dispatches buffers from a video file, along the lines of
camera capturer.
- Initial purpose of this class will be for testing.
BUG=webrtc:7581
Review-Url: https://codereview.webrtc.org/2887673002
Cr-Commit-Position: refs/heads/master@{#18412}
Committed: https://chromium.googlesource.com/external/webrtc/+/0d4d57f26d7974e1832d69dabd05c57567553882
Patch Set 1 #Patch Set 2 : Make file capturer ios only. - It is not needed for mac. #
Total comments: 9
Patch Set 3 : Fix typos #Patch Set 4 : Fix stopCapture method #
Total comments: 2
Patch Set 5 : Move all operations in dispatch block #
Total comments: 10
Patch Set 6 : Replace while loop and semaphore with post mechanism #Patch Set 7 : Use fullrange pixel format #
Total comments: 11
Patch Set 8 : Fix typo #Patch Set 9 : Fix casting and add return #Patch Set 10 : Rebase #
Messages
Total messages: 39 (19 generated)
The CQ bit was checked by denicija@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/21502)
The CQ bit was checked by denicija@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@webrtc.org, sakal@webrtc.org
PTAL
https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:16: AVAssetReader *_reader; This seems to be accessed from multiple threads. Is it safe? https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:124: - (void)dealoc { typo
https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:230: "objc/Framework/Classes/RTCFileVideoCapturer.m", Is this iOS only or can it compile on mac as well? https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:24: RTCLog("Capturer exists and reads anoter file. Start capture request failed."); typo nit: reads another file. https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:85: CFRelease(sampleBuffer); no need to release the pixel buffer?
https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:230: "objc/Framework/Classes/RTCFileVideoCapturer.m", On 2017/05/19 12:18:47, kthelgason wrote: > Is this iOS only or can it compile on mac as well? Can't compile on Mac :( https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:16: AVAssetReader *_reader; On 2017/05/19 11:53:16, sakal wrote: > This seems to be accessed from multiple threads. Is it safe? This kind of usage with dispatch queues should be sage enough so no need to make it atomic. https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:85: CFRelease(sampleBuffer); On 2017/05/19 12:18:47, kthelgason wrote: > no need to release the pixel buffer? No. The documentation for CMSampleBufferGetImageBuffer states @discussion The caller does not own the returned dataBuffer, and must retain it explicitly if the caller needs to maintain a reference to it.
sakal@google.com changed reviewers: + sakal@google.com
https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:16: AVAssetReader *_reader; On 2017/05/19 13:15:09, daniela-webrtc wrote: > On 2017/05/19 11:53:16, sakal wrote: > > This seems to be accessed from multiple threads. Is it safe? > > This kind of usage with dispatch queues should be sage enough so no need to make > it atomic. The main thing I am worried about is calling startCapturing immediately followed by stopCapture. Is possible that the code setting the reader variable has not been executed yet when stopCapture is called?
Description was changed from ========== Add RTCFileVideoCapturer class. - Reads and dispatches buffers from a video file, along the lines of camera capturer. - Initial purpose of this class will be for testing. BUG=webrtc:7581 ========== to ========== Add RTCFileVideoCapturer class. - Reads and dispatches buffers from a video file, along the lines of camera capturer. - Initial purpose of this class will be for testing. BUG=webrtc:7581 ==========
sakal@google.com changed reviewers: - sakal@google.com
On 2017/05/19 13:20:04, sakal_google.com wrote: > https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): > > https://codereview.webrtc.org/2887673002/diff/20001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:16: AVAssetReader > *_reader; > On 2017/05/19 13:15:09, daniela-webrtc wrote: > > On 2017/05/19 11:53:16, sakal wrote: > > > This seems to be accessed from multiple threads. Is it safe? > > > > This kind of usage with dispatch queues should be sage enough so no need to > make > > it atomic. > > The main thing I am worried about is calling startCapturing immediately followed > by stopCapture. Is possible that the code setting the reader variable has not > been executed yet when stopCapture is called? You are right. I also took a look at the documentation of the cancelReading and it's not intended to be used as I used it. I've changed the implementation and added documentation.
https://codereview.webrtc.org/2887673002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:24: if (_reader && _reader.status == AVAssetReaderStatusReading) { Can this be moved inside the dispatch, then _reader would only be accessed on a single thread?
https://codereview.webrtc.org/2887673002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:24: if (_reader && _reader.status == AVAssetReaderStatusReading) { On 2017/05/23 09:37:18, sakal wrote: > Can this be moved inside the dispatch, then _reader would only be accessed on a > single thread? Done.
lgtm https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:20: BOOL _capturerStopped; I think this should be a property declared as atomic or marked as volatile.
https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:26: RTCLog("Capturer exists and reads anoth6er file. Start capture request failed."); nit: anoth6er https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:51: kCVPixelBufferPixelFormatTypeKey : @(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) Is it possible to use FullRange? https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:79: // dispatch with delay, we want to achieve a real time play. nit: Begin sentence with capital letter. https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:89: dispatch_semaphore_signal(_frameSemaphore); I would like to only use one frame queue and avoid blocking, as we discussed offline.
https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:20: BOOL _capturerStopped; On 2017/05/24 08:34:09, sakal wrote: > I think this should be a property declared as atomic or marked as volatile. Not necessarily. The worst thing that can happen is that few frames slip by but it's not worth paying the price of locking the variable. https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:26: RTCLog("Capturer exists and reads anoth6er file. Start capture request failed."); On 2017/05/29 13:14:25, magjed_webrtc wrote: > nit: anoth6er Done. https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:51: kCVPixelBufferPixelFormatTypeKey : @(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) On 2017/05/29 13:14:25, magjed_webrtc wrote: > Is it possible to use FullRange? Yes. I've changed it https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:89: dispatch_semaphore_signal(_frameSemaphore); On 2017/05/29 13:14:25, magjed_webrtc wrote: > I would like to only use one frame queue and avoid blocking, as we discussed > offline. I've removed the semaphore and the blocking and now the buffers are posted to the frame queue. However I still want to keep the frameQueue separate and perform the rest of the tasks on separate queue. It's cleaner approach and is inline with what other capturers (for instance the camera one) would do.
Thanks for doing the work to remove the lock! I prefer this approach. I have some small comments before stamping. https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:89: dispatch_semaphore_signal(_frameSemaphore); On 2017/06/01 13:08:57, daniela-webrtc wrote: > On 2017/05/29 13:14:25, magjed_webrtc wrote: > > I would like to only use one frame queue and avoid blocking, as we discussed > > offline. > > I've removed the semaphore and the blocking and now the buffers are posted to > the frame queue. However I still want to keep the frameQueue separate and > perform the rest of the tasks on separate queue. It's cleaner approach and is > inline with what other capturers (for instance the camera one) would do. Sure, we can have a separate frameQueue to publish frames on, as long as we don't synchronize the queues by blocking. https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:26: RTCLog("Capturer exists and reads anoth6er file. Start capture request failed."); nit: still anoth6er https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:128: if (!pixelBuffer) { Maybe we can extract the CVPixelBufferRef and do this check in readNextBuffer instead, and publish the final pixelBuffer? https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:132: }); We should return here I guess? https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:136: int64_t timeStampNs = timeStampSeconds * NSEC_PER_SEC; Should we use lroundf here? https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:139: CFRelease(sampleBuffer); I noticed we don't do 'CFRelease(sampleBuffer);' in RTCCameraVideoCapturer.m. Is that something we missed?
https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:26: RTCLog("Capturer exists and reads anoth6er file. Start capture request failed."); On 2017/06/01 13:44:37, magjed_webrtc wrote: > nit: still anoth6er The patch didn't upload properly :D Should be fixed now. https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:128: if (!pixelBuffer) { On 2017/06/01 13:44:37, magjed_webrtc wrote: > Maybe we can extract the CVPixelBufferRef and do this check in readNextBuffer > instead, and publish the final pixelBuffer? We need to release the sampleBuffer after it was used to create the frame so passing the CVPixelBuffer would cause us to leak the sampleBuffer. And we need to create the frame in the block because we need to use the correct timestamp. https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:132: }); On 2017/06/01 13:44:38, magjed_webrtc wrote: > We should return here I guess? Done. https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:136: int64_t timeStampNs = timeStampSeconds * NSEC_PER_SEC; On 2017/06/01 13:44:37, magjed_webrtc wrote: > Should we use lroundf here? Yes. NSTimeInteval is double. https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:139: CFRelease(sampleBuffer); On 2017/06/01 13:44:37, magjed_webrtc wrote: > I noticed we don't do 'CFRelease(sampleBuffer);' in RTCCameraVideoCapturer.m. Is > that something we missed? We need to explicitly release the sampleBuffer here because of the way it is obtained in line 96. From the documentation of copyNextSampleBuffer "The client is responsible for calling CFRelease on the returned CMSampleBuffer object when finished with it" That's not the case in the RTCCameraVideoCapturer where we don't own the buffer passed to us by the delegate callback.
https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:128: if (!pixelBuffer) { On 2017/06/01 14:13:27, daniela-webrtc wrote: > On 2017/06/01 13:44:37, magjed_webrtc wrote: > > Maybe we can extract the CVPixelBufferRef and do this check in readNextBuffer > > instead, and publish the final pixelBuffer? > > We need to release the sampleBuffer after it was used to create the frame so > passing the CVPixelBuffer would cause us to leak the sampleBuffer. And we need > to create the frame in the block because we need to use the correct timestamp. Can we use CVBufferRetain(pixelBuffer) in readNextBuffer and then CVBufferRelease(pixelBuffer) after we have created the frame to overcome this problem?
On 2017/06/01 15:43:03, magjed_webrtc wrote: > https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m (right): > > https://codereview.webrtc.org/2887673002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/RTCFileVideoCapturer.m:128: if (!pixelBuffer) > { > On 2017/06/01 14:13:27, daniela-webrtc wrote: > > On 2017/06/01 13:44:37, magjed_webrtc wrote: > > > Maybe we can extract the CVPixelBufferRef and do this check in > readNextBuffer > > > instead, and publish the final pixelBuffer? > > > > We need to release the sampleBuffer after it was used to create the frame so > > passing the CVPixelBuffer would cause us to leak the sampleBuffer. And we need > > to create the frame in the block because we need to use the correct timestamp. > > > Can we use CVBufferRetain(pixelBuffer) in readNextBuffer and then > CVBufferRelease(pixelBuffer) after we have created the frame to overcome this > problem? No because they are different pointers. Also we don't own the pixelBuffer and calling CVBufferRelease might cause an over release.
lgtm
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2887673002/#ps160001 (title: "Fix casting and add return")
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: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/25147) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/26211) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17751)
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2887673002/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496411749100870,
"parent_rev": "f79ade1320d1f27dc85a1bf7da5ca430ca80ab85", "commit_rev":
"0d4d57f26d7974e1832d69dabd05c57567553882"}
Message was sent while issue was closed.
Description was changed from ========== Add RTCFileVideoCapturer class. - Reads and dispatches buffers from a video file, along the lines of camera capturer. - Initial purpose of this class will be for testing. BUG=webrtc:7581 ========== to ========== Add RTCFileVideoCapturer class. - Reads and dispatches buffers from a video file, along the lines of camera capturer. - Initial purpose of this class will be for testing. BUG=webrtc:7581 Review-Url: https://codereview.webrtc.org/2887673002 Cr-Commit-Position: refs/heads/master@{#18412} Committed: https://chromium.googlesource.com/external/webrtc/+/0d4d57f26d7974e1832d69dab... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/0d4d57f26d7974e1832d69dab... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
