[webkit-reviews] review granted: [Bug 200626] [GL][GStreamer] Instantiate GstGLContext when the shared GLContext is created : [Attachment 391535] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Víctor M. Jáquez L.
<vjaquez at igalia.com>'s request for review:
Bug 200626: [GL][GStreamer] Instantiate GstGLContext when the shared GLContext
is created
https://bugs.webkit.org/show_bug.cgi?id=200626

Attachment 391535: Patch

https://bugs.webkit.org/attachment.cgi?id=391535&action=review




--- Comment #24 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 391535
  --> https://bugs.webkit.org/attachment.cgi?id=391535
Patch

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

This is the idea, only a few more details. Thanks!

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:156
> +    destroyGstGLContext();

Use GRefPtr and you don't need this.

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:235
> +    destroyGstGLContext();

And here you can just set both members	to nullptr.

> Source/WebCore/platform/graphics/PlatformDisplay.h:81
> +    WEBCORE_EXPORT GstGLDisplay* sharingGstGLDisplay();
> +    WEBCORE_EXPORT GstGLContext* sharingGstGLContext();

We don't use WEBCORE_EXPORT macros. I know I proposed these names before, but
now I've realized the "sharing" is confusing here. The sharingGLContext is the
one use to share GL resources like textures. The GstGL context is the one used
by Gst for rendering, so I would remove the sharing prefix. gstGLDisplay() and
gstGLContext(). I'm sorry I didn't realize before. I also think we can make
these const by marking the members as mutable.

> Source/WebCore/platform/graphics/PlatformDisplay.h:118
> +    GstGLDisplay* m_gstGLDisplay;
> +    GstGLContext* m_gstGLContext;

GRefPtr

> Source/WebCore/platform/graphics/PlatformDisplay.h:120
> +#else
> +    void destroyGstGLContext() { };

You shouldn't need this, add #if ENABLE(VIDEO) && USE(GSTREAMER_GL) guards when
accessing the members.

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:124
> +    GRefPtr<GstGLDisplay> sharedGLDisplay =
sharedDisplay.sharingGstGLDisplay();
> +    GRefPtr<GstGLContext> sharedGLContext =
sharedDisplay.sharingGstGLContext();

Why do we need to take a reference here? Since diaply and context are not
expected to be nullptr at this point, add ASSERTS to ensure it.