Bug 122831

Created attachment 214259 [details] patch

Comment on attachment 214259 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=214259&action=review Drive-by review. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:539 > + m_videoSinkBin = gst_bin_new(0); nullptr > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:581 > g_object_set(m_fpsSink, "video-sink", m_webkitVideoSink.get(), NULL); nullptr here, and elsewhere too > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:606 > + // Add a ghostpad to the bin so it can proxy to tee. "ghost pad". Also I don't think the comment is correct any more -- here we are adding a sink pad to the video sink bin, no? And the child that it proxies to is not necessarily a tee it seems. I would remove the comment.

Created attachment 215374 [details] [player] make video-sink a bin

Comment on attachment 215374 [details] [player] make video-sink a bin View in context: https://bugs.webkit.org/attachment.cgi?id=215374&action=review LGTM in general, but it lacks a changelog entry. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:589 > } > > if (!m_fpsSink) { This should be an else block. I think the actualVideoSink name is misleading because it may be something that is not the actual video sink heh. Is videoSink taken? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:603 > - return actualVideoSink; > + firstChild = actualVideoSink; You could initialize firstChild to this instead of doing it in the else block, just a nit.

(In reply to comment #4) > (From update of attachment 215374 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215374&action=review > > LGTM in general, but it lacks a changelog entry. > OOPS :) > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:589 > > } > > > > if (!m_fpsSink) { > > This should be an else block. Hum, I don't think so because the variable is set in the if() branch in some cases, so checking it again here makes sense, I believe. > I think the actualVideoSink name is misleading because it may be something that is not the actual video sink heh. Is videoSink taken? > Good point, videoSink is not taken, yet! > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:603 > > - return actualVideoSink; > > + firstChild = actualVideoSink; > > You could initialize firstChild to this instead of doing it in the else block, just a nit. Right. I'll upload a new version after lunch. Thanks for the review.

Created attachment 215386 [details] [player] make video-sink a bin

Comment on attachment 215386 [details] [player] make video-sink a bin thanks =)

Committed r158258: <http://trac.webkit.org/changeset/158258>

(In reply to comment #2) > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:539 > > + m_videoSinkBin = gst_bin_new(0); > > nullptr > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:581 > > g_object_set(m_fpsSink, "video-sink", m_webkitVideoSink.get(), NULL); > > nullptr here, and elsewhere too I am not sure we want to use nullptr here instead of NULL, because in this case we are not defining or using WebKit/WebCore API, but GStreamer one and that's NULL based.