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

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>