GNOME Bugzilla – Bug 773709
player: Add get video snapshot API
Last modified: 2017-01-17 11:11:33 UTC
Add API to get video thumbnail, return GstSample of current video position
Created attachment 338806 [details] [review] patch for add get video thumbnail API
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 :)
Created attachment 339007 [details] [review] Update the get thumbnail patch according to previous review Updated the patch according to the previous review
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)
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
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
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
Created attachment 343219 [details] [review] patch for get video sanpshot / thumbnail
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
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.
(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"
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
Hi slomo, If the get thumbnail is better implement application-side, could we just upstream the patch for get_snapshot()? Thanks Lyon
Created attachment 343349 [details] [review] patch for add snapshot API
Yes, that's my preferred solution here. And then we can consider adding a thumbnailing API to GstDiscoverer or standalone.
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
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.
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
Created attachment 343526 [details] [review] Updated patch for add snapshot API Updated patch for gst_video_snapshot()
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?
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
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
Created attachment 343634 [details] [review] Updated patch for add snapshot API Updated patch: - JPG and PNG also use gst_caps_new_simple ()
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