Created attachment 338080 [details] [review]
get_track_num and set custom sink API

Created attachment 338081 [details] [review]
display area setting API

Created attachment 338083 [details] [review]
get video thumbnail API

Created attachment 338084 [details] [review]
set_seek_accurate API

Created attachment 338085 [details] [review]
subtitle setting API

Review of attachment 338080 [details] [review]:

Please split this into two patches

::: gst-libs/gst/player/gstplayer.h
@@ +200,3 @@
+guint  gst_player_get_video_num (GstPlayer * player);
+
+guint  gst_player_get_subtitle_num (GstPlayer * player);

These 3 functions are not needed. You can already get the same information from the media info

@@ +213,3 @@
+GstElement * gst_player_get_audio_sink(GstPlayer * player);
+
+GstElement * gst_player_get_text_sink(GstPlayer * player);

For video sinks, there is the GstPlayerVideoRenderer interface (for the video overlay implementation one could think of adding sink selection by factory name though). For audio/text sinks I would prefer to also have some abstraction here instead of exposing GStreamer internals that much

Review of attachment 338081 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +4489,3 @@
+    g_object_set(G_OBJECT(video_sink), "reconfig", 1, NULL);
+  } else if (g_object_class_find_property (gobjclass,"rotate-method")) {
+    g_object_set(G_OBJECT(video_sink), "rotate-method", rotation/90, NULL);

Rotation should be handled via the GstVideoDirection interface, and we should expose the enum of it here. This should also be inside the video renderer interface probably

@@ +4564,3 @@
+    g_object_get(G_OBJECT(video_sink), "overlay-top", &area->offsety, NULL);
+    g_object_get(G_OBJECT(video_sink), "overlay-width", &area->width, NULL);
+    g_object_get(G_OBJECT(video_sink), "overlay-height", &area->height, NULL);

This is all non-standard and not provided by any of our sinks. Also see the other comments

::: gst-libs/gst/player/gstplayer.h
@@ +236,3 @@
+void gst_player_expose_video(GstPlayer * player);
+
+gboolean gst_player_set_window(GstPlayer * player, guintptr winhandle);

This should be part of the GstPlayerVideoRenderer interface or specific implementations, and everything but the first two functions are part of the GstPlayerVideoOverlayVideoRenderer implementation

Review of attachment 338083 [details] [review]:

::: gst-libs/gst/player/gstplayer.h
@@ +244,3 @@
+    gint width;
+    gint height;
+}image_info;

This should be a proper type, e.g. a GstSample. It should contain information about the format (caps) and some kind of better memory abstraction.

@@ +247,3 @@
+
+gboolean gst_player_get_video_thumbnail(GstPlayer * player, gint sec,
+      image_info *thumbnail);

It should probably not do any seeks but get a snapshot of the current position (last-sample property), and convert it to another format if needed (OTOH that's easily done with the gst_video_convert_sample() API).


If you need thumbnails at arbitrary positions, this seems like something for which there should be a separate API. not GstPlayer. Some thumbnailing API inside GstDiscoverer probably.

Review of attachment 338084 [details] [review]:

::: gst-libs/gst/player/gstplayer.h
@@ +106,3 @@
 
+void         gst_player_set_seek_accurate             (GstPlayer    * player,
+                                                       gboolean accurate);

This should probably become part of the player configuration API, otherwise makes sense

Review of attachment 338085 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +4833,3 @@
+  g_return_val_if_fail (GST_IS_PLAYER (self), FALSE);
+
+  element = gst_bin_get_by_name (GST_BIN (self->playbin), "overlay");//textoverlay

This can't really work and will always be NULL. You could get the subtitleoverlay from playsink probably, and set these things on it. But that will also only work for font-desc and subtitle-encoding, not the positioning.

Also the positioning depends a lot on the subtitle format and renderer (OTOH the other settings too).


All this should probably get settings on playsink first, and then we can expose those here

::: gst-libs/gst/player/gstplayer.h
@@ +265,3 @@
+  SUBTITLE_V_ALIGN_POSITION = 3,
+  SUBTITLE_V_ALIGN_CENTER   = 4
+} SubtitleVAlign;

Types and enums need proper namespacing

(In reply to Sebastian Dröge (slomo) from comment #7)
> Review of attachment 338081 [details] [review] [review]:
> 
> ::: gst-libs/gst/player/gstplayer.c
> @@ +4489,3 @@
> +    g_object_set(G_OBJECT(video_sink), "reconfig", 1, NULL);
> +  } else if (g_object_class_find_property (gobjclass,"rotate-method")) {
> +    g_object_set(G_OBJECT(video_sink), "rotate-method", rotation/90, NULL);
> 
> Rotation should be handled via the GstVideoDirection interface, and we
> should expose the enum of it here. This should also be inside the video
> renderer interface probably

See also bug #765309 for the rotation. Should be handled in that bug.


It would probably also make sense to create one bug per feature and then we can discuss the specific details there. API-wise, some discussions are needed here to find a good solution.


In any case, thanks for all your patches :) We should try to find appropriate API for all these features for GstPlayer, they all seem useful to have.

slomo, Thanks for all the comments

I will have a re-work according to your comments

Hi, slomo
    I have some questions about your comments.
- for above gst_player_get_video_num / _get_audio_num / get_text_num API
you said there is no need to have these API as we can get media_info by _get_xxx_streams. yes , we can the Glist of audio/video/text stream, but will it be more convenient to get total track num directly instead of get the list and count the track num?

- For the rotation part
you mentioned we can use videoflip plugin to realize rotation. but considering the performance, if using videplip by software process, on embedded system, it is much slower than directly using video-sink 's "rotate-method"/"rotate" property. Like in glimagesink, there is 'rotate-method'. by the hardware video-sink, the performance would be much better than videoflip.

(In reply to Lyon from comment #13)
> Hi, slomo
>     I have some questions about your comments.
> - for above gst_player_get_video_num / _get_audio_num / get_text_num API
> you said there is no need to have these API as we can get media_info by
> _get_xxx_streams. yes , we can the Glist of audio/video/text stream, but
> will it be more convenient to get total track num directly instead of get
> the list and count the track num?

Indeed, but it should be part of the media info API then. Just store the size together with the list (or use a GQueue).

> - For the rotation part
> you mentioned we can use videoflip plugin to realize rotation. but
> considering the performance, if using videplip by software process, on
> embedded system, it is much slower than directly using video-sink 's
> "rotate-method"/"rotate" property. Like in glimagesink, there is
> 'rotate-method'. by the hardware video-sink, the performance would be much
> better than videoflip.

That's why I mentioned the GstVideoOrientation interface. See bug #765309. playsink should use videoflip if nothing else is possible (like it uses e.g. videobalance now), and otherwise use that interface on the video sink directly.

Hi, slomo
  I have some more question about gstplayer video render API when I was trying to implement set video sink API in video render.

I see there is an API "GstElement * gst_player_video_renderer_create_video_sink(render, player)" 

Is the purpose of this API to create a video-sink which can be set to playbin ?
However, when I check the definition of this function, it will invoke function pointer iface->create_video_sink, and the realization in gstplayer-video-overlay-video-renderer.c is function gst_player_video_overlay_video_renderer_create_video_sink(), but this function actually just get the pipeline and set the renderer overlay , hen set window handler and render rectangle, then just return NULL. 
I am a little bit confused, should it return some video-sink element to be set to playbin ?

Or shall there be another approach that we can use video-sink name to set to the renderer's video_overlay like below?
  video_sink = gst_parse_bin_from_description(options.video_sink_name, TRUE, NULL);  gst_player_video_overlay_video_renderer_set_overlay(GST_PLAYER_VIDEO_OVERLAY_VIDEO_RENDERER(renderer), GST_VIDEO_OVERLAY(video_sink));

looking forward to your comments

Thanks
Lyon

related ticket for adding gstplayer API mentioned above:
set accurate seek:     https://bugzilla.gnome.org/show_bug.cgi?id=773521
get track num:         https://bugzilla.gnome.org/show_bug.cgi?id=773570
get video thumbnail:   https://bugzilla.gnome.org/show_bug.cgi?id=773709

Let's close this one then and continue discussions there (and other new bugs) :)