Bug 122831 – [GStreamer] Store video-sink in a bin

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

It's already done for the dead NATIVE_VIDEO_FULLSCREEN code path but we need the video-sink in a bin for mediastream too.

[Philippe Normand]

Created attachment 214259 [details]
patch

[Andy Wingo]

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]

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

[Gustavo Noronha (kov)]

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]

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

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

[Gustavo Noronha (kov)]

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

thanks =)

[Philippe Normand]

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

[Xabier Rodríguez Calvar]

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