Created attachment 342470 [details] [review]
patch for add video/audio/text sink property

For the video-sink there's also the GstPlayerVideoRenderer interface. For audio/text sinks I'd prefer something similar too, something that does not involve juggling with raw GStreamer elements from the application.

Created attachment 342488 [details] [review]
patch for set video-sink via video-renderer.c

in gstplayer-video-overlay-video-renderer..c 
gst_player_video_overlay_video_renderer_create_video_sink() will always return NULL, and therefore, in gstplayer gst_player_main() thread, the custom video sink will not be set successfully.

The patch add "video-sink" property for overlayvideorender, so that it could  be set outside. also return the video-sink element via gst_player_video_overlay_video_renderer_create_video_sink()

also update gst_player_video_overlay_video_renderer_new() to set video-sink when creating the renderer.

Hi, Slomo,
    How about the above patch for the video sink set ?  do you have any advice about it?

Thanks
Lyon

Review of attachment 342488 [details] [review]:

This looks generally like the right approach

::: gst-libs/gst/player/gstplayer-video-overlay-video-renderer.c
@@ +86,3 @@
       break;
+    case VIDEO_OVERLAY_VIDEO_RENDERER_PROP_VIDEO_SINK:
+      self->video_sink = g_value_get_object (value);

_dup_object() and unref() in finalize()

@@ +204,3 @@
 GstPlayerVideoRenderer *
+gst_player_video_overlay_video_renderer_new (gpointer window_handle,
+    GstElement * video_sink)

Add documentation too

::: gst-libs/gst/player/gstplayer-video-overlay-video-renderer.h
@@ +42,2 @@
 GType gst_player_video_overlay_video_renderer_get_type (void);
+GstPlayerVideoRenderer * gst_player_video_overlay_video_renderer_new (gpointer window_handle, GstElement * video_sink);

Don't change the API please, better add a new "_new_with_sink()" function here

Created attachment 343211 [details] [review]
Update patch for videorenderer video sink set

Hi, Slomo,
    Please see attached file for the update patch.

Thanks
Lyon

Review of attachment 343211 [details] [review]:

::: gst-libs/gst/player/gstplayer-video-overlay-video-renderer.c
@@ +229,3 @@
+
+  return g_object_new (GST_TYPE_PLAYER_VIDEO_OVERLAY_VIDEO_RENDERER,
+      "window-handle", window_handle, "video-sink", video_sink, NULL);

You pass a string to a GstElement* property, that doesn't work

::: gst-libs/gst/player/gstplayer-video-overlay-video-renderer.h
@@ +42,3 @@
 GType gst_player_video_overlay_video_renderer_get_type (void);
 GstPlayerVideoRenderer * gst_player_video_overlay_video_renderer_new (gpointer window_handle);
+GstPlayerVideoRenderer * gst_player_video_overlay_video_renderer_new_with_sink (gpointer window_handle, const gchar * video_sink_name);

A GstElement* here would be better. You might want to pass something that does not have a GstElementFactory, like some custom bin with various other elements

Created attachment 343293 [details] [review]
Update patch for set video sink.

Update the patch, change back to pass GstElement * video_sink as parameter

Hi, Slomo,
    How about the updated patch for video-sink setting?

Thanks
Lyon

Review of attachment 343293 [details] [review]:

Thanks, mostly looks good just one more thing

::: gst-libs/gst/player/gstplayer-video-overlay-video-renderer.c
@@ +86,3 @@
       break;
+    case VIDEO_OVERLAY_VIDEO_RENDERER_PROP_VIDEO_SINK:
+      self->video_sink = g_value_dup_object (value);

This should actually probably work like the video-sink property on playsink. Please check the playsink code, but IIRC it does get_object() and then ref_sink() on the object.

This also needs to be mentioned in the docs then, that this function takes the argument as "transfer floating".

Created attachment 343633 [details] [review]
Update patch for set video sink

Update patch for set video sink
- modify set/get property of video-sink

commit b22fec8293521d9270b679f29415bc713eeb3749
Author: Lyon Wang <lyon.wang@nxp.com>
Date:   Tue Dec 27 17:13:58 2016 +0800

    player: Add support for selecting a specific video sink
    
    - Add overlay video renderer "video-sink" property, so that can be set
    - In create_video_sink, it returns video sink instead of always NULL
    - Add new renderer_new_with_sink() API to set video sink
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776490