Bug 122831 - [GStreamer] Store video-sink in a bin
Summary: [GStreamer] Store video-sink in a bin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 79203 123437
  Show dependency treegraph
 
Reported: 2013-10-15 07:41 PDT by Philippe Normand
Modified: 2013-10-30 03:35 PDT (History)
9 users (show)

See Also:


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
gns: review-
Details | Formatted Diff | Diff
[player] make video-sink a bin (8.33 KB, patch)
2013-10-29 07:15 PDT, Philippe Normand
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 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.
Comment 1 Philippe Normand 2013-10-15 08:25:50 PDT
Created attachment 214259 [details]
patch
Comment 2 Andy Wingo 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.
Comment 3 Philippe Normand 2013-10-29 03:23:25 PDT
Created attachment 215374 [details]
[player] make video-sink a bin
Comment 4 Gustavo Noronha (kov) 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.
Comment 5 Philippe Normand 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.
Comment 6 Philippe Normand 2013-10-29 07:15:42 PDT
Created attachment 215386 [details]
[player] make video-sink a bin
Comment 7 Gustavo Noronha (kov) 2013-10-29 13:08:34 PDT
Comment on attachment 215386 [details]
[player] make video-sink a bin

thanks =)
Comment 8 Philippe Normand 2013-10-30 01:02:42 PDT
Committed r158258: <http://trac.webkit.org/changeset/158258>
Comment 9 Xabier Rodríguez Calvar 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.