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 773570 - gstplayer: add media info get track num API
gstplayer: add media info get track num 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-27 06:38 UTC by Lyon
Modified: 2016-12-21 09:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for add media info get track num API (4.62 KB, patch)
2016-10-27 06:39 UTC, Lyon
needs-work Details | Review
Updated patch for get_number_of_streams (4.06 KB, patch)
2016-12-20 10:25 UTC, Lyon
none Details | Review
Updated patch for get_number_of_streams() (3.34 KB, patch)
2016-12-20 10:29 UTC, Lyon
none Details | Review
Updated patch for get_number_of_streams(), add doc / win32 symbol (4.33 KB, patch)
2016-12-21 03:38 UTC, Lyon
none Details | Review
Updated patch for get_number_of_streams(), add doc / win32 symbol (4.63 KB, patch)
2016-12-21 09:17 UTC, Lyon
committed Details | Review

Description Lyon 2016-10-27 06:38:41 UTC
Add media info get audio/video/subtitle total track number API
Comment 1 Lyon 2016-10-27 06:39:15 UTC
Created attachment 338580 [details] [review]
patch for add media info get track num API
Comment 2 Sebastian Dröge (slomo) 2016-10-31 11:59:32 UTC
Review of attachment 338580 [details] [review]:

Don't mark files as executable in your patch

::: gst-libs/gst/player/gstplayer-media-info-private.h
@@ +109,1 @@
   GList *subtitle_stream_list;

Instead of this, please use a GQueue here which automatically keeps track of the length

::: gst-libs/gst/player/gstplayer-media-info.h
@@ +192,3 @@
+guint  gst_player_get_audio_num  (const GstPlayerMediaInfo *info);
+guint  gst_player_get_video_num  (const GstPlayerMediaInfo *info);
+guint  gst_player_get_subtitle_num  (const GstPlayerMediaInfo *info);

num is not a word, and this is also grammatically weird: is it returning the number of the audio?
Maybe get_number_of_audio_streams()?
Comment 3 Lyon 2016-12-20 10:23:03 UTC
Hi, slomo,
If we replace current Glist with GQueue, we may have to change the media-info API, it returns Glist* in some API . besides, although GQueue keeps track of the length itself, it still need g_queue_get_length() to get the length. how about in the get_number_of_streams(), we just return g_list_length(steram_list), it will make it more easier to realize than use GQueue()

Could you please see the attached updated patch?
Comment 4 Lyon 2016-12-20 10:25:07 UTC
Created attachment 342253 [details] [review]
Updated patch for get_number_of_streams
Comment 5 Lyon 2016-12-20 10:29:17 UTC
Created attachment 342254 [details] [review]
Updated patch for get_number_of_streams()
Comment 6 Sebastian Dröge (slomo) 2016-12-20 10:58:57 UTC
(In reply to Lyon from comment #3)
> Hi, slomo,
> If we replace current Glist with GQueue, we may have to change the
> media-info API, it returns Glist* in some API .

GQueue contains a GList that could be used here probably :)

> besides, although GQueue
> keeps track of the length itself, it still need g_queue_get_length() to get
> the length.

g_queue_get_length() is O(1), g_list_length() is O(n).
Comment 7 Sebastian Dröge (slomo) 2016-12-20 11:03:19 UTC
Comment on attachment 342254 [details] [review]
Updated patch for get_number_of_streams()

Seems ok, we're not going to have *that* many streams that it matters. And if needed we can change this later
Comment 8 Sebastian Dröge (slomo) 2016-12-20 11:08:08 UTC
Review of attachment 342254 [details] [review]:

Actually not. Please don't change the .c and .h files to executable.

Also please add the new symbols to docs/libs/*sections.txt in the right place, and win32/common/libgstplayer.def
Comment 9 Lyon 2016-12-21 03:38:13 UTC
Created attachment 342293 [details] [review]
Updated patch for get_number_of_streams(), add doc / win32 symbol

Updated the patch:
- Remove the file mode change in the patch
- Update gst-plugins-bad-libs-sections.txt and libgstplayer.def

Thanks
Lyon
Comment 10 Sebastian Dröge (slomo) 2016-12-21 08:09:08 UTC
Review of attachment 342293 [details] [review]:

::: docs/libs/gst-plugins-bad-libs-sections.txt
@@ +1907,3 @@
+gst_player_get_number_of_video_streams
+gst_player_get_number_of_audio_streams
+gst_player_get_number_of_subtitle_streams

These should be at the corresponding places next to related functions above

::: gst-libs/gst/player/gstplayer-media-info.h
@@ +197,3 @@
+                (const GstPlayerMediaInfo *info);
+guint         gst_player_get_number_of_subtitle_streams
+                (const GstPlayerMediaInfo *info);

These should all be gst_player_*media_info*_get_...()

::: win32/common/libgstplayer.def
@@ +109,3 @@
+	gst_player_get_number_of_video_streams
+	gst_player_get_number_of_audio_streams
+	gst_player_get_number_of_subtitle_streams

These should be in alphabetic order. Run "make check-exports" to see the problem
Comment 11 Lyon 2016-12-21 09:17:22 UTC
Created attachment 342312 [details] [review]
Updated patch for get_number_of_streams(), add doc / win32 symbol

Updated:
- change function name: gst_player_get_xxx() to gst_player_media_info_get_xxx()
- Update doc / win32 def symbol position.
Comment 12 Sebastian Dröge (slomo) 2016-12-21 09:34:27 UTC
commit 00f9a21cd2c356900398af979897f141739ca557
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Dec 21 11:32:52 2016 +0200

    player: Move audio/video/subtitle stream list getters into the correct namespace

commit 9294dc4ac79e6a68dd05f7e5f66d947f6c1ff9c2
Author: Lyon Wang <lyon.wang@nxp.com>
Date:   Tue Dec 20 18:20:02 2016 +0800

    player: Add get track number media info API
    
    https://bugzilla.gnome.org/show_bug.cgi?id=773570