GNOME Bugzilla – Bug 628609
The qtwrapperaudiodec_samr decoder doesn't handle buffers containing many AMR frames properly
Last modified: 2011-05-30 12:46:20 UTC
Created attachment 169354 [details] A GDP dump of AMR packets sent over RTP If decoding the attached sample like this, gst-launch filesrc location=rtp-amr-dump.gdp ! gdpdepay ! rtpamrdepay ! qtwrapperaudiodec_samr ! wavenc ! filesink location=out-qt.wav I get a wav file with 2.5 seconds of audio. If decoding it using ffdec_amrnb instead, gst-launch filesrc location=rtp-amr-dump.gdp ! gdpdepay ! rtpamrdepay ! ffdec_amrnb ! wavenc ! filesink location=out-ff.wav I get a wav file that contains 17.5 seconds of audio (which is the correct length). The RTP packets in this dump contain 35 AMR frames each. If I split the packets into individual AMR frames, e.g. via this operation, the file decodes fine: gst-launch filesrc location=rtp-amr-dump.gdp ! gdpdepay ! rtpamrdepay ! ffmux_amr ! filesink location=dump.amr gst-launch filesrc location=dump.amr ! amrparse ! qtwrapperaudiodec_samr ! wavenc ! filesink location=out-qt2.wav
FWIW, it seems that it handles packets with up to 5 frames per packet just fine, if there's any more than that, it decodes too little data.
Created attachment 169394 [details] [review] Patch solving this issue The attached patch seems to solve the issue. The process_buffer_cb function is called whenever SCAudioFillBuffer needs more data to decode, but even if all input data is returned, SCAudioFillBuffer might not consume all of it at once. Once it has consumed all input data, and process_buffer_cb has no more data to give, process_buffer_cb returns 42, which is returned from SCAudioFillBuffer as status, indicating that all input data has been consumed. Alternatively, another solution would be to increase the size of the buffer allocated around lines 658-663: /* Create output bufferlist, big enough for 200ms of audio */ GST_DEBUG_OBJECT (qtwrapper, "Allocating bufferlist for %d channels", channels); qtwrapper->bufferlist = AllocateAudioBufferList (channels, qtwrapper->samplerate / 5 * qtwrapper->channels * 4); Although this seems to be meant to give a buffer size of 200 ms, I'm only able to fill it with 100 ms of audio in practice in this test case. Nevertheless, properly looping until all data has been consumed instead of increasing the buffer size blindly feels more sensible to me.
Marking bug as blocker so we don't forget to have a look at it before the next release. Thanks for the patch.
Ping? I can't say much about that patch but this all looks a bit hacky ;)
It looks a bit hacky yes, but should be correct IMO. Basically, it's a special return value (which can be chosen quite arbitrarily) signalled from the callback function to the caller. To clean it up, one could add e.g. #define NO_MORE_INPUT_DATA 42 at the start of the file, and replace all the occurrances of the magic value with the define instead.
I keep forgetting to push this until it's too close to the release, sorry. (And I can't test it myself) Yes, a define for the magic value would definitely be good as well.
Created attachment 187156 [details] [review] Cleanup patch
Created attachment 187157 [details] [review] Patch fixing the bug
And 42 can never be returned in normal cases? Other than that this looks good
As far as I know, 42 can't be returned in normal cases (I think the return values are 0 or what we return from the callback, or some QT specific error codes which probably can't be 42). I've seen other uses of similar CoreAudio APIs where the value 1 was used for the same purpose.
commit d536b73e25403bfbdd6dcc558fb82d0e507ea8b3 Author: Martin Storsjo <martin@martin.st> Date: Thu Sep 2 23:31:23 2010 +0300 qtwrapper: Decode audio until all input data is consumed The special return value is returned from our buffer callback when all input data has been consumed. commit 02fc41fde55ea777bed6018aa8bef6dbe164405f Author: Martin Storsjo <martin@martin.st> Date: Tue May 3 14:14:20 2011 +0300 qtwrapper: Replace the hackish 42 magic number with a define
Thanks!