Bug 93591 – Use the new AVPlayerItemVideoOutput API in MediaPlayerPrivateAVFoundation.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Use the new AVPlayerItemVideoOutput API in MediaPlayerPrivateAVFoundation.

[Jer Noble]

Created attachment 157542 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 157542 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:837
> +    m_imageGenerator = [AVAssetImageGenerator assetImageGeneratorWithAsset:m_avAsset.get()];

Shouldn't this use an adopt?

[Jer Noble]

Comment on attachment 157542 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:837
>> +    m_imageGenerator = [AVAssetImageGenerator assetImageGeneratorWithAsset:m_avAsset.get()];
> 
> Shouldn't this use an adopt?

It isn't clear that this function was simply moved, but if you look above (at original line 196), you see that this has always been like this.

It could be re-written to use the non-autoreleased version of the constructor, which would then need an adopt.  Or I could move this back to where it originally was and just add more  #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080.  But either way, modifying the old path is probably out of scope for this bug.

[Jer Noble]

Created attachment 157552 [details]
Patch

[Jer Noble]

Created attachment 157752 [details]
Patch

Fixed a logic error in setUpVideorendering()

[Eric Carlson]

Comment on attachment 157752 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        Pull out existing the AVAssetImageGenerator into its own functions:

Nit: typo "Pull out existing the ..." -> "Pull out the existing ..."

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:833
> +    NSDictionary* attributes = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInt:k32BGRAPixelFormat], kCVPixelBufferPixelFormatTypeKey,

I assume you are using k32BGRAPixelFormat because it is the most efficient format to draw? A comment about why this is so (is that the format <canvas> uses?) would be helpful.

Nit: I would prefer (slightly) if we didn't use autoreleased NSObjects.

[Jer Noble]

(In reply to comment #6)
> (From update of attachment 157752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157752&action=review
> 
> > Source/WebCore/ChangeLog:23
> > +        Pull out existing the AVAssetImageGenerator into its own functions:
> 
> Nit: typo "Pull out existing the ..." -> "Pull out the existing ..."

Fixed.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:833
> > +    NSDictionary* attributes = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInt:k32BGRAPixelFormat], kCVPixelBufferPixelFormatTypeKey,
> 
> I assume you are using k32BGRAPixelFormat because it is the most efficient format to draw? A comment about why this is so (is that the format <canvas> uses?) would be helpful.

Indeed.  I'll add a comment.

> Nit: I would prefer (slightly) if we didn't use autoreleased NSObjects.

I'd prefer if we could use dictionary literals.  One day... (Fixed)

[Jer Noble]

Committed r125318: <http://trac.webkit.org/changeset/125318>