Bug 107732 – Mac: Avoid using k32BGRAPixelFormat on certain platforms.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Mac: Avoid using k32BGRAPixelFormat on certain platforms.

[Jer Noble]

Created attachment 184305 [details]
Patch

[Jer Noble]

Created attachment 184357 [details]
Patch

[Benjamin Poulain]

Comment on attachment 184357 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:51
> +typedef struct OpaqueVTPixelTransferSession*  VTPixelTransferSessionRef;

Extra space.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1019
> -    NSDictionary* attributes = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInt:k32BGRAPixelFormat], kCVPixelBufferPixelFormatTypeKey,
> +    NSDictionary* attributes = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInt:
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
> +        kCVPixelFormatType_422YpCbCr8
> +#else
> +        k32BGRAPixelFormat
> +#endif
> +        ], kCVPixelBufferPixelFormatTypeKey,
>                                  nil];
>      m_videoOutput.adoptNS([[AVPlayerItemVideoOutput alloc] initWithPixelBufferAttributes:attributes]);

NSDictionary has a constructor for having a single key/object pair: dictionaryWithObject:forKey:.

I would turn the code a bit differently:
#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
    unsigned pixelFormat = kCVPixelFormatType_422YpCbCr8;
#else
    unsigned pixelFormat = k32BGRAPixelFormat
#endif
NSDictionary* attributes = [NSDictionary dictionaryWithObject:Foo forKey:Bar]

[Jer Noble]

(In reply to comment #3)
> (From update of attachment 184357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184357&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:51
> > +typedef struct OpaqueVTPixelTransferSession*  VTPixelTransferSessionRef;
> 
> Extra space.

Fixed.

> NSDictionary has a constructor for having a single key/object pair: dictionaryWithObject:forKey:.
> 
> I would turn the code a bit differently:
> #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
>     unsigned pixelFormat = kCVPixelFormatType_422YpCbCr8;
> #else
>     unsigned pixelFormat = k32BGRAPixelFormat
> #endif
> NSDictionary* attributes = [NSDictionary dictionaryWithObject:Foo forKey:Bar]

NSDictionary has an even simpler notation: 

NSDictionary* attributes = @{ (NSString*)kCVPixelBufferPixelFormatTypeKey, @ kCVPixelFormatType_422YpCbCr8 } 

...which we could safely use in the "#if 1090" block.

[Jer Noble]

Created attachment 184517 [details]
Patch

[Jer Noble]

Committed r140706: <http://trac.webkit.org/changeset/140706>

[Tim Horton]

Comment on attachment 184517 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:56
> +#import <VideoToolbox/VideoToolbox.h>

VideoToolbox was introduced in Mountain Lion, so this breaks the Lion build.

[Jer Noble]

Committed r140714: <http://trac.webkit.org/changeset/140714>