Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2129733004:
ImageCapture: Implement takePhoto() for Mac AVFoundation (Closed)
|
Created:
4 years, 5 months ago by mcasas Modified:
4 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImageCapture: Implement takePhoto() for Mac AVFoundation
By pulling the necessary symbols out of AVFoundation
and using them; most notably (Cr)AVCaptureStillImageOutput.
TakePhoto is already implemented for Android (both APIs).
Also adding VideoCaptureDeviceTest::MAYBE_TakePhoto
test case, enabled only for Mac.
BUG=518807
TEST=Run build with flag --enable-blink-features=ImageCapture,
navigate to [1] and push buttons
- Open Camera ...
- Create ImageCapturer
- takePhoto() (N times!) --> profit
[1] https://rawgit.com/Miguelao/demos/master/imagecapture.html
Committed: https://crrev.com/ad75c34fbb7940127a99be04ed6019ab09d73880
Cr-Commit-Position: refs/heads/master@{#404832}
Patch Set 1 #Patch Set 2 : General cleanup and added VideoCaptureDeviceTest::TakePhoto #
Total comments: 12
Patch Set 3 : Rebased _unittests.cc and rsesek@ comments #
Total comments: 6
Patch Set 4 : rsesek@ and emircan@s comments #
Total comments: 2
Messages
Total messages: 33 (16 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== [WIP] ImageCapture: Implement takePhoto() for Mac AVFoundation BUG=518807 ========== to ========== ImageCapture: Implement takePhoto() for Mac AVFoundation By pulling the necessary symbols out of AVFoundation and using them; most notably (Cr)AVCaptureStillImageOutput. BUG=518807 TEST=Run build with flag --enable-blink-features=ImageCapture, navigate to [1] and push buttons - Open Camera ... - Create ImageCapturer - takePhoto() (N times!) --> profit [1] https://rawgit.com/Miguelao/demos/master/imagecapture.html ==========
Patchset #1 (id:40001) has been deleted
Description was changed from ========== ImageCapture: Implement takePhoto() for Mac AVFoundation By pulling the necessary symbols out of AVFoundation and using them; most notably (Cr)AVCaptureStillImageOutput. BUG=518807 TEST=Run build with flag --enable-blink-features=ImageCapture, navigate to [1] and push buttons - Open Camera ... - Create ImageCapturer - takePhoto() (N times!) --> profit [1] https://rawgit.com/Miguelao/demos/master/imagecapture.html ========== to ========== ImageCapture: Implement takePhoto() for Mac AVFoundation By pulling the necessary symbols out of AVFoundation and using them; most notably (Cr)AVCaptureStillImageOutput. TakePhoto is already implemented for Android (both APIs). Also adding VideoCaptureDeviceTest::MAYBE_TakePhoto test case, enabled only for Mac. BUG=518807 TEST=Run build with flag --enable-blink-features=ImageCapture, navigate to [1] and push buttons - Open Camera ... - Create ImageCapturer - takePhoto() (N times!) --> profit [1] https://rawgit.com/Miguelao/demos/master/imagecapture.html ==========
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
mcasas@chromium.org changed reviewers: + emircan@chromium.org, rsesek@chromium.org
rsesek@ PTAL at all files minus _unittest.cc emircan@ PTAL at the unittest.cc (the others too if you want)
https://codereview.chromium.org/2129733004/diff/160001/media/base/mac/avfound... File media/base/mac/avfoundation_glue.h (right): https://codereview.chromium.org/2129733004/diff/160001/media/base/mac/avfound... media/base/mac/avfoundation_glue.h:5: // AVFoundation API is only introduced in Mac OS X > 10.6, and there is only one Chrome now supports only 10.9+ so I think all this can be cleaned up. Maybe file a bug for that and don't add new entries here? https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_avfoundation_mac.mm (right): https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... media/capture/video/mac/video_capture_device_avfoundation_mac.mm:115: char** baseAddress, naming: use under_scores in non-ObjC methods https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.h (right): https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.h:81: int image_length, Lengths should generally be size_t. https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:448: mojo::Array<uint8_t>::From(photo)); Does this copy the data? Does it take an rvalue reference for move?
PTAL https://codereview.chromium.org/2129733004/diff/160001/media/base/mac/avfound... File media/base/mac/avfoundation_glue.h (right): https://codereview.chromium.org/2129733004/diff/160001/media/base/mac/avfound... media/base/mac/avfoundation_glue.h:5: // AVFoundation API is only introduced in Mac OS X > 10.6, and there is only one On 2016/07/08 19:31:04, Robert Sesek wrote: > Chrome now supports only 10.9+ so I think all this can be cleaned up. Maybe file > a bug for that and don't add new entries here? Hmmm there is already one, and I thought perkj@ wanted to take it, but he didn't. I propose adding the methods here and removing this whole thing separately. In any case, updated this comment. https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_avfoundation_mac.mm (right): https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... media/capture/video/mac/video_capture_device_avfoundation_mac.mm:115: char** baseAddress, On 2016/07/08 19:31:04, Robert Sesek wrote: > naming: use under_scores in non-ObjC methods Oops, yes, done. https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.h (right): https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.h:81: int image_length, On 2016/07/08 19:31:04, Robert Sesek wrote: > Lengths should generally be size_t. Done. https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:448: mojo::Array<uint8_t>::From(photo)); On 2016/07/08 19:31:04, Robert Sesek wrote: > Does this copy the data? Does it take an rvalue reference for move? From() copies the data in this case [1], which is what we want since |image_data| belongs to a CoreMediaGlue::CMSampleBufferRef that has to be processed synchronously. [1] https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/array.h?sq=pack...
Patchset #3 (id:180001) has been deleted
https://codereview.chromium.org/2129733004/diff/160001/media/base/mac/avfound... File media/base/mac/avfoundation_glue.h (right): https://codereview.chromium.org/2129733004/diff/160001/media/base/mac/avfound... media/base/mac/avfoundation_glue.h:5: // AVFoundation API is only introduced in Mac OS X > 10.6, and there is only one On 2016/07/08 21:39:28, mcasas wrote: > On 2016/07/08 19:31:04, Robert Sesek wrote: > > Chrome now supports only 10.9+ so I think all this can be cleaned up. Maybe > file > > a bug for that and don't add new entries here? > > Hmmm there is already one, and I thought perkj@ > wanted to take it, but he didn't. I propose adding > the methods here and removing this whole thing > separately. In any case, updated this comment. It seems counter-productive to add code that just needs to be deleted later (specifically CrAVCaptureStillImageOutput, I understand the methods on the existing Cr objects), but I won't press it. https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:448: mojo::Array<uint8_t>::From(photo)); On 2016/07/08 21:39:28, mcasas wrote: > On 2016/07/08 19:31:04, Robert Sesek wrote: > > Does this copy the data? Does it take an rvalue reference for move? > > From() copies the data in this case [1], which is > what we want since |image_data| belongs to a > CoreMediaGlue::CMSampleBufferRef that has to be > processed synchronously. > > [1] > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/array.h?sq=pack... But this isn't being copied from the CoreMedia buffer, it's copying from |photo| which has already copied the data. It looks like mojo::Array has a vector rvalue reference ctor that would be a better choice. https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/vi... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:518: DVLOG(1) << __FUNCTION__; Remove.
lgtm % nits. https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_avfoundation_mac.mm (right): https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/ma... media/capture/video/mac/video_capture_device_avfoundation_mac.mm:124: length, base_address); We should check that it returns kCMBlockBufferNoErr. https://developer.apple.com/library/ios/documentation/CoreMedia/Reference/CMB... https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/vi... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:150: ASSERT_GT(data.size(), 4u); Why is this checking for >=4?
rsesek@ PTAL https://codereview.chromium.org/2129733004/diff/160001/media/base/mac/avfound... File media/base/mac/avfoundation_glue.h (right): https://codereview.chromium.org/2129733004/diff/160001/media/base/mac/avfound... media/base/mac/avfoundation_glue.h:5: // AVFoundation API is only introduced in Mac OS X > 10.6, and there is only one On 2016/07/11 15:20:43, Robert Sesek wrote: > On 2016/07/08 21:39:28, mcasas wrote: > > On 2016/07/08 19:31:04, Robert Sesek wrote: > > > Chrome now supports only 10.9+ so I think all this can be cleaned up. Maybe > > file > > > a bug for that and don't add new entries here? > > > > Hmmm there is already one, and I thought perkj@ > > wanted to take it, but he didn't. I propose adding > > the methods here and removing this whole thing > > separately. In any case, updated this comment. > > It seems counter-productive to add code that just needs to be deleted later > (specifically CrAVCaptureStillImageOutput, I understand the methods on the > existing Cr objects), but I won't press it. Acknowledged. https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2129733004/diff/160001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:448: mojo::Array<uint8_t>::From(photo)); On 2016/07/11 15:20:43, Robert Sesek wrote: > On 2016/07/08 21:39:28, mcasas wrote: > > On 2016/07/08 19:31:04, Robert Sesek wrote: > > > Does this copy the data? Does it take an rvalue reference for move? > > > > From() copies the data in this case [1], which is > > what we want since |image_data| belongs to a > > CoreMediaGlue::CMSampleBufferRef that has to be > > processed synchronously. > > > > [1] > > > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/array.h?sq=pack... > > But this isn't being copied from the CoreMedia buffer, it's copying from |photo| > which has already copied the data. It looks like mojo::Array has a vector rvalue > reference ctor that would be a better choice. Gotcha, done. https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_avfoundation_mac.mm (right): https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/ma... media/capture/video/mac/video_capture_device_avfoundation_mac.mm:124: length, base_address); On 2016/07/11 21:23:08, emircan wrote: > We should check that it returns kCMBlockBufferNoErr. > > https://developer.apple.com/library/ios/documentation/CoreMedia/Reference/CMB... Done. I reused |noErr| like in [1,2] [1] https://cs.chromium.org/chromium/src/media/base/mac/videotoolbox_helpers.cc?q... [2] https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/vi... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:150: ASSERT_GT(data.size(), 4u); On 2016/07/11 21:23:08, emircan wrote: > Why is this checking for >=4? Ooops, unfinished, done. (nit: it's _GT, not _GE). https://codereview.chromium.org/2129733004/diff/200001/media/capture/video/vi... media/capture/video/video_capture_device_unittest.cc:518: DVLOG(1) << __FUNCTION__; On 2016/07/11 15:20:44, Robert Sesek wrote: > Remove. Done.
Last comment https://codereview.chromium.org/2129733004/diff/220001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2129733004/diff/220001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:448: mojo::Array<uint8_t>(std::vector<uint8_t>( This needs to be std::move to get the rvalue ref constructor.
Last comment https://codereview.chromium.org/2129733004/diff/220001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2129733004/diff/220001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:448: mojo::Array<uint8_t>(std::vector<uint8_t>( This needs to be std::move to get the rvalue ref constructor.
https://codereview.chromium.org/2129733004/diff/220001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2129733004/diff/220001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:448: mojo::Array<uint8_t>(std::vector<uint8_t>( On 2016/07/12 12:29:47, Robert Sesek wrote: > This needs to be std::move to get the rvalue ref constructor. When constructing a temporary, copy elision is automatic, so std::move is not needed. This is what llvm says if I change it: ../../media/capture/video/mac/video_capture_device_mac.mm:448:45: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move] mojo::Array<uint8_t>(std::move(std::vector<uint8_t>( ../../media/capture/video/mac/video_capture_device_mac.mm:448:45: note: remove std::move call here mojo::Array<uint8_t>(std::move(std::vector<uint8_t>( (If it was a static var, I'd need std::move() to add the second & that'd force using the move constructor).
Ah, TIL! LGTM
mcasas@chromium.org changed reviewers: + sandersd@chromium.org
sandersd@ RS media/base/mac/*glue* plz
lgtm
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org Link to the patchset: https://codereview.chromium.org/2129733004/#ps220001 (title: "rsesek@ and emircan@s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ImageCapture: Implement takePhoto() for Mac AVFoundation By pulling the necessary symbols out of AVFoundation and using them; most notably (Cr)AVCaptureStillImageOutput. TakePhoto is already implemented for Android (both APIs). Also adding VideoCaptureDeviceTest::MAYBE_TakePhoto test case, enabled only for Mac. BUG=518807 TEST=Run build with flag --enable-blink-features=ImageCapture, navigate to [1] and push buttons - Open Camera ... - Create ImageCapturer - takePhoto() (N times!) --> profit [1] https://rawgit.com/Miguelao/demos/master/imagecapture.html ========== to ========== ImageCapture: Implement takePhoto() for Mac AVFoundation By pulling the necessary symbols out of AVFoundation and using them; most notably (Cr)AVCaptureStillImageOutput. TakePhoto is already implemented for Android (both APIs). Also adding VideoCaptureDeviceTest::MAYBE_TakePhoto test case, enabled only for Mac. BUG=518807 TEST=Run build with flag --enable-blink-features=ImageCapture, navigate to [1] and push buttons - Open Camera ... - Create ImageCapturer - takePhoto() (N times!) --> profit [1] https://rawgit.com/Miguelao/demos/master/imagecapture.html ==========
Message was sent while issue was closed.
Committed patchset #4 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== ImageCapture: Implement takePhoto() for Mac AVFoundation By pulling the necessary symbols out of AVFoundation and using them; most notably (Cr)AVCaptureStillImageOutput. TakePhoto is already implemented for Android (both APIs). Also adding VideoCaptureDeviceTest::MAYBE_TakePhoto test case, enabled only for Mac. BUG=518807 TEST=Run build with flag --enable-blink-features=ImageCapture, navigate to [1] and push buttons - Open Camera ... - Create ImageCapturer - takePhoto() (N times!) --> profit [1] https://rawgit.com/Miguelao/demos/master/imagecapture.html ========== to ========== ImageCapture: Implement takePhoto() for Mac AVFoundation By pulling the necessary symbols out of AVFoundation and using them; most notably (Cr)AVCaptureStillImageOutput. TakePhoto is already implemented for Android (both APIs). Also adding VideoCaptureDeviceTest::MAYBE_TakePhoto test case, enabled only for Mac. BUG=518807 TEST=Run build with flag --enable-blink-features=ImageCapture, navigate to [1] and push buttons - Open Camera ... - Create ImageCapturer - takePhoto() (N times!) --> profit [1] https://rawgit.com/Miguelao/demos/master/imagecapture.html Committed: https://crrev.com/ad75c34fbb7940127a99be04ed6019ab09d73880 Cr-Commit-Position: refs/heads/master@{#404832} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ad75c34fbb7940127a99be04ed6019ab09d73880 Cr-Commit-Position: refs/heads/master@{#404832}
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:220001) has been created in https://codereview.chromium.org/2147873002/ by henrika@chromium.org. The reason for reverting is: Most likely breaks WebRtcWebcamBrowserTests/WebRtcWebcamBrowserTest.MANUAL_TestAcquiringAndReacquiringWebcam/0 on the WebRTC in Chrome bots. See https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/568... for details.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
