Bug 107732 - Mac: Avoid using k32BGRAPixelFormat on certain platforms.
Summary: Mac: Avoid using k32BGRAPixelFormat on certain platforms.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-23 13:59 PST by Jer Noble
Modified: 2013-01-24 13:12 PST (History)
3 users (show)

See Also:

Attachments
Patch (2.22 KB, patch)
2013-01-23 14:03 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2013-01-23 17:02 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (6.68 KB, patch)
2013-01-24 09:51 PST, Jer Noble 		eric.carlson: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-01-23 13:59:25 PST 
Mac: Avoid using k32BGRAPixelFormat on certain platforms.
Comment 1 Jer Noble 2013-01-23 14:03:50 PST 
Created attachment 184305 [details]
Patch
Comment 2 Jer Noble 2013-01-23 17:02:57 PST 
Created attachment 184357 [details]
Patch
Comment 3 Benjamin Poulain 2013-01-23 23:27:58 PST 
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]
Comment 4 Jer Noble 2013-01-24 09:27:27 PST 
(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.
Comment 5 Jer Noble 2013-01-24 09:51:31 PST 
Created attachment 184517 [details]
Patch
Comment 6 Jer Noble 2013-01-24 12:20:17 PST 
Committed r140706: <http://trac.webkit.org/changeset/140706>
Comment 7 Tim Horton 2013-01-24 12:51:34 PST 
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.
Comment 8 Jer Noble 2013-01-24 13:12:28 PST 
Committed r140714: <http://trac.webkit.org/changeset/140714>