GNOME Bugzilla – Bug 650714
[amrparse] skips first few frames (problem in checking sync)
Last modified: 2011-09-05 14:06:06 UTC
Overview: amrparse skips first few frames (problem in checking sync) Steps to Reproduce: 1) Play amrnb file. (any amrnb file, you can test with your own amr file) 2) See that first few frames are not played and skipped. (because start position is not actually correct) Expected Results: amr parser should find correct offset by checking correct sync. Build Date & Platform: Build 2010-05-13 on Ubuntu 10.04 Build 2010-05-13 on ARM target Additional Information: I was using old gstreamer (core of 0.10.29) and recently upgraded to 0.10.33. I also installed latest version of gst-plugins-good (0.10.29), then I found that first few frames are not played since upgrade. Based on observation of amrparser log, It seems that code in check valid frame function is not correct. I think dsize can be same as fsize, so equal should be added. (see code diff below) Code Diff: sbshin80@sbshin80-desktop:~/devel/gst_110512/gst-plugins-good-0.10.29$ diff gst/audioparsers/gstamrparse.c gst/audioparsers/gstamrparse.c.my 312c312 < || (dsize > fsize && (data[fsize] & 0x83) == 0))) { --- > || (dsize >= fsize && (data[fsize] & 0x83) == 0))) {
Created attachment 188272 [details] [review] Code patch Code patch to get correct sync position
Comment on attachment 188272 [details] [review] Code patch This was changed in this commit from >= to >: commit 39da31638699d1b79dbed4caaf4a128f4591d212 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Thu Jan 13 15:26:21 2011 +0100 amrparse: properly check for sufficient available data prior to access And the change from >= to > definitely looks correct, you can't safely access data[fsize] if you allow data to only have size fsize.
Can you attach a sample file that reproduces this behaviour?
Could you make such an AMR file that is not parsed properly available? (e.g. attach it to this bug or upload it somewhere)
Created attachment 188368 [details] amr nb file with mode 7 (32bytes per frame including header byte) I attached the sample amr file which is encoded with mode 7 (32bytes per frame)
I agree that adding equal(=) is not good solution even it works.... I checked the code with different view and found the observation below. Using current code, the maximum size for amrnb is 32 bytes by gst_amr_parse_set_src_caps() static gboolean gst_amr_parse_set_src_caps (GstAmrParse * amrparse) { ..... } else { GST_DEBUG_OBJECT (amrparse, "setting srcpad caps to AMR-NB"); /* Max. size of NB frame is 31 bytes, so we can set the min. frame size to 32 (+1 for next frame header) */ gst_base_parse_set_min_frame_size (GST_BASE_PARSE (amrparse), 32); src_caps = gst_caps_new_simple ("audio/AMR", "channels", G_TYPE_INT, 1, "rate", G_TYPE_INT, 8000, NULL); } ..... } I think the problem is how we can check next frame header bytes with maximum size 32 bytes. in gst_base_parse_scan_frame() , gst_base_parse_pull_range () is called with min size ( max(32, fsize) ) I think data is not enough to check next frame header byte with 32 byte in mode 7.
What if setting minimum frame size for amrnb as 33? In comment description, it calcutate max size as 32 because of 31(max size nb frame) + 1 (for next frame header). My oppinion is that it should be 33 (32 of max nb frame size including frame header byte + 1 for next frame header byte) /* Max. size of NB frame is 31 bytes, so we can set the min. frame size to 32 (+1 for next frame header) */ => /* Max. size of NB frame is 32 bytes, so we can set the min. frame size to 33 (+1 for next frame header) */ Please verifiy my observation. <code diff> diff --git a/gst/audioparsers/gstamrparse.c b/gst/audioparsers/gstamrparse.c index 3f8cfc3..44eea3a 100644 --- a/gst/audioparsers/gstamrparse.c +++ b/gst/audioparsers/gstamrparse.c @@ -175,7 +175,7 @@ gst_amr_parse_set_src_caps (GstAmrParse * amrparse) GST_DEBUG_OBJECT (amrparse, "setting srcpad caps to AMR-NB"); /* Max. size of NB frame is 31 bytes, so we can set the min. frame size to 32 (+1 for next frame header) */ - gst_base_parse_set_min_frame_size (GST_BASE_PARSE (amrparse), 32); + gst_base_parse_set_min_frame_size (GST_BASE_PARSE (amrparse), 33); src_caps = gst_caps_new_simple ("audio/AMR", "channels", G_TYPE_INT, 1, "rate", G_TYPE_INT, 8000, NULL); }
Last suggestion might work/fix (though it would force needing more data than really required), but some amrparse frame checking seemed to be in need of some overhauling, and that appears to have handled this case as well: commit b9a54a38b09ddee217b23bcf0f3a832c845c4e74 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Mon Sep 5 15:50:04 2011 +0200 amrparse: fix and streamline valid frame checking ... to handle various combinations of sync or not, and sufficient data or not as might be expected. Fixes #650714.