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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122831





--- Comment #5 from Philippe Normand <pnormand at igalia.com>  2013-10-29 05:31:34 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.