<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#c17">Comment # 17</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:olivier.blin@softathome.com" title="Olivier Blin <olivier.blin@softathome.com>"> <span class="fn">Olivier Blin</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=159928#c15">comment #15</a>)
<span class="quote">> 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>
>
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:700
> > +bool MediaPlayerPrivateGStreamerBase::paintToCairoSurface(RefPtr<cairo_surface_t> outputSurface, cairo_device_t* device, GstVideoInfo* videoInfo)
>
> 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 >
Right, I have also simplified the callers, the WebGL case does not need a smart pointer at all.
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:702
> > + // m_sampleMutex should be locked by the caller
>
> I would move this comment before the function, saying something like: //
> This should be called with the sample mutex locked.
>
> > 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));
>
> Both callers do this, maybe you could pass this as a const reference.</span >
Ok
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:713
> > IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;
>
> Ditto.</span >
Ok
<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;
> > +
>
> Don't you need to make the context current in this case?</span >
See Miguel's answer.
<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;
>
> 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 >
I am not sure it is worth it, it is not much more readable after being extracted, but I will do so in the next patch proposal.
Also, the cairo_device needs to be kept around as well.
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:119
> > + bool paintToCairoSurface(RefPtr<cairo_surface_t> outputSurface, cairo_device_t*, GstVideoInfo*);
>
> This should be private.</span >
Yes, moved it to together with other protected methods.
Thanks for your feedback!</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>