Bug 143301 – [ThreadedCompositor] Support HTML5 Video

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gwang Yoon Hwang]

[ThreadedCompositor] Support HTML5 Video

[Gwang Yoon Hwang]

Created attachment 249908 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 249909 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 266996 [details]
Patch

[Gwang Yoon Hwang]

Finally, it reaches to the stage of review.
Zan, could you take a look at it?

[Zan Dobersek]

Comment on attachment 266996 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:111
> +    GstVideoFrameHolder(GstSample& sample)

It's uncommon to use references with C API objects (e.g. objects from the GLib, GStreamer and GTK+ APIs). In both places you use this reference in this constructor, you have to use the & unary operator just to retrieve the address. So I wouldn't advise using a C++ reference for this parameter.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:125
> +        m_videoFrame = new GstVideoFrame();
> +        if (UNLIKELY(!gst_video_frame_map(m_videoFrame, &videoInfo, buffer, static_cast<GstMapFlags>(GST_MAP_READ | GST_MAP_GL))))
> +            return;

As the macro in the branch hints, this would be unlikely, but in case the gst_video_frame_map() call fails you would end up leaking the heap-allocated m_videoFrame object.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:131
> +    ~GstVideoFrameHolder() override

Heh, regarding the virtual-vs-override-specifier, for the destructors I would very much prefer we stuck with the virtual specifier for the destructors. I don't think override specifier is used in this context anywhere in the WebKit codebase.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:140
> +        gst_gl_window_send_message_async(gstWindow, (GstGLWindowCB) unmapVideoFrameCallback, m_videoFrame, (GDestroyNotify) freeVideoFrameCallback);

There's no need for the extra space in the GstGLWindowCB and GDestroyNotify casts.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:147
> +    }
> +    static void freeVideoFrameCallback(GstVideoFrame* videoFrame)

You can afford a separate line between these two.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:152
> +    IntSize size() const { return m_size; }

In this case we normally return a const reference to the member variable, while for integers or enums a copy is returned (which usually fits into one register).

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:454
> +    texture.updateContents(srcData, WebCore::IntRect(WebCore::IntPoint(0, 0), size), WebCore::IntPoint(0, 0), stride, BitmapTexture::UpdateCannotModifyOriginalImageData);

You can remove the size variable and just use the 4-parameter IntRect constructor.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:510
> +    unique_ptr<TextureMapperPlatformLayerBuffer> buffer = m_platformLayerProxy->getAvailableBuffer(size, GraphicsContext3D::DONT_CARE);

std::unique_ptr<>

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:556
> +    updateOnCompositorThread();

This method could just be named updateTexture(), since in this case the name gives false impression.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:602
> +#if USE(COORDINATED_GRAPHICS_THREADED)
> +        return;

Should we also place an ASSERT_NOT_REACHED() here? Or do we actually call this method even with the threaded compositor?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:653
> +        textureMapper->drawTexture(*texture.get(), targetRect, matrix, opacity);

No need to retrieve the raw pointer from the RefPtr object -- just dereference the RefPtr object directly.

[Gwang Yoon Hwang]

Created attachment 267006 [details]
Patch

[Gwang Yoon Hwang]

Thanks for review.

(In reply to comment #5)
> Comment on attachment 266996 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266996&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:111
> > +    GstVideoFrameHolder(GstSample& sample)
> 
> It's uncommon to use references with C API objects (e.g. objects from the
> GLib, GStreamer and GTK+ APIs). In both places you use this reference in
> this constructor, you have to use the & unary operator just to retrieve the
> address. So I wouldn't advise using a C++ reference for this parameter.
> 

Ah, it looks familiar. Fixed as you suggest.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:125
> > +        m_videoFrame = new GstVideoFrame();
> > +        if (UNLIKELY(!gst_video_frame_map(m_videoFrame, &videoInfo, buffer, static_cast<GstMapFlags>(GST_MAP_READ | GST_MAP_GL))))
> > +            return;
> 
> As the macro in the branch hints, this would be unlikely, but in case the
> gst_video_frame_map() call fails you would end up leaking the heap-allocated
> m_videoFrame object.
> 

Nice point! Fixed.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:131
> > +    ~GstVideoFrameHolder() override
> 
> Heh, regarding the virtual-vs-override-specifier, for the destructors I
> would very much prefer we stuck with the virtual specifier for the
> destructors. I don't think override specifier is used in this context
> anywhere in the WebKit codebase.
> 

Changed to use virtual.

> > 
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:140
> > +        gst_gl_window_send_message_async(gstWindow, (GstGLWindowCB) unmapVideoFrameCallback, m_videoFrame, (GDestroyNotify) freeVideoFrameCallback);
> 
> There's no need for the extra space in the GstGLWindowCB and GDestroyNotify
> casts.
> 

nice. Fixed.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:147
> > +    }
> > +    static void freeVideoFrameCallback(GstVideoFrame* videoFrame)
> 
> You can afford a separate line between these two.
> 

Ditto.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:152
> > +    IntSize size() const { return m_size; }
> 
> In this case we normally return a const reference to the member variable,
> while for integers or enums a copy is returned (which usually fits into one
> register).
> 

Fixed to use const IntSize&

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:454
> > +    texture.updateContents(srcData, WebCore::IntRect(WebCore::IntPoint(0, 0), size), WebCore::IntPoint(0, 0), stride, BitmapTexture::UpdateCannotModifyOriginalImageData);
> 
> You can remove the size variable and just use the 4-parameter IntRect
> constructor.
> 

Modified as you suggest.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:510
> > +    unique_ptr<TextureMapperPlatformLayerBuffer> buffer = m_platformLayerProxy->getAvailableBuffer(size, GraphicsContext3D::DONT_CARE);
> 
> std::unique_ptr<>
> 

Ditto.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:556
> > +    updateOnCompositorThread();
> 
> This method could just be named updateTexture(), since in this case the name
> gives false impression.
> 

It is always hard to find nice name. :)
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:602
> > +#if USE(COORDINATED_GRAPHICS_THREADED)
> > +        return;
> 
> Should we also place an ASSERT_NOT_REACHED() here? Or do we actually call
> this method even with the threaded compositor?
> 

Let's use ASSERT_NOT_REACHED at this point.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:653
> > +        textureMapper->drawTexture(*texture.get(), targetRect, matrix, opacity);
> 
> No need to retrieve the raw pointer from the RefPtr object -- just
> dereference the RefPtr object directly.

Fixed.

[Zan Dobersek]

Comment on attachment 267006 [details]
Patch

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

OK, r=me.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:499
> +    return;

This return isn't strictly required.

[Gwang Yoon Hwang]

Created attachment 267009 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267009 [details]
Patch

Rejecting attachment 267009 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 267009, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/537425

[Gwang Yoon Hwang]

Created attachment 267011 [details]
Patch

[Michael Catanzaro]

(In reply to comment #5)
> It's uncommon to use references with C API objects (e.g. objects from the
> GLib, GStreamer and GTK+ APIs). In both places you use this reference in
> this constructor, you have to use the & unary operator just to retrieve the
> address. So I wouldn't advise using a C++ reference for this parameter.

I prefer to use C++ references as function parameters to document that the parameter cannot be nullptr. I believe we've been attempting to replace pointers with references throughout the tree, to reduce potential for mistakes with nullptr.

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #5)
> > It's uncommon to use references with C API objects (e.g. objects from the
> > GLib, GStreamer and GTK+ APIs). In both places you use this reference in
> > this constructor, you have to use the & unary operator just to retrieve the
> > address. So I wouldn't advise using a C++ reference for this parameter.
> 
> I prefer to use C++ references as function parameters to document that the
> parameter cannot be nullptr. I believe we've been attempting to replace
> pointers with references throughout the tree, to reduce potential for
> mistakes with nullptr.

But not for GObjects, it's so confusing to me seeing const GObject&, for example.

[Zan Dobersek]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #5)
> > > It's uncommon to use references with C API objects (e.g. objects from the
> > > GLib, GStreamer and GTK+ APIs). In both places you use this reference in
> > > this constructor, you have to use the & unary operator just to retrieve the
> > > address. So I wouldn't advise using a C++ reference for this parameter.
> > 
> > I prefer to use C++ references as function parameters to document that the
> > parameter cannot be nullptr. I believe we've been attempting to replace
> > pointers with references throughout the tree, to reduce potential for
> > mistakes with nullptr.
> 
> But not for GObjects, it's so confusing to me seeing const GObject&, for
> example.

I agree, and like I said I feel the same about every C API that operates with raw pointers.

[WebKit Commit Bot]

Comment on attachment 267011 [details]
Patch

Clearing flags on attachment: 267011

Committed r193836: <http://trac.webkit.org/changeset/193836>

[Gwang Yoon Hwang]

The patch was landed. Closing the bug.