Bug 165214 – [MediaStream][Mac] Video presets sometimes don't work

• Status: ASSIGNED
• Component: Media

[Eric Carlson]

Applying an AVCapture preset sometimes don't change the size of the capture stream. Use AVCaptureVideoDataOutput videoSettings to work around this.

[Radar WebKit Bug Importer]

<rdar://problem/29444533>

[Eric Carlson]

Created attachment 295852 [details]
Proposed patch.

[Jer Noble]

Comment on attachment 295852 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=295852&action=review

r=me with nits.

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h:68
> +    IntSize sizeForPreset(NSString*);

No need for this to be a class instance method; it could easily be a static method in the .mm file.

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:324
> +        RetainPtr<NSDictionary> settingsDictionary = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys: [NSNumber numberWithInt:size.width()], kCVPixelBufferWidthKey, [NSNumber numberWithInt:size.height()], kCVPixelBufferHeightKey, [NSNumber numberWithInt:videoCaptureFormat], kCVPixelBufferPixelFormatTypeKey, nil]);

This could be:

auto settingsDictionary = @{
  (NSString*)kCVPixelBufferPixelFormatTypeKey: @(videoCaptureFormat),
  (NSString*)kCVPixelBufferWidthKey: @(size.width()),
  (NSString*)kCVPixelBufferHeightKey: @(size.height()),
};

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:401
> +    auto settingsDictionary = adoptNS([[NSMutableDictionary alloc] initWithObjectsAndKeys: [NSNumber numberWithInt:videoCaptureFormat], kCVPixelBufferPixelFormatTypeKey, nil]);
> +    if (m_pendingPreset) {
> +#if PLATFORM(MAC)
> +        auto size = sizeForPreset(m_pendingPreset.get());
> +        [settingsDictionary.get() setObject:[NSNumber numberWithInt:size.width()] forKey:(NSString*)kCVPixelBufferWidthKey];
> +        [settingsDictionary.get() setObject:[NSNumber numberWithInt:size.height()] forKey:(NSString*)kCVPixelBufferHeightKey];
> +#endif
> +    }

This could be:

auto size = sizeForPreset(m_pendingPreset.get())
auto settingsDictionary = @{
  (NSString*)kCVPixelBufferPixelFormatTypeKey: @(videoCaptureFormat),
#if PLATFORM(MAC)
  (NSString*)kCVPixelBufferWidthKey: @(size.width()),
  (NSString*)kCVPixelBufferHeightKey: @(size.height()),
#endif
};

[Eric Carlson]

Comment on attachment 295852 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=295852&action=review

Thanks for the review!

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h:68
>> +    IntSize sizeForPreset(NSString*);
> 
> No need for this to be a class instance method; it could easily be a static method in the .mm file.

Fixed.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:324
>> +        RetainPtr<NSDictionary> settingsDictionary = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys: [NSNumber numberWithInt:size.width()], kCVPixelBufferWidthKey, [NSNumber numberWithInt:size.height()], kCVPixelBufferHeightKey, [NSNumber numberWithInt:videoCaptureFormat], kCVPixelBufferPixelFormatTypeKey, nil]);
> 
> This could be:
> 
> auto settingsDictionary = @{
>   (NSString*)kCVPixelBufferPixelFormatTypeKey: @(videoCaptureFormat),
>   (NSString*)kCVPixelBufferWidthKey: @(size.width()),
>   (NSString*)kCVPixelBufferHeightKey: @(size.height()),
> };

Good idea, fixed.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:401
>> +    }
> 
> This could be:
> 
> auto size = sizeForPreset(m_pendingPreset.get())
> auto settingsDictionary = @{
>   (NSString*)kCVPixelBufferPixelFormatTypeKey: @(videoCaptureFormat),
> #if PLATFORM(MAC)
>   (NSString*)kCVPixelBufferWidthKey: @(size.width()),
>   (NSString*)kCVPixelBufferHeightKey: @(size.height()),
> #endif
> };

That doesn't work because we can't set the width and height attributes unless m_pendingPreset is non-NULL.

[Eric Carlson]

Created attachment 295865 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 295865 [details]
Patch for landing.

Rejecting attachment 295865 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 295865, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2602220

[Eric Carlson]

Created attachment 295869 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 295869 [details]
Patch for landing.

Clearing flags on attachment: 295869

Committed r209188: <http://trac.webkit.org/changeset/209188>