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 773521 - player: add set accurate seek config API
player: add set accurate seek config API
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-10-26 08:49 UTC by Lyon
Modified: 2016-11-01 17:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for add accurate seek (3.86 KB, patch)
2016-10-26 08:50 UTC, Lyon
none Details | Review
modified patch for add accurate seek (3.89 KB, patch)
2016-10-26 10:24 UTC, Lyon
accepted-commit_after_freeze Details | Review
Update version according to slomo's comments (3.89 KB, patch)
2016-10-27 02:35 UTC, Lyon
none Details | Review
Update caccording to slomo's commnets (4.13 KB, patch)
2016-10-27 02:51 UTC, Lyon
committed Details | Review

Description Lyon 2016-10-26 08:49:43 UTC
Add set accurate seek config API
so that can switch between accurate and no-accurate seek
Comment 1 Lyon 2016-10-26 08:50:27 UTC
Created attachment 338485 [details] [review]
patch for add accurate seek

The reason why the set_seek_accurate API use GstPlayer *player as parameter instead of * config, is that if using config, we have get_config() first and then set_config() back, however for set_config(), only in STOPPED status it can set successfully, so if we want to seek when playing (either accurate or no-accurate), we have to call gst_player_stop() first, then set_config(). but it takes time to change state to STOPPED which might cause set_config failed. 

So here use GstPlayer *player as input parameter, so that we can set accurate flag in player->config directly without have to stop the pipeline.

Please kindly find the attached file for the patch.

Thanks
Lyon
Comment 2 Sebastian Dröge (slomo) 2016-10-26 09:18:57 UTC
Review of attachment 338485 [details] [review]:

Thanks, looks good except for cosmetic things and docs

::: gst-libs/gst/player/gstplayer.c
@@ +4219,3 @@
+ *
+ * set accurate seek flag.
+ */

Since: 1.12 (also for the other function)


Please also explain in more detail in the documentation what this actually means

@@ +4235,3 @@
+ * @config: a #GstPlayer configuration
+ *
+ * Returns: current position update interval in milliseconds

Copy and paste error from other documentation

@@ +4242,3 @@
+gst_player_config_get_seek_accurate(const GstStructure * config)
+{
+  gboolean  accurate = FALSE;

Double space before the variable name here. please run the .c files through gst-indent

::: gst-libs/gst/player/gstplayer.h
@@ +205,3 @@
+void           gst_player_config_set_seek_accurate  (GstPlayer *player,
+                                                                gboolean          accurate);
+gboolean       gst_player_config_get_seek_accurate  (const GstStructure * config);

Fix up indentation please
Comment 3 Lyon 2016-10-26 10:24:35 UTC
Created attachment 338494 [details] [review]
modified patch for add accurate seek

Modified patch according to the review comments
Comment 4 Sebastian Dröge (slomo) 2016-10-26 12:11:57 UTC
Review of attachment 338494 [details] [review]:

Please run your .c files through gst-indent


(If you can, update the patch, otherwise I'll fix up these things when merging after 1.10.0)

::: gst-libs/gst/player/gstplayer.c
@@ +4219,3 @@
+ *
+ * set accurate seek flag.
+ * If set, seek process will get to accurate seek position

Basically what this should say is something like:

Enable/disable accurate seeking. When enabled, if possible, elements will try harder to seek as accurately as possible to the requested seek position and generally seeking can be slower. Without this flag, they will seek as close to the requested seek position as is possible without slowing down seeking too much.



Maybe also taking a look at the wording of the GST_SEEK_FLAG_ACCURATE docs could help here.

::: gst-libs/gst/player/gstplayer.h
@@ +205,3 @@
+void           gst_player_config_set_seek_accurate  (GstPlayer *player,
+                                                     gboolean  accurate);
+gboolean       gst_player_config_get_seek_accurate  (const GstStructure * config);

And fix the indentation here to be aligned with the functions above :)
Comment 5 Lyon 2016-10-27 02:35:36 UTC
Created attachment 338570 [details] [review]
Update version according to slomo's comments

slomo, thanks so much for your kindly review: 
patch updated:
- gst-indent the gstplayer.c
- Updated documentation for set_seek_accurate API
- fix indentation in gstplayer.h
Comment 6 Lyon 2016-10-27 02:51:07 UTC
Created attachment 338571 [details] [review]
Update caccording to slomo's commnets

Sorry, previous patch is uploaded wrongly.
Please ignore previous one and use this one.
Comment 7 Sebastian Dröge (slomo) 2016-11-01 17:52:09 UTC
commit 8d41d816a240b0a4739e99640a382d0beab670e0
Author: Lyon Wang <lyon.wang@nxp.com>
Date:   Wed Oct 26 16:28:10 2016 +0800

    player: Add configuration for enabling accurate seeks
    
    https://bugzilla.gnome.org/show_bug.cgi?id=773521