GNOME Bugzilla – Bug 773570
gstplayer: add media info get track num API
Last modified: 2016-12-21 09:34:39 UTC
Add media info get audio/video/subtitle total track number API
Created attachment 338580 [details] [review] patch for add media info get track num API
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()?
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?
Created attachment 342253 [details] [review] Updated patch for get_number_of_streams
Created attachment 342254 [details] [review] Updated patch for get_number_of_streams()
(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 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
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
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
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
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.
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