Bug 61279 – [Qt] Enable usage of QuickTime mediaplayer for the Qt port on Mac.

• Component: Media
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

[Qt] Enable usage of QuickTime mediaplayer for the Qt port.

[Alexis Menard (darktears)]

Created attachment 94417 [details]
Patch

[Alexis Menard (darktears)]

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.

[Early Warning System Bot]

Comment on attachment 94417 [details]
Patch

Attachment 94417 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8723817

[Alexis Menard (darktears)]

Created attachment 94424 [details]
Patch

[Alexis Menard (darktears)]

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

[Jer Noble]

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?

[Alexis Menard (darktears)]

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.

[WebKit Commit Bot]

Comment on attachment 94424 [details]
Patch

Attachment 94424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8729379

[Alexis Menard (darktears)]

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

[Alexis Menard (darktears)]

Created attachment 94619 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #10)
> Created an attachment (id=94619) [details]
> Patch

Better version of the patch in the paint function.

[WebKit Commit Bot]

Comment on attachment 94619 [details]
Patch

Attachment 94619 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8732433

[Jer Noble]

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

[Alexis Menard (darktears)]

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

[Jer Noble]

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

[Alexis Menard (darktears)]

Created attachment 94678 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 94679 [details]
Patch

[Eric Carlson]

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.

[Alexis Menard (darktears)]

Created attachment 94803 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 94803 [details]
Patch for landing

Clearing flags on attachment: 94803

Committed r87312: <http://trac.webkit.org/changeset/87312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r87312 cherry-picked into qtwebkit-2.2 with commit 1b5dd98 <http://gitorious.org/webkit/qtwebkit/commit/1b5dd98>