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 728845 - gst-play: add option to supply input media-files from a playlist file
gst-play: add option to supply input media-files from a playlist file
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-04-24 04:24 UTC by RaviKiran
Modified: 2014-04-28 14:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for proposed changes. (3.14 KB, patch)
2014-04-24 04:24 UTC, RaviKiran
needs-work Details | Review
Test cases (403 bytes, text/plain)
2014-04-24 04:25 UTC, RaviKiran
  Details
New patch incorporating review comments. (3.24 KB, patch)
2014-04-28 13:19 UTC, RaviKiran
committed Details | Review

Description RaviKiran 2014-04-24 04:24:42 UTC
Created attachment 275020 [details] [review]
Patch for proposed changes.

tools/gst-play.c tool accepts input media files/directory from command line.
It will be useful if there is an option to supply input media-files or URLs from a list file. Testers can make custom list of media-files and URLs and supply.
Media files can be fetched from the playlist file (if provided) and command arguments.

Patch attached for the proposed changes. Please review and test my patch. Feedback welcome.
Comment 1 RaviKiran 2014-04-24 04:25:54 UTC
Created attachment 275021 [details]
Test cases

Test cases attached.
Comment 2 Tim-Philipp Müller 2014-04-26 10:02:22 UTC
Comment on attachment 275020 [details] [review]
Patch for proposed changes.

Thanks for the patch. This looks like a good addition.

>+  gchar *list = NULL;

Please rename to playlist_file or playlist_fn or so.

>+    {"playlist", 0, 0, G_OPTION_ARG_STRING, &list,
>+        N_("Playlist file containing input media files"), NULL},

I think this should be a G_OPTION_ARG_FILENAME instead.

>+#define MAX_PATH_LEN 512
>+  FILE *playlistfd = NULL;
>+  gchar line[MAX_PATH_LEN];

We prefer not to hardcode lengths like this if not needed, but rather do a few extra string copies, at least in tools like this, see below.

>+  if (list != NULL) {
>+    playlistfd = fopen (list, "r");
>+
>+    if (playlistfd != NULL) {
>+      while (fgets (line, MAX_PATH_LEN, playlistfd) != NULL) {
>+        if (strlen (line) - 1) {
>+          /* Ignore line feed */
>+          line[strlen (line) - 1] = 0;
>+          add_to_playlist (playlist, line);
>+        }
>+      }
>+      fclose (playlistfd);
>+    }
>+  }

Could you rewrite this bit using g_file_get_contents() + g_strsplit()?
Comment 3 RaviKiran 2014-04-28 13:19:38 UTC
Created attachment 275349 [details] [review]
New patch incorporating review comments.
Comment 4 RaviKiran 2014-04-28 13:20:13 UTC
Thanks for the feedback. New patch attached. Please review.
Comment 5 Tim-Philipp Müller 2014-04-28 14:05:31 UTC
Thanks a lot, pushed this with minor changes (moved some variables into the local block, and shortened them; reuse existing GError variable; use playlist->num directly instead of assigning it to num and then checking num):

 commit 96efc280d5050cc794a33cfcc687e86853942a96
 Author: Ravi Kiran K N <ravi.kiran@samsung.com>
 Date:   Mon Apr 28 18:47:06 2014 +0530

    gst-play: add option to supply media files from playlist file
    
    https://bugzilla.gnome.org/show_bug.cgi?id=728845