<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GStreamer] Support a direct GPU-to-GPU copy of video textures to WebGL"
href="https://bugs.webkit.org/show_bug.cgi?id=159928#c15">Comment # 15</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GStreamer] Support a direct GPU-to-GPU copy of video textures to WebGL"
href="https://bugs.webkit.org/show_bug.cgi?id=159928">bug 159928</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=289463&action=diff" name="attach_289463" title="Patch">attachment 289463</a> <a href="attachment.cgi?id=289463&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=289463&action=review">https://bugs.webkit.org/attachment.cgi?id=289463&action=review</a>
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:700
> +bool MediaPlayerPrivateGStreamerBase::paintToCairoSurface(RefPtr<cairo_surface_t> outputSurface, cairo_device_t* device, GstVideoInfo* videoInfo)</span >
I don't think you need to pass the smart pointer to this function, and in that case I would pass a reference instead of a copy, but I think you can simply pass the pointer.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:702
> + // m_sampleMutex should be locked by the caller</span >
I would move this comment before the function, saying something like: // This should be called with the sample mutex locked.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:710
> - IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> + IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(videoInfo), GST_VIDEO_INFO_HEIGHT(videoInfo));</span >
Both callers do this, maybe you could pass this as a const reference.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:713
> IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;</span >
Ditto.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:773
> + GLContext* glContext = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();
> + cairo_device_t* device = glContext->cairoDevice();
> +
> + // Thread-awareness is a huge performance hit on non-Intel drivers.
> + cairo_gl_device_set_thread_aware(device, FALSE);
> +
> + IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> + IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;
> +</span >
Don't you need to make the context current in this case?
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:813
> + GstVideoInfo videoInfo;
> + WTF::GMutexLocker<GMutex> lock(m_sampleMutex);
> + if (!getSampleVideoInfo(m_sample.get(), videoInfo))
> + return nullptr;
> +
> + GLContext* context = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();
> + context->makeContextCurrent();
> + cairo_device_t* device = context->cairoDevice();
> +
> + // Thread-awareness is a huge performance hit on non-Intel drivers.
> + cairo_gl_device_set_thread_aware(device, FALSE);
> +
> + IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> + IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;</span >
All this is mostly duplicated code, could we move it to a helper function? maybe a function that prepares the context and returns the GLContext as return value, and video info, and sizes as output parameters
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:119
> + bool paintToCairoSurface(RefPtr<cairo_surface_t> outputSurface, cairo_device_t*, GstVideoInfo*);</span >
This should be private.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>