Bug 143049 - [GStreamer] share GL context in pipeline, part 2
Summary: [GStreamer] share GL context in pipeline, part 2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Víctor M. Jáquez L.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-25 08:10 PDT by Víctor M. Jáquez L.
Modified: 2015-03-27 03:31 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.72 KB, patch)
2015-03-25 08:36 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (10.43 KB, patch)
2015-03-25 11:06 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (10.08 KB, patch)
2015-03-26 03:07 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2015-03-27 02:40 PDT, Víctor M. Jáquez L.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Víctor M. Jáquez L. 2015-03-25 08:10:25 PDT
This is a follow up of bug 142693
Comment 1 Víctor M. Jáquez L. 2015-03-25 08:36:29 PDT
Created attachment 249406 [details]
Patch
Comment 2 Carlos Garcia Campos 2015-03-25 09:44:49 PDT
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.
Comment 3 Víctor M. Jáquez L. 2015-03-25 11:06:47 PDT
Created attachment 249414 [details]
Patch
Comment 4 Carlos Garcia Campos 2015-03-25 11:38:38 PDT
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.
Comment 5 Víctor M. Jáquez L. 2015-03-26 03:07:56 PDT
Created attachment 249480 [details]
Patch
Comment 6 WebKit Commit Bot 2015-03-26 06:48:34 PDT
Comment on attachment 249480 [details]
Patch

Rejecting attachment 249480 [details] from review queue.

vjaquez@igalia.com 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.
Comment 7 Zan Dobersek 2015-03-26 11:00:51 PDT
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.
Comment 8 Víctor M. Jáquez L. 2015-03-27 02:40:17 PDT
Created attachment 249559 [details]
Patch
Comment 9 WebKit Commit Bot 2015-03-27 03:31:45 PDT
Comment on attachment 249559 [details]
Patch

Clearing flags on attachment: 249559

Committed r182056: <http://trac.webkit.org/changeset/182056>
Comment 10 WebKit Commit Bot 2015-03-27 03:31:51 PDT
All reviewed patches have been landed.  Closing bug.