Bug 47996 - Video -> Canvas doesn't work on Windows
Summary: Video -> Canvas doesn't work on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Jer Noble
URL: http://www.html5rocks.com/tutorials/v...
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-10-20 11:50 PDT by Jer Noble
Modified: 2010-10-21 11:19 PDT (History)
1 user (show)

See Also:


Attachments
Patch (29.02 KB, patch)
2010-10-20 12:18 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (21.21 KB, patch)
2010-10-20 13:38 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (22.60 KB, patch)
2010-10-21 01:30 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (24.22 KB, patch)
2010-10-21 10:44 PDT, 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 2010-10-20 11:50:52 PDT
* 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.
Comment 1 Jer Noble 2010-10-20 11:51:28 PDT
<rdar://problem/7884690>
Comment 2 Jer Noble 2010-10-20 12:18:25 PDT
Created attachment 71318 [details]
Patch
Comment 3 Jer Noble 2010-10-20 13:38:46 PDT
Created attachment 71331 [details]
Patch
Comment 4 Jer Noble 2010-10-21 01:30:57 PDT
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.
Comment 5 Eric Carlson 2010-10-21 09:13:02 PDT
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).
Comment 6 Jer Noble 2010-10-21 09:44:21 PDT
(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.
Comment 7 Eric Carlson 2010-10-21 10:04:41 PDT
(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.
Comment 8 Jer Noble 2010-10-21 10:15:55 PDT
(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.
Comment 9 Jer Noble 2010-10-21 10:44:10 PDT
Created attachment 71458 [details]
Patch

Made the QTDecompressionSession persistent within the MediaPlayer. It's also now RefPtr compatible.
Comment 10 Eric Carlson 2010-10-21 10:55:49 PDT
Comment on attachment 71458 [details]
Patch

Thanks!
Comment 11 Jer Noble 2010-10-21 11:19:42 PDT
Committed r70252: <http://trac.webkit.org/changeset/70252>