WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
122831
[GStreamer] Store video-sink in a bin
https://bugs.webkit.org/show_bug.cgi?id=122831
Summary
[GStreamer] Store video-sink in a bin
Philippe Normand
Reported
2013-10-15 07:41:24 PDT
It's already done for the dead NATIVE_VIDEO_FULLSCREEN code path but we need the video-sink in a bin for mediastream too.
Attachments
patch
(6.36 KB, patch)
2013-10-15 08:25 PDT
,
Philippe Normand
no flags
Details
Formatted Diff
Diff
[player] make video-sink a bin
(7.29 KB, patch)
2013-10-29 03:23 PDT
,
Philippe Normand
gustavo
: review-
Details
Formatted Diff
Diff
[player] make video-sink a bin
(8.33 KB, patch)
2013-10-29 07:15 PDT
,
Philippe Normand
gustavo
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Philippe Normand
Comment 1
2013-10-15 08:25:50 PDT
Created
attachment 214259
[details]
patch
Andy Wingo
Comment 2
2013-10-28 02:35:44 PDT
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.
Philippe Normand
Comment 3
2013-10-29 03:23:25 PDT
Created
attachment 215374
[details]
[player] make video-sink a bin
Gustavo Noronha (kov)
Comment 4
2013-10-29 04:58:15 PDT
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.
Philippe Normand
Comment 5
2013-10-29 05:31:34 PDT
(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.
Philippe Normand
Comment 6
2013-10-29 07:15:42 PDT
Created
attachment 215386
[details]
[player] make video-sink a bin
Gustavo Noronha (kov)
Comment 7
2013-10-29 13:08:34 PDT
Comment on
attachment 215386
[details]
[player] make video-sink a bin thanks =)
Philippe Normand
Comment 8
2013-10-30 01:02:42 PDT
Committed
r158258
: <
http://trac.webkit.org/changeset/158258
>
Xabier Rodríguez Calvar
Comment 9
2013-10-30 03:35:35 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug