GNOME Bugzilla – Bug 566605
Support the new libav metadata API
Last modified: 2015-08-16 13:40:31 UTC
It'd be nice if we support ffmpeg's metadata API that was added recently: http://svn.ffmpeg.org/ffmpeg/trunk/libavcodec/avcodec.h?r1=16424&r2=16423&pathrev=16424 AVCodecContext and AVFormatContext now have a metadata field.
Old metadata API has been removed, we need to switch before the upcoming release.
Bumping to next release
Created attachment 308453 [details] [review] Map ffmpeg metadata to GStreamer tags Update to the metadata API ffmpeg has had in place for a long time now, and reenable output of GStreamer tags from the demuxer.
Review of attachment 308453 [details] [review]: ::: ext/libav/gstavutils.c @@ +494,3 @@ + +static gchar * +my_safe_copy (gchar * input) Hmm, using my_ is uncommon in gstreamer ;-P @@ +524,3 @@ + g_hash_table_insert (tmp, (void *) "comment", (void *) GST_TAG_COMMENT); + g_hash_table_insert (tmp, (void *) "composer", (void *) GST_TAG_COMPOSER); + g_hash_table_insert (tmp, (void *) "copyright", (void *) GST_TAG_COPYRIGHT); There is usually no reason in C to cast to void *. @@ +526,3 @@ + g_hash_table_insert (tmp, (void *) "copyright", (void *) GST_TAG_COPYRIGHT); + //g_hash_table_insert (tmp, "creation_time", GST_TAG_DATE_TIME); /* Need to convert ISO 8601 to GstDateTime */ + //g_hash_table_insert (tmp, "date", GST_TAG_DATE); /* ditto */ C++ style comments. And why is there something commented out ? ::: ext/libav/gstavutils.h @@ +96,3 @@ +GstTagList * +gst_ffmpeg_metadata_to_tag_list (AVDictionary * metadata); Should on same line, but I see you are following already in place style.
(In reply to Nicolas Dufresne (stormer) from comment #4) > Review of attachment 308453 [details] [review] [review]: Thanks for the review :) > ::: ext/libav/gstavutils.c > @@ +494,3 @@ > + > +static gchar * > +my_safe_copy (gchar * input) > > Hmm, using my_ is uncommon in gstreamer ;-P Yeah - I just moved the existing function to the new file without renaming. Turns out it should probably all just stay in gstavdemux.c > @@ +524,3 @@ > + g_hash_table_insert (tmp, (void *) "comment", (void *) GST_TAG_COMMENT); > + g_hash_table_insert (tmp, (void *) "composer", (void *) > GST_TAG_COMPOSER); > + g_hash_table_insert (tmp, (void *) "copyright", (void *) > GST_TAG_COPYRIGHT); > > There is usually no reason in C to cast to void *. It's a const thing - g_hash_table takes non-const pointers. > @@ +526,3 @@ > + g_hash_table_insert (tmp, (void *) "copyright", (void *) > GST_TAG_COPYRIGHT); > + //g_hash_table_insert (tmp, "creation_time", GST_TAG_DATE_TIME); /* > Need to convert ISO 8601 to GstDateTime */ > + //g_hash_table_insert (tmp, "date", GST_TAG_DATE); /* ditto */ > > C++ style comments. And why is there something commented out ? I cut and pasted the list of all standard tag names from the ffmpeg header, but ran out of time to implement them all. I wanted to put the patch up so it doesn't get lost. I'll add a FIXME. > > ::: ext/libav/gstavutils.h > @@ +96,3 @@ > > +GstTagList * > +gst_ffmpeg_metadata_to_tag_list (AVDictionary * metadata); > > Should on same line, but I see you are following already in place style. Ja.
Created attachment 308474 [details] [review] Map ffmpeg metadata to GStreamer tags Update to the metadata API ffmpeg has had in place for a long time now, and reenable output of GStreamer tags from the demuxer.
Review of attachment 308474 [details] [review]: Looks good. Arguably a feature, but I think it can. Ask Tim if unsure.
Attachment 308474 [details] pushed as 51896af - Map ffmpeg metadata to GStreamer tags