Bug 143301

Created attachment 249908 [details] Patch

Created attachment 249909 [details] Patch

Created attachment 266996 [details] Patch

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

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.

Created attachment 267006 [details] Patch

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.

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.

Created attachment 267009 [details] Patch

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

Created attachment 267011 [details] Patch

(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.

(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.

(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.

Comment on attachment 267011 [details] Patch Clearing flags on attachment: 267011 Committed r193836: <http://trac.webkit.org/changeset/193836>

The patch was landed. Closing the bug.