Bug 143049 – [GStreamer] share GL context in pipeline, part 2

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Víctor M. Jáquez L.]

This is a follow up of bug 142693

[Víctor M. Jáquez L.]

Created attachment 249406 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 249406 [details]
Patch

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

Thanks for the follow up patch

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2047
> +bool MediaPlayerPrivateGStreamer::createGstGL()

create functions normally return the created object, I would use ensureGstGLContext() instead.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2062
> +#else
> +        return false;
> +#endif

I don't think this is possible.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2065
> +    // USE(GSTREAMER_GL) implies USE(3D_GRAPHICS)

No need to clarify this now.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2066
> +    auto contextHandle = reinterpret_cast<guintptr>(GLContext::sharingContext()->platformContext());

I would use PlatformGraphicsContext3D here instead of the auto to make this clear, and without the cast.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2076
> +#if USE(GLX)
> +    glPlatform = GST_GL_PLATFORM_GLX;
> +#elif USE(EGL)
> +    glPlatform = GST_GL_PLATFORM_EGL;
> +#endif

At this point either GLX or EFL are defined, because we are inside a USE(GSTREAMER_GL)  block, so I would simplify this by avoiding the previous declaration and inicitalization to GST_GL_PLATFORM_ANY.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2079
> +#if USE(GLES2)
> +    glAPI = GST_GL_API_GLES2;
> +#endif

And here I would also move the declaration inside the ifdef block, adding #else GstGLAPI glAPI = GST_GL_API_OPENGL;

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2080
> +    m_glContext = gst_gl_context_new_wrapped(m_glDisplay.get(), contextHandle, glPlatform, glAPI);

Here I would cast the contextHandle.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2083
> +    return true;

Do we really want to return true when !USE(GSTREAMER_GL)? I think this function should only be defined if USE(GSTREAMER_GL), since it's only called inside a USE(GSTREAMER_GL) block.

[Víctor M. Jáquez L.]

Created attachment 249414 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 249414 [details]
Patch

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

> Source/WebCore/ChangeLog:28
> +        (WebCore::MediaPlayerPrivateGStreamer::handleSyncMessage): Changed the
> +        signature to use bool instead of gboolean.

This is no longer true, now is void for some reason.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:112
> -static gboolean mediaPlayerPrivateSyncMessageCallback(GstBus*, GstMessage* message, MediaPlayerPrivateGStreamer* player)
> +static void mediaPlayerPrivateSyncMessageCallback(GstBus*, GstMessage* message, MediaPlayerPrivateGStreamer* player)
>  {
> -    return player->handleSyncMessage(message);
> +    player->handleSyncMessage(message);
>  }

Don't we need to update the caller of mediaPlayerPrivateSyncMessageCallback to not handle the return value?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:934
> -gboolean MediaPlayerPrivateGStreamer::handleSyncMessage(GstMessage* message)
> +void MediaPlayerPrivateGStreamer::handleSyncMessage(GstMessage* message)

so, this is now void?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:965
> -    return FALSE;
> +    return;

I guess we don't need any return here now that this is void.

[Víctor M. Jáquez L.]

Created attachment 249480 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 249480 [details]
Patch

Rejecting attachment 249480 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Zan Dobersek]

Comment on attachment 249480 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Carlos already reviewed this, so NOBODY should be replaced with his full name. After that landing through cq shouldn't be a problem.

> ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Ditto.

[Víctor M. Jáquez L.]

Created attachment 249559 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 249559 [details]
Patch

Clearing flags on attachment: 249559

Committed r182056: <http://trac.webkit.org/changeset/182056>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.