GNOME Bugzilla – Bug 769390
wavparse: Add bitrate and container format tags
Last modified: 2016-08-02 12:24:15 UTC
WAV is a container format for audio data. Inform the application that the data comes from an WAV container by adding a CONTAINER_FORMAT tag. Also, add bitrate tags for uncompressed PCM data. This helps downstream elements which use the bitrate to estimate network buffer sizes.
Created attachment 332487 [details] [review] Additional PCM bitrate and container tags
Review of attachment 332487 [details] [review]: ::: gst/wavparse/gstwavparse.c @@ +1228,2 @@ + gst_tag_list_add (wav->tags, GST_TAG_MERGE_REPLACE, + GST_TAG_CONTAINER_FORMAT, "WAV", NULL); Use gst_pb_utils_add_codec_description_to_tag_list() here. That ensures consistent naming @@ +1229,3 @@ + GST_TAG_CONTAINER_FORMAT, "WAV", NULL); + + if (wav->bps) { How is this only for raw audio? And you can also add this for alaw/mulaw and a few other codecs probably.
For non-raw audio, additional parsers/encoders are used downstream. These take care of the bitrate tags. One example is MP3 data inside a WAV file (many older .mp3 files are actually exactly this). In this case, mpegaudioparse is autoplugged downstream. Also, for alaw/mulaw/adpcm, bps is already nonzero (it is calculated in gst-plugins-base/gst-libs/gst/riff/riff-media.c).
Created attachment 332505 [details] [review] Additional PCM bitrate and container tags, v2
Created attachment 332507 [details] [review] Additional PCM bitrate and container tags, v3 Removed maximum, minimum, nominal bitrate tags. bps is the _average_ bitrate, and it might be calculated, so there is no guarantee that it is the minimum/maximum/nominal bitrate. Just stick to GST_TAG_BITRATE.
(In reply to Carlos Rafael Giani from comment #3) > For non-raw audio, additional parsers/encoders are used downstream. These > take care of the bitrate tags. One example is MP3 data inside a WAV file > (many older .mp3 files are actually exactly this). In this case, > mpegaudioparse is autoplugged downstream. > > Also, for alaw/mulaw/adpcm, bps is already nonzero (it is calculated in > gst-plugins-base/gst-libs/gst/riff/riff-media.c). Right, but don't we set bps to non-zero for compressed formats too in some cases? Or not?
bps actually comes from the RIFF WAV header. It is the "ByteRate" field in this spec: http://soundfile.sapp.org/doc/WaveFormat/ so even if it is set for compressed formats, it is still a valid value that should be interpreted as a bitrate. This is also why I didn't mention the word "uncompressed" in the comments. bps is also used this way in the gst_wavparse_calculate_duration() function. (bps is manually set to zero for MPEG audio formats in the wavparse code here: https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/wavparse/gstwavparse.c#n1182 )
It should probably not be used as min,max and avg then but only avg :)
Yes. See comment #5 :)
Review of attachment 332507 [details] [review]: ::: gst/wavparse/gstwavparse.c @@ +1225,3 @@ wav->got_fmt = TRUE; + wav->tags = gst_tag_list_new_empty (); Check if a) anything could already have created ->tags before, and b) if all other code works fine with you creating them here (i.e. nothing unconditionally puts a new taglist here, like line 1227). (Same thing for aiffparse) @@ +1231,3 @@ + gst_pb_utils_add_codec_description_to_tag_list (wav->tags, + GST_TAG_CONTAINER_FORMAT, desccaps); + gst_caps_unref (desccaps); gst_pad_get_pad_template_caps() on the sinkpad, no need to create new caps here.
Created attachment 332544 [details] [review] Additional PCM bitrate and container tags, v4
commit 91e302e00de45ac8ccfc64ea34ab952d2040acf0 Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Tue Aug 2 14:01:14 2016 +0200 wavparse: Add tags for container format and bitrate for uncompressed PCM The PCM bitrate is added to help downstream elements (like uridecodebin) figure out a proper network buffer size https://bugzilla.gnome.org/show_bug.cgi?id=769390