GNOME Bugzilla – Bug 765302
player: Add support for multiview settings
Last modified: 2016-10-31 14:19:27 UTC
Created attachment 326397 [details] [review] Patch for MVC support on gstplayer element. Dear All Current implementation of gstplayer element is missing MVC support. so Adding it.
Review of attachment 326397 [details] [review]: Thanks for your patch! It would be good to additionally have setter/getter functions for this too ::: gst-libs/gst/player/gstplayer.c @@ +591,3 @@ + case PROP_VIDEO_MULTIVIEW_MODE: + g_object_set (G_OBJECT (self->playbin), "video-multiview-mode", + g_value_get_enum (value), NULL); You could use g_object_set_value() here to prevent the GValue round-trip @@ +692,3 @@ + GstVideoMultiviewFramePacking v; + g_object_get (G_OBJECT (self->playbin), "video-multiview-mode", &v, NULL); + g_value_set_enum (value, v); You could use g_object_get_value() here to prevent the GValue round-trip
(In reply to Sebastian Dröge (slomo) from comment #1) > Review of attachment 326397 [details] [review] [review]: > > Thanks for your patch! It would be good to additionally have setter/getter > functions for this too > > ::: gst-libs/gst/player/gstplayer.c > @@ +591,3 @@ > + case PROP_VIDEO_MULTIVIEW_MODE: > + g_object_set (G_OBJECT (self->playbin), "video-multiview-mode", > + g_value_get_enum (value), NULL); > > You could use g_object_set_value() here to prevent the GValue round-trip > > @@ +692,3 @@ > + GstVideoMultiviewFramePacking v; > + g_object_get (G_OBJECT (self->playbin), "video-multiview-mode", &v, > NULL); > + g_value_set_enum (value, v); > > You could use g_object_get_value() here to prevent the GValue round-trip Mr. Sebastian, I will do the changes and upload the patch. Also, I am working on to find other list of features/api that we can add in gstplayer element. Tomorrow I will start a new discussion for this. Thanks Barun Kr. Singh
I created various enhancement bug reports about GstPlayer some time earlier today: https://bugzilla.gnome.org/buglist.cgi?quicksearch=summary%3A%22player%22%20product%3A%22GStreamer%22%20component%3A%22gst-plugins-bad%22&list_id=117445
Created attachment 326469 [details] [review] Adding getter/setter of mvc settings in gstplayer. Incorporating review comments.
Please wait. I uploaded wrong patch. Uploading right one. Sorry. :(
Review of attachment 326469 [details] [review]: ::: gst-libs/gst/player/gstplayer.c @@ +3725,3 @@ + * (6): row-interleaved - GST_VIDEO_MULTIVIEW_FRAME_PACKING_ROW_INTERLEAVED + * (7): top-bottom - GST_VIDEO_MULTIVIEW_FRAME_PACKING_TOP_BOTTOM + * (8): checkerboard - GST_VIDEO_MULTIVIEW_FRAME_PACKING_CHECKERBOARD You don't have to include all the possible values here btw, it will get out of date and people can just click on the enum type in the docs to get to it anyway @@ +3730,3 @@ +gst_player_get_multiview_mode (GstPlayer * self) +{ + /* Initialize mode with default value */ And this comment is not really needed :)
Created attachment 326473 [details] [review] Adding getter/setter for mvc settings.
Added right PATCH file.
Review of attachment 326473 [details] [review]: Thanks, just some minor things and cosmetic changes below :) ::: gst-libs/gst/player/gstplayer.c @@ +3714,3 @@ + * + * Returns: The current value of @type, Default: -1 "none". In case of + * error default value is returned. Add a "Since: 1.10" to all the new functions @@ +3727,3 @@ + + return val; + Please remove the empty lines at the end of all functions, and at the beginning @@ +3745,3 @@ + g_return_if_fail (GST_IS_PLAYER (self)); + + g_object_set (self, "video-multiview-mode", &mode, NULL); Without the & @@ +3786,3 @@ + g_return_if_fail (GST_IS_PLAYER (self)); + + g_object_set (self, "video-multiview-flags", &flags, NULL); Without the & ::: gst-libs/gst/player/gstplayer.h @@ +24,2 @@ #include <gst/gst.h> +#include <gst/video/video-info.h> Include gst/video/video.h here, and it also will have to be added to the pkg-config file then
Created attachment 326476 [details] [review] Adding getter/setter for mvc settings. All comments incorporated except updating pkg-config. couldn't find where to update. please let me know.
Review of attachment 326476 [details] [review]: Check gst-plugins-bad/pkgconfig/gstreamer-player*.pc.in. You would add it to Requires You also need to add the new functions to docs/libs/*sections.txt in the right place ::: gst-libs/gst/player/gstplayer.c @@ +3710,3 @@ + * @player: #GstPlayer instance + * @type: #GstVideoMultiviewMode + * @Since 1.10 Without the @. Just "Since: 1.10" and as the very last line of the documentation after the Return: and before the */
Created attachment 326482 [details] [review] Adding getter/setter for mvc settings. I hope patch is complete now. :)
Review of attachment 326482 [details] [review]: ::: gst-libs/gst/player/gstplayer.c @@ +3714,3 @@ + * + * Returns: The current value of @type, Default: -1 "none". In case of + * error default value is returned. There is no valid error case here, I'll remove that part when merging @@ +3716,3 @@ + * error default value is returned. + * + * Since 1.10 A : is missing here, but I'll add those when merging :)
commit aa754f634c9060f08da3ef3afbc4da4ce3a04d75 Author: Barun Kumar Singh <barun.singh@samsung.com> Date: Tue Apr 19 10:59:46 2016 +0530 player: Add support for multiview settings https://bugzilla.gnome.org/show_bug.cgi?id=765302