Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
860333003:
Mac Video Capture: Add support for pixel format NV12 for AVFoundation (Closed)
|
Created:
5 years, 11 months ago by magjed_chromium Modified:
5 years, 10 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac Video Capture: Add support for pixel format NV12 for AVFoundation
BUG=346634
Committed: https://crrev.com/40c2c94fbd52f9748ee082573e07404ae2621b9b
Cr-Commit-Position: refs/heads/master@{#313886}
Patch Set 1 #
Total comments: 12
Patch Set 2 : mcasas@ comments #Messages
Total messages: 16 (6 generated)
magjed@chromium.org changed reviewers: + perkj@chromium.org, tommi@chromium.org
Please take a look.
lgtm
magjed@chromium.org changed reviewers: + mcasas@chromium.org
magjed@chromium.org changed reviewers: + mcasas@chromium.org
mcasas - Do you want to take a look?
https://codereview.chromium.org/860333003/diff/1/media/base/mac/coremedia_glue.h File media/base/mac/coremedia_glue.h (right): https://codereview.chromium.org/860333003/diff/1/media/base/mac/coremedia_glu... media/base/mac/coremedia_glue.h:51: kCMPixelFormat_420YpCbCr8BiPlanarVideoRange = '420v', |kCMPixelFormat_422YpCbCr8_yuvs| comes from CMFormatDescription.h but the formats in l.51 and 52 don't , they are called |kCVPixelFormat_...| and come from CVPixelBuffer, which is part of another framework, the CoreVideo. Please correct and move to [1], as matter of fact half the work is done [2]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/base/mac/cor... [2] https://code.google.com/p/chromium/codesearch#chromium/src/media/base/mac/cor... https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_avfoundation_mac.mm (right): https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:20: media::VideoPixelFormat FourCCToChromiumPixelFormat(FourCharCode code) { This shouldn't be FourCharCode, that's too generic. We should use CMPixelFormatType instead [1] but that doesn't cover the kCVPixel... coming from CVPixelBuffer.h, sigh :( we might need to stick to it. [1] https://developer.apple.com/library/ios/documentation/CoreMedia/Reference/CMF... https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:29: case CoreMediaGlue::kCMPixelFormat_420YpCbCr8BiPlanarFullRange: Correct naming as per earlier comments. https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:197: FourCharCode best_fourcc = kCVPixelFormatType_422YpCbCr8; See comments above for FourCharCode. https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:211: FourCCToChromiumPixelFormat(best_fourcc)) { Hmm here the semantics are somehow cumbersome because we translate, with an implicit priority, CVPixel format to chromium, which is itself non-evidently enum-ordered by priority. Syntactic sugar proposal? Surprised you didn't come up with this Magnus, this is your specialty :) best_fourcc = s std::max(fourcc, best_fourcc, [](const FourCharCode& c1, const FourCharCode& c2) { return FourCCToChromiumPixelFormat(c1) < FourCCToChromiumPixelFormat(c2); }) Then a concern: AFAIK all cameras support I422 formats, and they are higher in the FourCCToChromiumPixelFormat() prio scheme than I420 formats. How can we be sure this code path is used? https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:333: static_cast<UInt>(captureFormat.frame_size.width())); |UInt| looks funny. What about |uint32| or even better |size_t|?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Thanks for your comments Miguel, and welcome back :) Regarding your comments about code that was introduced in CL=838723004, I will fix those in a separate CL and add you as a reviewer. https://codereview.chromium.org/860333003/diff/1/media/base/mac/coremedia_glue.h File media/base/mac/coremedia_glue.h (right): https://codereview.chromium.org/860333003/diff/1/media/base/mac/coremedia_glu... media/base/mac/coremedia_glue.h:51: kCMPixelFormat_420YpCbCr8BiPlanarVideoRange = '420v', On 2015/01/28 20:17:01, mcasas wrote: > |kCMPixelFormat_422YpCbCr8_yuvs| comes from CMFormatDescription.h > but the formats in l.51 and 52 don't , they are called > |kCVPixelFormat_...| and come from CVPixelBuffer, which is part > of another framework, the CoreVideo. Please correct and move to > [1], as matter of fact half the work is done [2]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/mac/cor... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/mac/cor... Acknowledged. I changed so that I only add support for video range in this CL, since I haven't found any device that enumerates '420f' to test with. https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_avfoundation_mac.mm (right): https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:20: media::VideoPixelFormat FourCCToChromiumPixelFormat(FourCharCode code) { On 2015/01/28 20:17:01, mcasas wrote: > This shouldn't be FourCharCode, that's too generic. We should use > CMPixelFormatType instead [1] but that doesn't cover the kCVPixel... > coming from CVPixelBuffer.h, sigh :( we might need to stick to it. > > [1] > https://developer.apple.com/library/ios/documentation/CoreMedia/Reference/CMF... Acknowledged. https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:29: case CoreMediaGlue::kCMPixelFormat_420YpCbCr8BiPlanarFullRange: On 2015/01/28 20:17:01, mcasas wrote: > Correct naming as per earlier comments. Done. https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:197: FourCharCode best_fourcc = kCVPixelFormatType_422YpCbCr8; On 2015/01/28 20:17:01, mcasas wrote: > See comments above for FourCharCode. Acknowledged. https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:211: FourCCToChromiumPixelFormat(best_fourcc)) { On 2015/01/28 20:17:01, mcasas wrote: > Hmm here the semantics are somehow cumbersome because we > translate, with an implicit priority, CVPixel format to chromium, > which is itself non-evidently enum-ordered by priority. > > Syntactic sugar proposal? Surprised you didn't come up > with this Magnus, this is your specialty :) > > best_fourcc = s std::max(fourcc, best_fourcc, > [](const FourCharCode& c1, const FourCharCode& c2) { > return FourCCToChromiumPixelFormat(c1) < > FourCCToChromiumPixelFormat(c2); > }) > > > Then a concern: AFAIK all cameras support I422 formats, and they > are higher in the FourCCToChromiumPixelFormat() prio scheme than > I420 formats. How can we be sure this code path is used? I will certainly consider a lambda :), but in a different CL. Regarding I420/I422 order, all I420 are above I422 in media::VideoPixelFormat: https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... https://codereview.chromium.org/860333003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_avfoundation_mac.mm:333: static_cast<UInt>(captureFormat.frame_size.width())); On 2015/01/28 20:17:01, mcasas wrote: > |UInt| looks funny. What about |uint32| or even better |size_t|? Done.
LGTM modulo follow-up CL. If you can't do it right away please add TODO() + bug.
The CQ bit was checked by magjed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/860333003/60001
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/40c2c94fbd52f9748ee082573e07404ae2621b9b Cr-Commit-Position: refs/heads/master@{#313886}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:60001) has been created in https://codereview.chromium.org/886933006/ by magjed@chromium.org. The reason for reverting is: Crashes on the assert that biplanar NV12 is tightly packed. BUG=455618 . |
