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 765302 - player: Add support for multiview settings
player: Add support for multiview settings
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.8.0
Other Linux
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-20 10:27 UTC by barunsingh
Modified: 2016-10-31 14:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for MVC support on gstplayer element. (2.96 KB, patch)
2016-04-20 10:27 UTC, barunsingh
needs-work Details | Review
Adding getter/setter of mvc settings in gstplayer. Incorporating review comments. (9.27 KB, patch)
2016-04-21 06:19 UTC, barunsingh
reviewed Details | Review
Adding getter/setter for mvc settings. (6.67 KB, patch)
2016-04-21 07:40 UTC, barunsingh
needs-work Details | Review
Adding getter/setter for mvc settings. (6.68 KB, patch)
2016-04-21 10:06 UTC, barunsingh
needs-work Details | Review
Adding getter/setter for mvc settings. (8.66 KB, patch)
2016-04-21 11:46 UTC, barunsingh
committed Details | Review

Description barunsingh 2016-04-20 10:27:11 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.
Comment 1 Sebastian Dröge (slomo) 2016-04-20 10:37:02 UTC
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
Comment 2 barunsingh 2016-04-20 14:22:10 UTC
(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
Comment 3 Sebastian Dröge (slomo) 2016-04-20 14:26:46 UTC
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
Comment 4 barunsingh 2016-04-21 06:19:25 UTC
Created attachment 326469 [details] [review]
Adding getter/setter of mvc settings in gstplayer. Incorporating review comments.
Comment 5 barunsingh 2016-04-21 06:23:22 UTC
Please wait. I uploaded wrong patch. Uploading right one. Sorry. :(
Comment 6 Sebastian Dröge (slomo) 2016-04-21 06:32:06 UTC
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 :)
Comment 7 barunsingh 2016-04-21 07:40:28 UTC
Created attachment 326473 [details] [review]
Adding getter/setter for mvc settings.
Comment 8 barunsingh 2016-04-21 07:41:06 UTC
Added right PATCH file.
Comment 9 Sebastian Dröge (slomo) 2016-04-21 08:25:38 UTC
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
Comment 10 barunsingh 2016-04-21 10:06:52 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2016-04-21 10:30:48 UTC
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 */
Comment 12 barunsingh 2016-04-21 11:46:47 UTC
Created attachment 326482 [details] [review]
Adding getter/setter for mvc settings.

I hope patch is complete now. :)
Comment 13 Sebastian Dröge (slomo) 2016-04-21 11:54:02 UTC
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 :)
Comment 14 Sebastian Dröge (slomo) 2016-04-21 11:58:35 UTC
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