Summary: | [GStreamer] Store video-sink in a bin | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, glenn, gustavo, jer.noble, menard, mrobinson, pnormand | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 79203, 123437 | ||||||||||
Attachments: |
|
Description
Philippe Normand
2013-10-15 07:41:24 PDT
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. |