[Bug 773709] player: Add get video thumbnail API

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773709

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #343219|none                        |needs-work
             status|                            |

--- Comment #9 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 343219:
 --> (https://bugzilla.gnome.org/review?bug=773709&attachment=343219)

::: gst-libs/gst/player/gstplayer.c
@@ +4327,3 @@
+
+  caps = gst_sample_get_caps (sample);
+  if (!caps) {

Does this ever happen? When? That seems like a bug

@@ +4351,3 @@
+GstSample *
+gst_player_get_video_thumbnail (GstPlayer * self,
+    GstPlayerThumbnail_info * thumbnail_info, GstClockTime position)

I don't think this is good API as it interrupts playback. Such thumbnailing
should be handled outside the player IMHO in a separate API, or if the
application wants this behaviour it can be implemented application-side. I
wouldn't know why an application would like this though, it's not very nice
behaviour for the user experience.

::: gst-libs/gst/player/gstplayer.h
@@ +220,3 @@
+    gint height;
+    gint par_x;
+    gint par_y;

We usually call this par_n and par_d

@@ +221,3 @@
+    gint par_x;
+    gint par_y;
+}GstPlayerThumbnail_info;

Should be GstPlayerThumbnailInfo. But it's more a configuration than an info,
right? Why did you consider adding a struct instead of making it parameters?

Also the struct needs padding

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.