Bug 50770 – [GStreamer] PlatformVideoWindowMac implementation

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

A new PlatformVideoWindow implementation is needed for GStreamer-based fullscreen video display on Mac.

[Philippe Normand]

Created attachment 76096 [details]
Mac PlatformVideoWindow implementation

[Philippe Normand]

Hum I just wondered, maybe PlatformVideoWindowCocoa would be a better name?

[Eric Carlson]

Comment on attachment 76096 [details]
Mac PlatformVideoWindow implementation

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

I won't r+ this because I don't know enough about the gstreamer portion to tell if it is correct or not.


> WebCore/ChangeLog:17
> +        * platform/graphics/gstreamer/GStreamerGWorld.cpp:
> +        (WebCore::gstGWorldSyncMessageCallback):
> +        * platform/graphics/gstreamer/PlatformVideoWindowMac.mm: Added.
> +        (PlatformVideoWindow::PlatformVideoWindow):
> +        (PlatformVideoWindow::~PlatformVideoWindow):
> +        (PlatformVideoWindow::prepareForOverlay):
> +

Looks like you forgot to add PlatformVideoWindowMac.h


> WebCore/platform/graphics/gstreamer/PlatformVideoWindowMac.mm:40
> +PlatformVideoWindow::PlatformVideoWindow()
> +{
> +    m_window = [[NSView alloc] init];
> +    m_videoWindowId = reinterpret_cast<unsigned long>(m_window);
> +}
> +
> +PlatformVideoWindow::~PlatformVideoWindow()
> +{
> +    [m_window release];
> +    m_window = 0;
> +    m_videoWindowId = 0;
> +}

m_window should be a RetainPtr.

[Philippe Normand]

(In reply to comment #3)
> (From update of attachment 76096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76096&action=review
> 
> I won't r+ this because I don't know enough about the gstreamer portion to tell if it is correct or not.
> 
> 
> > WebCore/ChangeLog:17
> > +        * platform/graphics/gstreamer/GStreamerGWorld.cpp:
> > +        (WebCore::gstGWorldSyncMessageCallback):
> > +        * platform/graphics/gstreamer/PlatformVideoWindowMac.mm: Added.
> > +        (PlatformVideoWindow::PlatformVideoWindow):
> > +        (PlatformVideoWindow::~PlatformVideoWindow):
> > +        (PlatformVideoWindow::prepareForOverlay):
> > +
> 
> Looks like you forgot to add PlatformVideoWindowMac.h
> 
> 

The generic PlatformVideoWindow.h header can be used. This is just the platform-specific implementation.

> > WebCore/platform/graphics/gstreamer/PlatformVideoWindowMac.mm:40
> > +PlatformVideoWindow::PlatformVideoWindow()
> > +{
> > +    m_window = [[NSView alloc] init];
> > +    m_videoWindowId = reinterpret_cast<unsigned long>(m_window);
> > +}
> > +
> > +PlatformVideoWindow::~PlatformVideoWindow()
> > +{
> > +    [m_window release];
> > +    m_window = 0;
> > +    m_videoWindowId = 0;
> > +}
> 
> m_window should be a RetainPtr.

Hum but that's specific to the mac platform. m_window is declared in the PlatformVideoWindow.h header, I could define it differently on mac I guess.

[Philippe Normand]

Comment on attachment 76096 [details]
Mac PlatformVideoWindow implementation

Needs a little more work as per Eric's comment.

[Philippe Normand]

Created attachment 78512 [details]
updated patch

[WebKit Review Bot]

Attachment 78512 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h:41:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 78513 [details]
updated patch

[Eric Carlson]

Comment on attachment 78513 [details]
updated patch

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

r+ if you fix the RetainPtr assignment problem.

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h:48
> +        PlatformWidget window() const
> +{
> +#if !PLATFORM(MAC)
> +           return m_window;
> +#else
> +           return m_window.get();
> +#endif
> +}

I know the style bot is happy with it, but this brace/#if indentation is strange. It might be easier to read if this function was moved to the .mm file.


> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowMac.mm:2
> + * Copyright (C) 2010 Igalia S.L

Probably want 2011 here.


> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowMac.mm:31
> +    m_window = [[NSView alloc] init];

This should be m_window.adoptNS([[NSView alloc] init]);


> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowMac.mm:44
> +        m_videoWindow = static_cast<PlatformWidget>(g_value_get_pointer(gst_structure_get_value(message->structure, "nsview")));
> +        [m_window.get() addSubview:m_videoWindow];

Might be worth ASSERT-ing m_videoWindow here.

[Philippe Normand]

I tried moving the window() implementation to the .mm file but then I got a symbol lookup error when linking WebKit. So I kept the #if in the header but it's more readable now, imho

Committed r75590: <http://trac.webkit.org/changeset/75590>