Bug 47996 – Video -> Canvas doesn't work on Windows

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

* STEPS TO REPRODUCE
1. go to http://www.html5rocks.com/tutorials/video/basics/
2. scroll down to section 5.4, second example
3. Start playing video in first frame

* RESULTS
Its supposed to draw in second and third frame, but it does not.

[Jer Noble]

<rdar://problem/7884690>

[Jer Noble]

Created attachment 71318 [details]
Patch

[Jer Noble]

Created attachment 71331 [details]
Patch

[Jer Noble]

Created attachment 71404 [details]
Patch

Fixed a couple of leaks: one pixelBufferAttributes dictionary and one ImageDescriptionHandle.  Updated the part of the QTDecompressionSession which creates the ImageDescription.

[Eric Carlson]

Comment on attachment 71404 [details]
Patch

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::ConfigureForCGImage);

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).

[Jer Noble]

(In reply to comment #5)
> (From update of attachment 71404 [details])
> 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.

I'll see what I can do.  The issue with keeping it around is, once it's created, it's dedicated to decompressing only one pixel format; if the media changes and begins outputting a different type of pixel format, the QTDecompressionSession will need to be recreated.

> > 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?

It's horribly, horribly long.  We don't have to put each param on a different line, but it's much more readable that way.  I could get rid of the comments describing what the (unused) parameters are, and shorted it to about 3 lines.

> > 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?

I think I can do this with only static methods by moving the m_latestFrame member into QTDecompressionSession and making the client class a friend.

> > 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::ConfigureForCGImage);
> 
> 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).

Even if I do keep the session around for more than one frame, this part of the code should still only ever be called once (when setting up the ICMDecompressionSession.  So I'm not sure it would make sense to make them member variables.

[Eric Carlson]

(In reply to comment #6)
> (In reply to comment #5)
> > 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.
> 
> I'll see what I can do.  The issue with keeping it around is, once it's created, it's dedicated to decompressing only one pixel format; if the media changes and begins outputting a different type of pixel format, the QTDecompressionSession will need to be recreated.
> 
True, but the QTMovie will be destroyed if the movie is changed and I don't think a single visual context will vend more than one pixel format.


> > 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).
> 
> Even if I do keep the session around for more than one frame, this part of the code should still only ever be called once (when setting up the ICMDecompressionSession.  So I'm not sure it would make sense to make them member variables.

Well, if you do keep the decompression session around the canvas size could change so the image description would have to be updated.

[Jer Noble]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > 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.
> > 
> > I'll see what I can do.  The issue with keeping it around is, once it's created, it's dedicated to decompressing only one pixel format; if the media changes and begins outputting a different type of pixel format, the QTDecompressionSession will need to be recreated.
> > 
> True, but the QTMovie will be destroyed if the movie is changed and I don't think a single visual context will vend more than one pixel format.
> 
> 
> > > 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).
> > 
> > Even if I do keep the session around for more than one frame, this part of the code should still only ever be called once (when setting up the ICMDecompressionSession.  So I'm not sure it would make sense to make them member variables.
> 
> Well, if you do keep the decompression session around the canvas size could change so the image description would have to be updated.

Okay, even though you r+'d the last patch, I'll update a new one with the changes you proposed above.

[Jer Noble]

Created attachment 71458 [details]
Patch

Made the QTDecompressionSession persistent within the MediaPlayer. It's also now RefPtr compatible.

[Eric Carlson]

Comment on attachment 71458 [details]
Patch

Thanks!

[Jer Noble]

Committed r70252: <http://trac.webkit.org/changeset/70252>