Summary: | Video -> Canvas doesn't work on Windows | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | eric.carlson | ||||||||||
Priority: | P2 | Keywords: | InRadar, PlatformOnly | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Windows 7 | ||||||||||||
URL: | http://www.html5rocks.com/tutorials/video/basics/#toc-fun-canvas | ||||||||||||
Attachments: |
|
Description
Jer Noble
2010-10-20 11:50:52 PDT
Created attachment 71318 [details]
Patch
Created attachment 71331 [details]
Patch
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 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). (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. (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. (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. Created attachment 71458 [details]
Patch
Made the QTDecompressionSession persistent within the MediaPlayer. It's also now RefPtr compatible.
Comment on attachment 71458 [details]
Patch
Thanks!
Committed r70252: <http://trac.webkit.org/changeset/70252> |