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 720995 - matroskamux: add g726 adpcm support
matroskamux: add g726 adpcm support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-23 23:06 UTC by Nicola
Modified: 2014-01-19 09:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
adpcm g726 wave format (1.91 KB, patch)
2013-12-23 23:06 UTC, Nicola
needs-work Details | Review
adpcm g726 support in matroskamux (2.27 KB, patch)
2013-12-23 23:07 UTC, Nicola
needs-work Details | Review
adpcm g726 wave format (1.65 KB, patch)
2014-01-03 14:28 UTC, Nicola
committed Details | Review
matroskamux: add support for g726 adpcm (2.01 KB, patch)
2014-01-03 14:31 UTC, Nicola
committed Details | Review
fix gstreamer playback (985 bytes, patch)
2014-01-08 10:41 UTC, Nicola
needs-work Details | Review
fix G726 playback (1.75 KB, patch)
2014-01-18 18:15 UTC, Nicola
none Details | Review
fix G726 playback (1.75 KB, patch)
2014-01-18 18:23 UTC, Nicola
committed Details | Review

Description Nicola 2013-12-23 23:06:53 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
Comment 1 Nicola 2013-12-23 23:07:41 UTC
Created attachment 264824 [details] [review]
adpcm g726 support in matroskamux
Comment 2 Sebastian Dröge (slomo) 2014-01-03 09:49:29 UTC
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 3 Sebastian Dröge (slomo) 2014-01-03 09:51:09 UTC
Comment on attachment 264824 [details] [review]
adpcm g726 support in matroskamux

Also without the common changes please
Comment 4 Nicola 2014-01-03 14:28:25 UTC
Created attachment 265220 [details] [review]
adpcm g726 wave format

modified patch
Comment 5 Nicola 2014-01-03 14:31:11 UTC
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
Comment 6 Nicola 2014-01-03 14:41:03 UTC
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)
Comment 7 Sebastian Dröge (slomo) 2014-01-08 08:58:34 UTC
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
Comment 8 Nicola 2014-01-08 10:41:09 UTC
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
Comment 9 Nicola 2014-01-08 10:48:32 UTC
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
Comment 10 Sebastian Dröge (slomo) 2014-01-08 10:54:07 UTC
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.
Comment 11 Tim-Philipp Müller 2014-01-08 10:59:26 UTC
I think you're supposed to return the template caps if strf is NULL.
Comment 12 Nicola 2014-01-08 11:18:19 UTC
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
Comment 13 Sebastian Dröge (slomo) 2014-01-08 11:25:15 UTC
(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.
Comment 14 Nicola 2014-01-08 15:04:07 UTC
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?
Comment 15 Nicola 2014-01-08 15:07:42 UTC
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
Comment 16 Sebastian Dröge (slomo) 2014-01-09 16:25:23 UTC
Ok, so please also report a bug against libav :)

Otherwise your plan sounds like a good solution to me
Comment 17 Nicola 2014-01-18 18:15:58 UTC
Created attachment 266616 [details] [review]
fix G726 playback

please review,

bug against libav here:

https://bugzilla.libav.org/show_bug.cgi?id=625
Comment 18 Nicola 2014-01-18 18:23:42 UTC
Created attachment 266617 [details] [review]
fix G726 playback
Comment 19 Sebastian Dröge (slomo) 2014-01-19 09:35:12 UTC
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