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 650714 - [amrparse] skips first few frames (problem in checking sync)
[amrparse] skips first few frames (problem in checking sync)
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.10.29
Other Linux
: Normal major
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-05-21 07:20 UTC by SeungBae Shin
Modified: 2011-09-05 14:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Code patch (521 bytes, patch)
2011-05-21 07:49 UTC, SeungBae Shin
rejected Details | Review
amr nb file with mode 7 (32bytes per frame including header byte) (23.44 KB, audio/AMR)
2011-05-23 11:26 UTC, SeungBae Shin
  Details

Description SeungBae Shin 2011-05-21 07:20:05 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))) {
Comment 1 SeungBae Shin 2011-05-21 07:49:26 UTC
Created attachment 188272 [details] [review]
Code patch

Code patch to get correct sync position
Comment 2 Sebastian Dröge (slomo) 2011-05-23 08:50:24 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2011-05-23 08:50:58 UTC
Can you attach a sample file that reproduces this behaviour?
Comment 4 Tim-Philipp Müller 2011-05-23 08:52:06 UTC
Could you make such an AMR file that is not parsed properly available? (e.g. attach it to this bug or upload it somewhere)
Comment 5 SeungBae Shin 2011-05-23 11:26:46 UTC
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)
Comment 6 SeungBae Shin 2011-05-23 11:50:21 UTC
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.
Comment 7 SeungBae Shin 2011-05-30 07:49:26 UTC
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);
   }
Comment 8 Mark Nauwelaerts 2011-09-05 14:06:06 UTC
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.