Bug 61279 - [Qt] Enable usage of QuickTime mediaplayer for the Qt port on Mac.
Summary: [Qt] Enable usage of QuickTime mediaplayer for the Qt port on Mac.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-05-23 06:30 PDT by Alexis Menard (darktears)
Modified: 2011-06-03 14:06 PDT (History)
7 users (show)

See Also:


Attachments
Patch (34.99 KB, patch)
2011-05-23 06:48 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (34.95 KB, patch)
2011-05-23 07:25 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (34.75 KB, patch)
2011-05-24 08:59 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (34.75 KB, patch)
2011-05-24 13:27 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (34.75 KB, patch)
2011-05-24 13:32 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (35.11 KB, patch)
2011-05-25 10:19 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2011-05-23 06:30:31 PDT
[Qt] Enable usage of QuickTime mediaplayer for the Qt port.
Comment 1 Alexis Menard (darktears) 2011-05-23 06:48:54 PDT
Created attachment 94417 [details]
Patch
Comment 2 Alexis Menard (darktears) 2011-05-23 07:02:10 PDT
Comment on attachment 94417 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:466
> +#else

Could be done better. Will change it when landing. parentView = 0 and just an #if for mac. Thanks caio.
Comment 3 Early Warning System Bot 2011-05-23 07:10:33 PDT
Comment on attachment 94417 [details]
Patch

Attachment 94417 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8723817
Comment 4 Alexis Menard (darktears) 2011-05-23 07:25:42 PDT
Created attachment 94424 [details]
Patch
Comment 5 Alexis Menard (darktears) 2011-05-23 07:26:40 PDT
(In reply to comment #4)
> Created an attachment (id=94424) [details]
> Patch

Should fix the ews issue and this patch includes Caio's suggestion for parentView.
Comment 6 Jer Noble 2011-05-23 08:06:19 PDT
Comment on attachment 94424 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1361
> +#else
> +    QWebPageClient* client = m_player->frameView()->hostWindow()->platformPageClient();
> +    CGContextRef cgContext = static_cast<CGContextRef>(client ? client->ownerWidget()->macCGHandle() : 0);
> +    CGContextSaveGState(cgContext);
> +    CGContextSetInterpolationQuality(cgContext, kCGInterpolationLow);
> +    CGContextTranslateCTM(cgContext, r.x(), r.y() + r.height());
> +    CGContextScaleCTM(cgContext, scaleFactor.width(), scaleFactor.height());
> +
> +    newContext = [NSGraphicsContext graphicsContextWithGraphicsPort:cgContext flipped:NO];
> +#endif

It looks like you're ignoring the passed-in context parameter here.  Is that intentional?  Is there a way to pass the correct context into paint() instead of calling back out to the pageClient for a context?
Comment 7 Alexis Menard (darktears) 2011-05-23 08:11:49 PDT
Comment on attachment 94424 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1361
>> +#endif
> 
> It looks like you're ignoring the passed-in context parameter here.  Is that intentional?  Is there a way to pass the correct context into paint() instead of calling back out to the pageClient for a context?

No because the context we have in Qt is not valid for drawing with CoreGraphics/QT. The context we get is a QPainter for Qt, if we apply the transformations on it, it won't change anything because at the end the video won't be draw by Qt but by QuickTime. That's why I have to get the CoreGraphics Context and apply the transformation manually.
Comment 8 WebKit Commit Bot 2011-05-23 19:21:44 PDT
Comment on attachment 94424 [details]
Patch

Attachment 94424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8729379
Comment 9 Alexis Menard (darktears) 2011-05-23 19:25:53 PDT
(In reply to comment #8)
> (From update of attachment 94424 [details])
> Attachment 94424 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/8729379


ERROR: WebCoreObjCExtras.o has one or more global initializers in it! (/Users/abarth/git/webkit-queue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/WebCoreObjCExtras.o), near _WebCoreObjCFinalizeOnMainThread _WebCoreObjCFinalizeOnMainThread.eh

not sure to understand this properly and how my change could be related. Someone with Mac knowledge can help me please?
Comment 10 Alexis Menard (darktears) 2011-05-24 08:59:45 PDT
Created attachment 94619 [details]
Patch
Comment 11 Alexis Menard (darktears) 2011-05-24 09:00:34 PDT
(In reply to comment #10)
> Created an attachment (id=94619) [details]
> Patch

Better version of the patch in the paint function.
Comment 12 WebKit Commit Bot 2011-05-24 11:11:36 PDT
Comment on attachment 94619 [details]
Patch

Attachment 94619 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8732433
Comment 13 Jer Noble 2011-05-24 11:22:17 PDT
(In reply to comment #12)
> (From update of attachment 94619 [details])
> Attachment 94619 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/8732433

This build error is probably due to the "#include <iostream>" added to Source/WebCore/platform/mac/WebCoreObjCExtras.mm.  

Is that addition necessary?
Comment 14 Alexis Menard (darktears) 2011-05-24 11:46:00 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 94619 [details] [details])
> > Attachment 94619 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/8732433
> 
> This build error is probably due to the "#include <iostream>" added to Source/WebCore/platform/mac/WebCoreObjCExtras.mm.  
> 
> Is that addition necessary?

Well yes because otherwise std::pair is not defined in the file. I could protect it into a #if PLATFORM(QT) but I wonder how it could compile for Mac. I grep the code for iostream nothing.
Comment 15 Jer Noble 2011-05-24 12:38:19 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 94619 [details] [details] [details])
> > > Attachment 94619 [details] [details] [details] did not pass mac-ews (mac):
> > > Output: http://queues.webkit.org/results/8732433
> > 
> > This build error is probably due to the "#include <iostream>" added to Source/WebCore/platform/mac/WebCoreObjCExtras.mm.  
> > 
> > Is that addition necessary?
> 
> Well yes because otherwise std::pair is not defined in the file. I could protect it into a #if PLATFORM(QT) but I wonder how it could compile for Mac. I grep the code for iostream nothing.

stl_pair.h is included by Source/WebCore/WebCorePrefix.h, by way of: #include "string.h".

Even then, "#include <iostream>" is the wrong way to go about defining std::pair.
Comment 16 Alexis Menard (darktears) 2011-05-24 13:27:39 PDT
Created attachment 94678 [details]
Patch
Comment 17 Alexis Menard (darktears) 2011-05-24 13:32:22 PDT
Created attachment 94679 [details]
Patch
Comment 18 Eric Carlson 2011-05-25 07:53:16 PDT
Comment on attachment 94679 [details]
Patch

r+, but please consider making the changes I suggest before committing.


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

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:712
> +#if PLATFORM(QT) && USE(QTKIT)
> +    return 0;
> +#else
>      return m_qtVideoLayer.get();
> +#endif

You could get rid of the #if here by returning 0 if there is no layer:

    if (!m_qtVideoLayer)
        return 0;
    return m_qtVideoLayer.get();

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1348
> +#if PLATFORM(QT) && USE(QTKIT)
> +    CGContextRef cgContext = static_cast<CGContextRef>([[NSGraphicsContext currentContext] graphicsPort]);
> +    CGContextSaveGState(cgContext);
> +    CGContextSetInterpolationQuality(cgContext, kCGInterpolationLow);
> +    CGContextTranslateCTM(cgContext, r.x(), r.y() + r.height());
> +    CGContextScaleCTM(cgContext, scaleFactor.width(), scaleFactor.height());
> +
> +    newContext = [NSGraphicsContext currentContext];

A comment here about why you can't use the passed context will help people not familiar with the Qt architecture understand and not break this in the future.

> Source/WebCore/platform/mac/WebCoreObjCExtras.mm:40
> +#include <utility>
>  #include <objc/objc-auto.h>
>  #include <objc/objc-runtime.h>
>  #include <wtf/Assertions.h>

The new #include should be inserted in sort order.
Comment 19 Alexis Menard (darktears) 2011-05-25 10:19:13 PDT
Created attachment 94803 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2011-05-25 12:22:37 PDT
Comment on attachment 94803 [details]
Patch for landing

Clearing flags on attachment: 94803

Committed r87312: <http://trac.webkit.org/changeset/87312>
Comment 21 WebKit Commit Bot 2011-05-25 12:22:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Ademar Reis 2011-06-03 14:06:39 PDT
Revision r87312 cherry-picked into qtwebkit-2.2 with commit 1b5dd98 <http://gitorious.org/webkit/qtwebkit/commit/1b5dd98>