[webkit-reviews] review granted: [Bug 47996] Video -> Canvas doesn't work on Windows : [Attachment 71404] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 47996: Video -> Canvas doesn't work on Windows
https://bugs.webkit.org/show_bug.cgi?id=47996

Attachment 71404: Patch
https://bugs.webkit.org/attachment.cgi?id=71404&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=71404&action=review

r=me, but consider keeping the decompression session around for more than one
frame.

>
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:743
> +	       // We are probably being asked to render the video into a
canvas, but 
> +	       // there's a good chance the QTPixelBuffer is not ARGB and thus
can't be
> +	       // drawn using CG.  If so, fire up an ICMDecompressionSession
and convert 
> +	       // the current frame into something which can be rendered by CG.

> +	       if (!buffer.pixelFormatIs32ARGB() &&
!buffer.pixelFormatIs32BGRA()) {
> +		   QTDecompressionSession session;
> +		   buffer = session.decompress(buffer);
> +	       }

If we are being asked to render a frame to a canvas, we will almost certainly
be asked to render more. Please consider keeping the decompression session
around instead of creating it every time.

> WebCore/platform/graphics/win/QTDecompressionSession.cpp:43
> +    static void trackingCallback(void *decompressionTrackingRefCon, 
> +	   OSStatus, // result 
> +	   ICMDecompressionTrackingFlags decompressionTrackingFlags, 
> +	   CVPixelBufferRef pixelBuffer, 
> +	   TimeValue64, // displayTime 
> +	   TimeValue64, // displayDuration 
> +	   ICMValidTimeFlags, // validTimeFlags 
> +	   void *, // reserved 
> +	   void *) // sourceFrameRefCon 
> +    {

The style of this declaration is odd for WebKit, do you need to have each param
on a different line?

> WebCore/platform/graphics/win/QTDecompressionSession.cpp:55
> +QTDecompressionSession::QTDecompressionSession()
> +    : m_client(new QTDecompressionSessionClient())

The client class is small and you always want one, why allocate it on the heap?


> WebCore/platform/graphics/win/QTDecompressionSession.cpp:125
> +	   ImageDescriptionHandle description =
(ImageDescriptionHandle)NewHandleClear(sizeof(ImageDescription));
> +	   (**description).idSize = sizeof(ImageDescription);
> +	   (**description).cType = cType;
> +	   (**description).version = 2;
> +	   (**description).spatialQuality = codecLosslessQuality;
> +	   (**description).width = inBuffer.width();
> +	   (**description).height = inBuffer.height();
> +	   (**description).hRes = 72 << 16; // 72 DPI as a fixed-point number
> +	   (**description).vRes = 72 << 16; // 72 DPI as a fixed-point number
> +	   (**description).frameCount = 1;
> +	   (**description).depth =  depth;
> +	   (**description).clutID = -1;
> +
> +	   // Create the mandatory ICMDecompressionSessionOptions, but leave
> +	   // all the default values.
> +	   ICMDecompressionSessionOptionsRef options = 0;
> +	   ICMDecompressionSessionOptionsCreate(kCFAllocatorDefault, &options);

> +
> +	   CFDictionaryRef pixelBufferAttributes =
QTPixelBuffer::createPixelBufferAttributesDictionary(QTPixelBuffer::ConfigureFo
rCGImage);

You always need all of these and the values and/or sizes are always the same,
so you might want to make them member variables (especially if you keep the
decompression session around for more than one frame).