After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 773709 - player: Add get video snapshot API
player: Add get video snapshot API
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.11.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-10-31 03:13 UTC by Lyon
Modified: 2017-01-17 11:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for add get video thumbnail API (3.11 KB, patch)
2016-10-31 03:13 UTC, Lyon
none Details | Review
Update the get thumbnail patch according to previous review (4.08 KB, patch)
2016-11-03 06:18 UTC, Lyon
needs-work Details | Review
Updated patch (4.08 KB, patch)
2016-11-04 05:57 UTC, Lyon
reviewed Details | Review
patch for get video sanpshot / thumbnail (5.27 KB, patch)
2017-01-10 08:56 UTC, Lyon
needs-work Details | Review
patch for get video sanpshot / thumbnail (updated by add config parameter) (5.29 KB, patch)
2017-01-11 10:11 UTC, Lyon
needs-work Details | Review
patch for add snapshot API (4.02 KB, patch)
2017-01-12 09:36 UTC, Lyon
none Details | Review
Updated patch for add snapshot API (4.34 KB, patch)
2017-01-13 03:36 UTC, Lyon
none Details | Review
Updated patch for add snapshot API (4.64 KB, patch)
2017-01-16 05:56 UTC, Lyon
none Details | Review
Updated patch for add snapshot API (4.65 KB, patch)
2017-01-17 02:38 UTC, Lyon
none Details | Review
Updated patch for add snapshot API (4.64 KB, patch)
2017-01-17 11:04 UTC, Lyon
committed Details | Review

Description Lyon 2016-10-31 03:13:19 UTC
Add API to get video thumbnail, return GstSample of current video position
Comment 1 Lyon 2016-10-31 03:13:57 UTC
Created attachment 338806 [details] [review]
patch for add get video thumbnail API
Comment 2 Sebastian Dröge (slomo) 2016-10-31 12:20:11 UTC
Review of attachment 338806 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +4208,3 @@
+ *
+ * Get the video sample thumbnail if video existed
+ # the sample includes data in current video and postion

Returns: comes after the description about what the function does, and after Returns: you should put a "Since: 1.12" here

@@ +4230,3 @@
+      "format", G_TYPE_STRING, "RGB",
+      "width", G_TYPE_INT, 720,
+      "pixel-aspect-ratio", GST_TYPE_FRACTION, 1, 1, NULL);

Why these specific settings? It would probably be better to have an enum here for: PNG, JPG, raw RGB, raw native. Where raw native is just what comes out of the decoder.

And width/height parameters which can be -1 for the default

@@ +4250,3 @@
+  gst_structure_get_int (st, "height", &height);
+  if (width <= 0 || height <= 0) {
+    GST_WARNING_OBJECT (self, "height and width of image are less than 0");

This can't really happen, can it?

::: gst-libs/gst/player/gstplayer.h
@@ +203,3 @@
 guint          gst_player_config_get_position_update_interval  (const GstStructure * config);
 
+/* get video thumnail at current position */

No "documentation" in the headers :)
Comment 3 Lyon 2016-11-03 06:18:58 UTC
Created attachment 339007 [details] [review]
Update the get thumbnail patch according to previous review

Updated the patch according to the previous review
Comment 4 Sebastian Dröge (slomo) 2016-11-03 11:00:23 UTC
Review of attachment 339007 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +4295,3 @@
+
+  width = gst_player_video_info_get_width (video_info);
+  height = gst_player_video_info_get_height (video_info);

Why? I think it would make more sense to add optional width/height parameters, and otherwise leave them out of the caps below.

@@ +4300,3 @@
+    case GST_PLAYER_THUMBNAIL_RAW_RGB:
+      caps = gst_caps_new_simple ("video/x-raw",
+          "format", G_TYPE_STRING, "RGB",

Why RGB, that's usually very difficult to handle. Why not xRGB on big endian and BGRx on little endian (that's what UI toolkits usually like)
Comment 5 Lyon 2016-11-04 05:57:47 UTC
Created attachment 339098 [details] [review]
Updated patch

patch updated:
- Add parameter width and height
- remove enum RGB and add xRGB and BGRx
- if width and height is valid, then set width/height caps to convert
Comment 6 Sebastian Dröge (slomo) 2016-11-04 15:25:27 UTC
Review of attachment 339098 [details] [review]:

Generally not too happy with the API yet, but I also don't have better suggestions

::: gst-libs/gst/player/gstplayer.c
@@ +4302,3 @@
+      caps = gst_caps_new_simple ("video/x-raw",
+          "format", G_TYPE_STRING, "BGRx",
+          "pixel-aspect-ratio", GST_TYPE_FRACTION, 1, 1, NULL);

Usually it makes most sense to generate xRGB on big endian and BGRx on little endian

@@ +4315,3 @@
+    default:
+      caps = gst_caps_new_simple ("video/x-raw",
+          "pixel-aspect-ratio", GST_TYPE_FRACTION, 1, 1, NULL);

Maybe the PAR should also be an (optional) parameter. For native it might make sense to not rescale

::: gst-libs/gst/player/gstplayer.h
@@ +210,3 @@
+  GST_PLAYER_THUMBNAIL_RAW_NATIVE = 0,
+  GST_PLAYER_THUMBNAIL_RAW_XRGB,
+  GST_PLAYER_THUMBNAIL_RAW_BGRX,

Capitalization
Comment 7 Lyon 2017-01-10 08:55:51 UTC
Hi, Slomo, 
  For the previous patch, actually I think it should be called get_video_sanpshot, because it just get the current position video sample. 

Considering there might be user case that application need get the thumbnail of the video (while there is no cover-image tag in the stream), also user may need get video sample at certain position as video thumbnail.

I refined previous patch:
-  Add struct GstPlayerThumbnail_info to set image type/width/height/pixel-aspect-ratio
-  Add gst_player_get_video_snapshot()  API to get current video snapshot at current position
-  Add gst_player_get_video_thumbnail() API to get video thumbnail at given position.

Could you please help review the new patch.

Thanks
Lyon
Comment 8 Lyon 2017-01-10 08:56:50 UTC
Created attachment 343219 [details] [review]
patch for get video sanpshot / thumbnail
Comment 9 Sebastian Dröge (slomo) 2017-01-10 15:19:26 UTC
Review of attachment 343219 [details] [review]:

::: 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
Comment 10 Lyon 2017-01-10 15:40:13 UTC
The original thought of adding this thumbnail API is coming from get the thumbnail for video preview before open it to play (and add the seek position is because usually at the beginning of the film, there will be all black frames for several seconds ).

It is kind like media info for the video stream, but then I find it is not very proper to add this API in the media-info interface, as it is not the information got from media information data.
So I add this API at gstplayer API 


The reason I use structure is that for setting type/width/height/par_n/par_d, there seems too many parameter for the function.
Comment 11 Sebastian Dröge (slomo) 2017-01-10 15:44:02 UTC
(In reply to Lyon from comment #10)
> The original thought of adding this thumbnail API is coming from get the
> thumbnail for video preview before open it to play (and add the seek
> position is because usually at the beginning of the film, there will be all
> black frames for several seconds ).
> 
> It is kind like media info for the video stream, but then I find it is not
> very proper to add this API in the media-info interface, as it is not the
> information got from media information data.
> So I add this API at gstplayer API 

I'd like to have this integrated into GstDiscoverer somehow, or probably rather as a separate thumbnailing API.

> The reason I use structure is that for setting
> type/width/height/par_n/par_d, there seems too many parameter for the
> function.

We could make it two probably: the enum and an optional GstStructure for "additional configuration"
Comment 12 Lyon 2017-01-11 10:11:19 UTC
Created attachment 343290 [details] [review]
patch for get video sanpshot / thumbnail (updated by add config parameter)

Update the patch change parameter as type and configuration
Comment 13 Lyon 2017-01-12 09:29:06 UTC
Hi slomo,
     If the get thumbnail is better implement application-side, could we just upstream the patch for get_snapshot()?

Thanks
Lyon
Comment 14 Lyon 2017-01-12 09:36:01 UTC
Created attachment 343349 [details] [review]
patch for add snapshot API
Comment 15 Sebastian Dröge (slomo) 2017-01-12 09:36:15 UTC
Yes, that's my preferred solution here. And then we can consider adding a thumbnailing API to GstDiscoverer or standalone.
Comment 16 Sebastian Dröge (slomo) 2017-01-12 10:37:45 UTC
Review of attachment 343349 [details] [review]:

Typo in commit message: "sanpshot"

Please also include a link to this bug report, and the name of the new function at least

::: gst-libs/gst/player/gstplayer.c
@@ +4264,3 @@
+ * gst_player_get_video_snapshot:
+ * @player: #GstPlayer instance
+ * @type: prefered output format

This is not just preferred, it *is* the output format. Also maybe call it format, not type everywhere

@@ +4265,3 @@
+ * @player: #GstPlayer instance
+ * @type: prefered output format
+ * @config: Additional configuration including width/height/par-n/par-d

Just additional configuration. Also: "@config: (allow-none): ..."

@@ +4269,3 @@
+ * Get the video sample snapshot if video existed
+ # the sample includes data in current video and postion
+ # the prefered output format / width / height / pixel-aspect-ratio can be set

How about:

Get a snapshot of the currently selected video stream, if any. The format can be selected with @type and optional additional configuration is possible with @config. Currently supported settings are:
- width, height of type G_TYPE_INT
- pixel-aspect-ratio of type GST_TYPE_FRACTION

@@ +4271,3 @@
+ # the prefered output format / width / height / pixel-aspect-ratio can be set
+ *
+ * Returns: Current video snapshot sample

Returns: (transfer full): ... or %NULL on failure

@@ +4321,3 @@
+    if (!gst_structure_get_int(config, "par-n", &par_n))
+      par_n = 0;
+    if (!gst_structure_get_int(config, "par-d", &par_d))

Should be "pixel-aspect-ratio" of type GST_TYPE_FRACTION

@@ +4323,3 @@
+    if (!gst_structure_get_int(config, "par-d", &par_d))
+      par_d = 0;
+  }

If no config, except for native I would set pixel-aspect-ratio to 1/1 (and mention that in the documentation).

::: gst-libs/gst/player/gstplayer.h
@@ +216,3 @@
+
+GstSample * gst_player_get_video_snapshot (GstPlayer * player,
+    GstPlayerThumbnailType type, GstStructure * config);

The structure could be const I guess
Comment 17 Lyon 2017-01-13 03:36:07 UTC
Created attachment 343400 [details] [review]
Updated patch for add snapshot API

Hi, Slomo,
    Thanks for your review comments. please see attached file for the new patch.
Comment 18 Sebastian Dröge (slomo) 2017-01-13 10:34:11 UTC
Review of attachment 343400 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +4265,3 @@
+ * @player: #GstPlayer instance
+ * @format: output format of the video snapshot
+ * @config: (allow-none) Additional configuration

(allow-none) should be followed by a colon (:). Also actually nowadays this should be (nullable) instead

@@ +4272,3 @@
+ * - width, height of type G_TYPE_INT
+ * - pixel-aspect-ratio of type GST_TYPE_FRACTION
+ *   If no config is set, pixel-aspect-ratio would be 1/1 excpet RAW_NATIVE format

Typo: excpet. Also write the whole enum value instead of just RAW_NATIVE

@@ +4334,3 @@
+  }
+
+  if (format != GST_PLAYER_THUMBNAIL_RAW_NATIVE) {

If given in the config, I would also override it for RAW_NATIVE. Just if it's *not* given the default should be used for RAW_NATIVE instead of 1/1
Comment 19 Lyon 2017-01-16 05:56:11 UTC
Created attachment 343526 [details] [review]
Updated patch for add snapshot API

Updated patch for gst_video_snapshot()
Comment 20 Sebastian Dröge (slomo) 2017-01-16 10:25:28 UTC
Review of attachment 343526 [details] [review]:

Looks mostly good, just minor things now

::: gst-libs/gst/player/gstplayer.c
@@ +4267,3 @@
+ * @config: (allow-none) Additional configuration
+ *
+ * Get a sanpshot of the currently slected video stream, if any. The format can be

Typo: sanpshot -> snapshot, slected -> selected

@@ +4272,3 @@
+ * - width, height of type G_TYPE_INT
+ * - pixel-aspect-ratio of type GST_TYPE_FRACTION
+ *   If no config is set, pixel-aspect-ratio would be 1/1 except GST_PLAYER_THUMBNAIL_RAW_NATIVE format

except for GST_PLAYER... format in which case it is the original pixel-aspect-ratio

@@ +4275,3 @@
+ *
+ * Returns: (transfer full):  Current video snapshot sample or %NULL on failure
+ * Since 1.12

Needs an empty line before the Since:

@@ +4313,3 @@
+    case GST_PLAYER_THUMBNAIL_RAW_NATIVE:
+    default:
+      caps = gst_caps_new_simple ("video/x-raw", NULL, NULL);

Use gst_caps_new_empty_simple() if no fields are to be set

::: gst-libs/gst/player/gstplayer.h
@@ +213,3 @@
+  GST_PLAYER_THUMBNAIL_JPG,
+  GST_PLAYER_THUMBNAIL_PNG
+} GstPlayerThumbnailType;

GstPlayerThumbnailFormat maybe instead?
Comment 21 Lyon 2017-01-17 02:38:58 UTC
Created attachment 343605 [details] [review]
Updated patch for add snapshot API

Hi, Slomo,
   Updated the patch:
- Fix typo
- Modify GstPlayerThumbnailType -> GstPlayerThumbnailFormat
- gst_caps_new_simple() -> gst_caps_new_empty_simple()

Thanks
Lyon
Comment 22 Sebastian Dröge (slomo) 2017-01-17 10:58:48 UTC
Review of attachment 343605 [details] [review]:

Just one final thing, otherwise good to go :) Thanks!

::: gst-libs/gst/player/gstplayer.c
@@ +4310,3 @@
+      break;
+    case GST_PLAYER_THUMBNAIL_PNG:
+      caps = gst_caps_new_simple ("image/png", NULL, NULL);

new_empty_simple() for JPG and PNG too please
Comment 23 Lyon 2017-01-17 11:04:18 UTC
Created attachment 343634 [details] [review]
Updated patch for add snapshot API

Updated patch:
- JPG and PNG also use gst_caps_new_simple ()
Comment 24 Sebastian Dröge (slomo) 2017-01-17 11:10:12 UTC
commit 2609aaa48b163a35ca01013c4894c625f18a4045
Author: Lyon Wang <lyon.wang@nxp.com>
Date:   Tue Jan 10 16:38:21 2017 +0800

    player: Add get video snapshot API
    
    Add get video snapshot API:
      gst_player_get_video_snapshot()
    
    https://bugzilla.gnome.org/show_bug.cgi?id=773709