GNOME Bugzilla – Bug 728845
gst-play: add option to supply input media-files from a playlist file
Last modified: 2014-04-28 14:06:20 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.
Created attachment 275021 [details] Test cases Test cases attached.
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()?
Created attachment 275349 [details] [review] New patch incorporating review comments.
Thanks for the feedback. New patch attached. Please review.
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