GNOME Bugzilla – Bug 720995
matroskamux: add g726 adpcm support
Last modified: 2014-01-19 09:35:15 UTC
Created attachment 264823 [details] [review] adpcm g726 wave format gst-launch-1.0 alsasrc num-buffers=1000 ! avenc_g726 ! matroskamux ! filesink location=/tmp/test.mkv produce a file that vlc can play fine, maybe something is still missing since playback from gstreamer fails with this error: 0:00:00.070559320 9786 0x165fad0 ERROR libav :0:: Invalid number of bits 25
Created attachment 264824 [details] [review] adpcm g726 support in matroskamux
Comment on attachment 264823 [details] [review] adpcm g726 wave format Without the common changes please Is there any other software that supports this ADPCM variant?
Comment on attachment 264824 [details] [review] adpcm g726 support in matroskamux Also without the common changes please
Created attachment 265220 [details] [review] adpcm g726 wave format modified patch
Created attachment 265221 [details] [review] matroskamux: add support for g726 adpcm modified patch, I'm not aware of any gstreamer muxer that support adpcm with g726 layout, with this patch the produced files are playable with mplayer/ffplay/vlc but not with gstreamer, probably something is missing in the matroskademux
you can also use ffmpeg/libav with something like this: ffmpeg -f alsa -ar 8000 -i "hw:0,0" -acodec g726 -ac 1 -ar 8000 /tmp/test1.mkv to produce an mkv with g726 audio (gstreamer still fails to play it)
Any idea why it fails to play with GStreamer? avdec_g726 should handle it in theory but complains about unsupported number of bits here... while avplay just plays it fine. commit 3a8c1b3550383b613afa692a5720e8f956ae4964 Author: Nicola Murino <nicola.murino@gmail.com> Date: Fri Jan 3 15:24:29 2014 +0100 riff: Add G726 ADPCM support https://bugzilla.gnome.org/show_bug.cgi?id=720995 commit 2cddf3a0a9008bf78f9c418bc6d87843a0627eab Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Jan 8 09:46:55 2014 +0100 matroskamux: Error out if ADPCM caps don't contain the layout field commit bbb5a2853e26d09a95edbc8091f27d46ced0e418 Author: Nicola Murino <nicola.murino@gmail.com> Date: Fri Jan 3 15:25:23 2014 +0100 matroskamux: Add support for g726 ADPCM https://bugzilla.gnome.org/show_bug.cgi?id=720995
Created attachment 265683 [details] [review] fix gstreamer playback Please note that the attached patch assume that the bitrate information in the header is correct, for other adpcm formats there are comments such this /* Many encoding tools create a wrong bitrate information in the * header, so either we calculate the bitrate or mark it as invalid * as this would probably confuse timing */ and the bitrate is recalculated another note: gst-launch-1.0 -v alsasrc num-buffers=1000 ! avenc_g726 ! fakesink silent=false show block_align setted to 0, if I encode with libav directly block align is set to 1
the latest patch is missing null check (if (strf != NULL) { ...) if there are no other thing to change I'll resend later with this modification, what is the correct things to do if strf is NULL? without this info gstreamer will be unable to playback the file anyway
Review of attachment 265683 [details] [review]: I think we should make sure that a) the bitrate in riff-media is correctly calculated for this case too, b) block_align is properly set there ::: gst-libs/gst/riff/riff-media.c @@ +1398,3 @@ caps = gst_caps_new_simple ("audio/x-adpcm", + "layout", G_TYPE_STRING, "g726", "bitrate", G_TYPE_INT, + strf->av_bps * 8, NULL); and here check for strf!=NULL, and also strf->av_bps!=0. If we have no valid bitrate we should fail muxing.
I think you're supposed to return the template caps if strf is NULL.
thanks for the review, I'll work on the patch later today or this weekend, regarding the invalid bitrate and block align during mux, in the patch I sent for matroskamux actually we do these checks: if (!gst_structure_get_int (structure, "block_align", &block_align)) { GST_WARNING_OBJECT (mux, "Missing block_align on adpcm caps"); goto refuse_caps; } if (!gst_structure_get_int (structure, "bitrate", &bitrate)) { GST_WARNING_OBJECT (mux, "Missing bitrate on adpcm g726 caps"); goto refuse_caps; } probably bitrate = 0 or block_align = 0 are invalid too, do you want I'll add these checks in matroskamux too? Probably the same considerations apply for others audio format in matroskamux, for example wma, additionally avenc_g726 seems to produce a wrong block_align (please try gst-launch-1.0 -v alsasrc ! avenc_g726 ! fakesink and see the output), for what I can understand block_align should be 1 for g726 audio, however I'm not 100% sure
(In reply to comment #12) > thanks for the review, > > I'll work on the patch later today or this weekend, regarding the invalid > bitrate and block align during mux, in the patch I sent for matroskamux > actually we do these checks: > > if (!gst_structure_get_int (structure, "block_align", &block_align)) { > GST_WARNING_OBJECT (mux, "Missing block_align on adpcm caps"); > goto refuse_caps; > } > > if (!gst_structure_get_int (structure, "bitrate", &bitrate)) { > GST_WARNING_OBJECT (mux, "Missing bitrate on adpcm g726 caps"); > goto refuse_caps; > } > > probably bitrate = 0 or block_align = 0 are invalid too, do you want I'll add > these checks in matroskamux too? Probably the same considerations apply for > others audio format in matroskamux, for example wma, Yes, I think so too. > additionally avenc_g726 seems to produce a wrong block_align (please try > gst-launch-1.0 -v alsasrc ! avenc_g726 ! fakesink and see the output), for what > I can understand block_align should be 1 for g726 audio, however I'm not 100% > sure I don't know either, sorry. But 0 seems wrong in any case :) That's the value libav gives us though.
based on this link: http://blogs.msdn.com/b/dawate/archive/2009/06/23/intro-to-audio-programming-part-2-demystifying-the-wav-format.aspx the block align for G726 should be: 2 for 16kbit/s 3 for 24kbit/s 4 for 32kbit/s 5 for 40kbit/s these values are obtained as bitrate/rate, the values are confirmed here: http://en.wikipedia.org/wiki/G.726 on my system using avconv (0.8.9 system version) like this: avconv -f alsa -i "hw:0,0" -acodec g726 -ac 1 -ar 8000 -ab 32K /tmp/test1.mkv I have: - block_align 5 for bitrate 40000 (ok) - block_align 1 for bitrate 32000 (wrong, should be 4) - block_align 3 for bitrate 24000 (ok) - block_align 1 for bitrate 16000 (wrong should be 2) gstreamer can produce g726 audio using avenc_g726, bitrate is configurable on the encoder and works as excepected (it accept values greater that 40000 and smaller than 16000 but if you set bitrate 12000 it will be automatically adjusted to 16000 and 48000 will be automatically adjusted to 40000) but block align is ever set to 0 since avconv and so many encoder out of there set wrong block align I suggest - to be permissive for muxing (to be able to mux audio produced by ourself), so no change to the actual muxing code - in riff-media.c adjust block align based on bitrate if strf->av_bps != 0 and so return correct bitrate and block align - in riff-media.c return caps as "audio/x-adpcm","layout", G_TYPE_STRING, "g726" if strf is null or strf->av_bps == 0 and block_align is invalid (<2 or >5), this will make the file unplayable and should be correct since the bitrate is invalid or missing - in riff-media.c recalculate bitrate based on block-align if block align is valid (between 2 and 5) and bitrate invalid (0) - to be permissive if both block_align and bitrate are valid but not compatible each other (for example bitrate 40000 and block_align 2) what do you think about? Am I missing something?
forgot about the latest point, we end up with bitrate and block align ever valid if we adjust block align based on bitrate if bitrate !=0 and viceversa
Ok, so please also report a bug against libav :) Otherwise your plan sounds like a good solution to me
Created attachment 266616 [details] [review] fix G726 playback please review, bug against libav here: https://bugzilla.libav.org/show_bug.cgi?id=625
Created attachment 266617 [details] [review] fix G726 playback
commit 26e3c92093892729f6a3bd9d4a98d6291050c25a Author: Nicola Murino <nicola.murino@gmail.com> Date: Sat Jan 18 19:22:12 2014 +0100 riff: Fix G726 caps creation https://bugzilla.gnome.org/show_bug.cgi?id=720995