Bug 122831

Summary: [GStreamer] Store video-sink in a bin
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: 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 Flags
patch
none
[player] make video-sink a bin
gustavo: review-
[player] make video-sink a bin gustavo: review+

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.