GNOME Bugzilla – Bug 736797
audio: correct condition for MPEG case in iec61937 / SPDIF payloader
Last modified: 2014-09-27 10:15:40 UTC
Created attachment 286369 [details] [review] audio: correct condition for MPEG case. gst-libs/gst/audio: In gstaudioiec61937.c for frames=2304 I think layer should be equals to 2 or 3. correct me if I am wrong. Please review Thanks
Hi Anuj, Could you please provide me more information of why it should be layer 2 instead of layer 1?
Because the condition has already been check for layer == 1 in the previous if. So something has to change, but it's not clear what and to which value.
Yeah, it's obviously wrong but not clear what it should be. Someone needs to check the spec.
Yes, I was asking Anuj if he could provide a link to the spec to show why his patch is correct.
I found this equation for calculating frame size: FrameLengthInBytes = 144 * BitRate / SampleRate + Padding. http://www.datavoyage.com/mpgscript/mpeghdr.htm But I am not sure about this doc is the latest one or some modifications have been made into MPEG.
I don't find any newer doc. It looks like a good fix to me. Maybe the frame number can be tweaked in the future. But currently we have two else if cases that have the same condition, so the second one is never going to happen. If Tim and Sebastian are OK with this I will merge the patch.
Note that this is iec61937, so looking at the information for MP1/2/3 does not necessarily help. Also 2304 never appeared anywhere on that website and I also never saw it used in combination with MP1/2/3. So we need to check where this number comes from in iec61937
Created attachment 286986 [details] [review] audio: Fixes for MPEG-2 LSF IEC61937 payloading The low sample frequency case for MPEG-2 is <=12kHz (the 32kHz number applies to MPEG-1). In addition, there was a copy-pasto for the layer 1 vs. layer 2 case. Also adds some documentation of what the fallthrough case is supposed to cover.
The layer fix was correct, but there was another bug fixed by the attached patch. Would be nice if someone could also double-check against the spec to make sure I got it right this time. Else I can push this.
Comment on attachment 286986 [details] [review] audio: Fixes for MPEG-2 LSF IEC61937 payloading Looks like I missed a bunch of related fixes with the 32->12kHz changes. Updated fix coming up.
Comment on attachment 286369 [details] [review] audio: correct condition for MPEG case. Unobsoleting Anuj's patch to keep credit, and adding a couple of patches on top.
Created attachment 286987 [details] [review] audio: Fixes for MPEG-2 LSF IEC61937 payloading The low sample frequency case for MPEG-2 is <=12kHz (the 32kHz number applies to MPEG-1).
Created attachment 286988 [details] [review] audio: Trivial comment for unhandled MPEG-2 payloading case
Just as context for the last trivial comment - the spec mentions a version of the MPEG-2 frame with a base frame and extension frame. I don't have IEC 13818-3 to figure out what that is, and don't see any references in search results, so it's a FIXME for now.
The following fixes have been pushed: 324ebd1 audio: Trivial comment for unhandled MPEG-2 payloading case 2965b79 audio: Fixes for MPEG-2 LSF IEC61937 payloading 798ff6e audio: correct condition for MPEG case.
Created attachment 287223 [details] [review] audio: Trivial comment for unhandled MPEG-2 payloading case The spec mentions a version of the MPEG-2 frame with a base frame and extension frame. I don't have IEC 13818-3 to figure out what that is, and don't see any references in search results, so it's a FIXME for now.
Created attachment 287224 [details] [review] audio: Fixes for MPEG-2 LSF IEC61937 payloading The low sample frequency case for MPEG-2 is <=12kHz (the 32kHz number applies to MPEG-1).
Created attachment 287225 [details] [review] audio: correct condition for MPEG case. Signed-off-by: Anuj Jaiswal <anuj.jaiswal@samsung.com>