<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#c20">Comment # 20</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#c19">comment #19</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=289542&action=diff" name="attach_289542" title="Patch">attachment 289542</a> <a href="attachment.cgi?id=289542&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=289542&action=review">https://bugs.webkit.org/attachment.cgi?id=289542&action=review</a>
>
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:701
> > +GLContext* MediaPlayerPrivateGStreamerBase::prepareContextForCairoPaint(cairo_device_t*& device, GstVideoInfo& videoInfo, IntSize& size, IntSize& rotatedSize)
>
> We don't need to return the device, you can get it from the context.</span >
Ok
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:710
> > + cairo_gl_device_set_thread_aware(device, FALSE);
>
> You can do cairo_gl_device_set_thread_aware(glContext->cairoDevice(),
> FALSE); directly.</span >
Ok
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:773
> > + cairo_device_t* device;
>
> So, this is not needed.</span >
Ok
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:778
> > + if (!prepareContextForCairoPaint(device, videoInfo, size, rotatedSize))
> > + return false;
>
> Get the context here and use it below to pass the cairo device to
> paintToCairoSurface()</span >
Ok
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:784
> > + // cairo_gl_surface_create_for_texture sets these parameters to GL_NEAREST, so we need to backup them
>
> Nit: finish comments with '.'</span >
Ok
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:806
> > + cairo_device_t* device;
>
> Same here.</span >
Ok
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:813
> > + GLContext* context = prepareContextForCairoPaint(device, videoInfo, size, rotatedSize);
> > + if (!context)
> > + return nullptr;
> > + context->makeContextCurrent();
>
> I still think that making the context current in prepareContextForCairoPaint
> wouldn't hurt even if it's not needed in the other case.</span >
Done as well
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:819
> > + cairo_surface_t* rotatedSurface = cairo_gl_surface_create(device, CAIRO_CONTENT_COLOR_ALPHA, rotatedSize.width(), rotatedSize.height());
> > + if (!paintToCairoSurface(rotatedSurface, device, videoInfo, size, rotatedSize))
> > + return nullptr;
> > +
> > + return adoptRef(rotatedSurface);
>
> The smart pointer was correct here, now you are leaking the surface if
> paintToCairoSurface() returns false.</span >
That's right, I will fix for both cases.
Thanks for spotting this.
<span class="quote">> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:136
> > + bool paintToCairoSurface(cairo_surface_t* outputSurface, cairo_device_t*, GstVideoInfo&, const IntSize&, const IntSize&);
>
> outputSurface can be omitted in this case</span >
Ok</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>