Bug 147642

Summary: Adding ability to generate image from sample buffer for MediaPlayerPrivateMediaStream
Product: WebKit Reporter: Matthew Daiter <mdaiter>
Component: WebCore Misc.Assignee: Matthew Daiter <mdaiter>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, eric.carlson, jeremyj-wk, jer.noble, jonlee, mdaiter, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: PlatformOnly
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Matthew Daiter 2015-08-04 11:50:56 PDT 
Need the ability to generate CGImageRefs from CMSampleBuffers in MediaPlayerPrivateMediaStreamAVFObjC, in order to generate proper buffers for methods that need to paint to a context. If we don't have this method, we can't make an image, and we can't make a context out of a MediaStream.
Comment 1 Matthew Daiter 2015-08-04 11:56:08 PDT 
Created attachment 258194 [details]
Patch
Comment 2 Eric Carlson 2015-08-04 13:01:40 PDT 
Comment on attachment 258194 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:350
> +    // Get a CMSampleBuffer's Core Video image buffer for the media data

Nit: WebKit style is to not add a comment for something that is obvious from reading the code. This doesn't reach that bar.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:353
> +    // Lock the base address of the pixel buffer

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:356
> +    // Get the number of bytes per row for the pixel buffer

This comment is just wrong.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:361
> +    // Get the number of bytes per row for the pixel buffer
> +    size_t bytesPerRow = CVPixelBufferGetBytesPerRow(imageBuffer);
> +    // Get the pixel buffer width and height

These are obvious.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:365
> +    // Create a device-dependent RGB color space

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:372
> +    // Unlock the pixel buffer

Ditto.
Comment 3 Matthew Daiter 2015-08-04 13:06:01 PDT 
Created attachment 258203 [details]
Patch
Comment 4 Matthew Daiter 2015-08-04 13:06:44 PDT 
Comment on attachment 258194 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:350
>> +    // Get a CMSampleBuffer's Core Video image buffer for the media data
> 
> Nit: WebKit style is to not add a comment for something that is obvious from reading the code. This doesn't reach that bar.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:353
>> +    // Lock the base address of the pixel buffer
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:356
>> +    // Get the number of bytes per row for the pixel buffer
> 
> This comment is just wrong.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:361
>> +    // Get the pixel buffer width and height
> 
> These are obvious.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:365
>> +    // Create a device-dependent RGB color space
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:372
>> +    // Unlock the pixel buffer
> 
> Ditto.

Fixed.
Comment 5 WebKit Commit Bot 2015-08-04 13:30:04 PDT 
Comment on attachment 258203 [details]
Patch

Clearing flags on attachment: 258203

Committed r187883: <http://trac.webkit.org/changeset/187883>
Comment 6 WebKit Commit Bot 2015-08-04 13:30:06 PDT 
All reviewed patches have been landed.  Closing bug.