Mac AVFoundation: Allow use of all camera's supported resolutions.

Mac AVFoundation: Allow use of all camera's supported resolutions.

Historically, QTKit had the camera resolutions limited to QVGA/VGA/HD
due to some internal rescaling not respecting aspect ratio in those
systems. AVFoundation has not shown such problems, to the best
of the author's knowledge, and the limited resolutions prevent 
using all the camera's capabilities. This CL enables the use of all
resolutions listed by the camera as supported.

QTKit implementation remains untouched.

Tested: Locally using C910 and Facetime HD alternatively. This CL
solves the attached bugs 367668, 367670.

[1] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/capture/mac/video_capture_device_mac.mm&sq=package:chromium&l=189

BUG=288562, 367668, 367670
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266845

[mcasas, 2014-04-28 12:16:58]

bemasc@ PTAL since you wrote the QTKit resolution deciding code.
rsesek@ PTAL.
tommi@ PTAL/owners stamp.

[tommi (sloooow) - chröme, 2014-04-28 12:42:05]

rs lgtm

[Robert Sesek, 2014-04-28 14:14:45]

lgtm

[bemasc, 2014-04-28 17:17:32]

https://codereview.chromium.org/258783007/diff/1/media/video/capture/mac/vide...
File media/video/capture/mac/video_capture_device_avfoundation_mac.mm (right):

https://codereview.chromium.org/258783007/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_avfoundation_mac.mm:178: //
Identify the sessionPreset that corresponds to the desired resolution.
This comment should probably be updated, since the preset no longer corresponds
to the desired resolution.  The user could have requested some other resolution
(e.g. 1920x1080).

Moreover ... why is this logic needed at all?  According to the API
documentation, setting the frame size or framerate explicitly automatically
overrides the preset:

"When you change the device’s format, the session preset automatically changes
to this value [AVCaptureSessionPresetInputPriority], indicating that the
AVCaptureSession object has relinquished responsibility for configuring its
inputs and outputs."

Why can't you just delete all mention of presets, if you intend to set the
capture mode explicitly?

https://codereview.chromium.org/258783007/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_avfoundation_mac.mm:187:
canSetSessionPreset:AVFoundationGlue::AVCaptureSessionPreset320x240()]) {
Does such a preset even exist?  It isn't documented at

https://developer.apple.com/library/ios/documentation/AVFoundation/Reference/...

[mcasas, 2014-04-28 19:01:36]

bemasc@, PTAL.

https://codereview.chromium.org/258783007/diff/1/media/video/capture/mac/vide...
File media/video/capture/mac/video_capture_device_avfoundation_mac.mm (right):

https://codereview.chromium.org/258783007/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_avfoundation_mac.mm:178: //
Identify the sessionPreset that corresponds to the desired resolution.
Seems that you're right, despite the line you copy referring to
the AVCaptureDevice configured preset, which is coming from
a limited number of presets, again, unless going the 
-activeFormat way, which is not in the code now. Let me 
explain:

The way we configure the device is via sessionPreset +
CrAVCaptureVideoDataOutput, this last one using some
configuration keys in the dictionary that are supposedly
not supported but in practice necessary (hence the long 
comment).  See [1]:  "The only key currently supported is 
the kCVPixelBufferPixelFormatTypeKey key."

But, it seems that for some reason, removing the preset
logic allows the CrAVCaptureVideoDataOutput to
pass the configuration to Session and Device, so all
in all, your comment is taken in, and I removed all the
preset logic after dutifully testing with my cameras :)

[1]
https://developer.apple.com/library/ios/documentation/AVFoundation/Reference/...


On 2014/04/28 17:17:33, bemasc wrote:
> This comment should probably be updated, since the preset no longer
corresponds
> to the desired resolution.  The user could have requested some other
resolution
> (e.g. 1920x1080).
> 
> Moreover ... why is this logic needed at all?  According to the API
> documentation, setting the frame size or framerate explicitly automatically
> overrides the preset:
> 
> "When you change the device’s format, the session preset automatically changes
> to this value [AVCaptureSessionPresetInputPriority], indicating that the
> AVCaptureSession object has relinquished responsibility for configuring its
> inputs and outputs."
> 
> Why can't you just delete all mention of presets, if you intend to set the
> capture mode explicitly?

https://codereview.chromium.org/258783007/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_avfoundation_mac.mm:187:
canSetSessionPreset:AVFoundationGlue::AVCaptureSessionPreset320x240()]) {
Not in iOS but in Mac:

https://developer.apple.com/library/mac/documentation/AVFoundation/Reference/...

On 2014/04/28 17:17:33, bemasc wrote:
> Does such a preset even exist?  It isn't documented at
> 
>
https://developer.apple.com/library/ios/documentation/AVFoundation/Reference/...

[bemasc, 2014-04-28 19:18:03]

lgtm

https://codereview.chromium.org/258783007/diff/10001/media/video/capture/mac/...
File media/video/capture/mac/video_capture_device_avfoundation_mac.mm (right):

https://codereview.chromium.org/258783007/diff/10001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_avfoundation_mac.mm:179: //
detailing that setting the sessionPreset is enough. The reason for this
Nit: "would be enough", to be clear that we don't actually use it.

[mcasas, 2014-04-29 06:56:31]

https://codereview.chromium.org/258783007/diff/10001/media/video/capture/mac/...
File media/video/capture/mac/video_capture_device_avfoundation_mac.mm (right):

https://codereview.chromium.org/258783007/diff/10001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_avfoundation_mac.mm:179: //
detailing that setting the sessionPreset is enough. The reason for this
On 2014/04/28 19:18:03, bemasc wrote:
> Nit: "would be enough", to be clear that we don't actually use it.

Done.

[mcasas, 2014-04-29 06:56:38]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-04-29 06:57:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/258783007/30001

[commit-bot: I haz the power, 2014-04-29 07:00:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 07:00:54]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[mcasas, 2014-04-29 07:21:01]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-04-29 07:23:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/258783007/30001

[commit-bot: I haz the power, 2014-04-29 11:21:22]

Change committed as 266845