Re: [FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c
2010/1/9 Ronald S. Bultje rsbul...@gmail.com:
Hi,
On Fri, Jan 8, 2010 at 10:19 PM, Vitor Sessak vitor1...@gmail.com wrote:
I think that this is only guaranteed to work if you declare the AMRNBFrame
struct as packed, since the spec allows the compiler to add some padding
space inside structs otherwise.
I'm not sure that's true (at least in practice), since practically
AMRNBFrame is a uint16_t[]. I'd say this is OK in principle (although
I have to go through it a little to see if this is actually easier).
Ronald
I could use an extra byte for the field index (217 extra bytes of ROM
total). Then I would have the option of using offsetof.
According to the following article, old versions of the Think C
compiler would add padding to something like AMRNBSubframe (i.e.
struct { uint16_t a,b,c,d[10]; } ):
http://www.goingware.com/tips/getting-started/alignment.html
--
Colin McQuillan
___
FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Re: [FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c
Colin McQuillan wrote: 2010/1/9 Ronald S. Bultje rsbul...@gmail.com: Hi, On Fri, Jan 8, 2010 at 10:19 PM, Vitor Sessak vitor1...@gmail.com wrote: I think that this is only guaranteed to work if you declare the AMRNBFrame struct as packed, since the spec allows the compiler to add some padding space inside structs otherwise. I'm not sure that's true (at least in practice), since practically AMRNBFrame is a uint16_t[]. I'd say this is OK in principle (although I have to go through it a little to see if this is actually easier). Ronald I could use an extra byte for the field index (217 extra bytes of ROM total). Then I would have the option of using offsetof. I think it is a nice idea. I think spec compliance an future-proofing is worth 217 bytes of table size. -Vitor ___ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Re: [FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c
On Fri, Jan 08, 2010 at 10:19:54PM -0500, Vitor Sessak wrote:
cmcq wrote:
Author: cmcq
Date: Fri Jan 8 18:16:25 2010
New Revision: 5570
Log:
Save about 1KB of space using a field-to-bit lookup instead of
bit-to-field.
Modified:
amr/amrnbdata.h
amr/amrnbdec.c
Modified: amr/amrnbdata.h
==
--- amr/amrnbdata.h Fri Jan 8 18:13:05 2010(r5569)
+++ amr/amrnbdata.h Fri Jan 8 18:16:25 2010(r5570)
@@ -68,15 +68,6 @@ enum Mode {
#define LP_FILTER_ORDER 10/// linear predictive coding filter
order
[...]
+// Each field in AMRNBFrame is stored as:
+// one byte of 16 * index in AMRNBFrame relative to previous field
+// + number of bits in the field
+// then, one byte for each bit of the field (from most-significant to
least)
+// of the position of that bit in the AMR frame.
+static const uint8_t order_MODE_4k75[] = {
+0x28, 7, 6, 5, 4, 3, 2, 1, 0,
+0x18, 15, 14, 13, 12, 11, 10, 9, 8,
+0x17, 51, 35, 34, 50, 33, 49, 32,
+0x38, 23, 22, 21, 20, 19, 18, 43, 42,
[...]
if (mode = MODE_DTX) {
uint16_t *data = (uint16_t *)p-frame;
-const AMROrder *order = amr_unpacking_bitmaps_per_mode[mode];
-int i;
+const uint8_t *order = amr_unpacking_bitmaps_per_mode[mode];
+int field_header; // 16 * relative field index + number of field
bits
memset(p-frame, 0, sizeof(AMRNBFrame));
-for (i = 0; i mode_bits[mode]; i++)
-data[order[i].index] += get_bits1(gb) order[i].bit;
+buf++;
+while ((field_header = *order++)) {
+int field = 0;
+data += field_header 4;
+field_header = 0xf;
+while (field_header--) {
+ int bit = *order++;
+ field = 1;
+ field |= buf[bit 3] (bit 7) 1;
+}
+*data = field;
+}
I think that this is only guaranteed to work if you declare the AMRNBFrame
struct as packed, since the spec allows the compiler to add some padding
space inside structs otherwise.
the struct contains only uint16_t fields, so this would seem odd to me if
a compiler did it but iam not saying no compiler does ...
Also I'm undecided if the extra complexity is worth the 1kb smaller tables
(of whose only 400 bytes at most would be used in a single frame). Michael,
since you are the one who suggested trying to make the tables smaller, was
that you had in mind?
I like the new code better than the old
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
signature.asc
Description: Digital signature
___
FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Re: [FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c
Michael Niedermayer wrote:
On Fri, Jan 08, 2010 at 10:19:54PM -0500, Vitor Sessak wrote:
cmcq wrote:
Author: cmcq
Date: Fri Jan 8 18:16:25 2010
New Revision: 5570
Log:
Save about 1KB of space using a field-to-bit lookup instead of
bit-to-field.
Modified:
amr/amrnbdata.h
amr/amrnbdec.c
Modified: amr/amrnbdata.h
==
--- amr/amrnbdata.h Fri Jan 8 18:13:05 2010(r5569)
+++ amr/amrnbdata.h Fri Jan 8 18:16:25 2010(r5570)
@@ -68,15 +68,6 @@ enum Mode {
#define LP_FILTER_ORDER 10/// linear predictive coding filter
order
[...]
+// Each field in AMRNBFrame is stored as:
+// one byte of 16 * index in AMRNBFrame relative to previous field
+// + number of bits in the field
+// then, one byte for each bit of the field (from most-significant to
least)
+// of the position of that bit in the AMR frame.
+static const uint8_t order_MODE_4k75[] = {
+0x28, 7, 6, 5, 4, 3, 2, 1, 0,
+0x18, 15, 14, 13, 12, 11, 10, 9, 8,
+0x17, 51, 35, 34, 50, 33, 49, 32,
+0x38, 23, 22, 21, 20, 19, 18, 43, 42,
[...]
if (mode = MODE_DTX) {
uint16_t *data = (uint16_t *)p-frame;
-const AMROrder *order = amr_unpacking_bitmaps_per_mode[mode];
-int i;
+const uint8_t *order = amr_unpacking_bitmaps_per_mode[mode];
+int field_header; // 16 * relative field index + number of field
bits
memset(p-frame, 0, sizeof(AMRNBFrame));
-for (i = 0; i mode_bits[mode]; i++)
-data[order[i].index] += get_bits1(gb) order[i].bit;
+buf++;
+while ((field_header = *order++)) {
+int field = 0;
+data += field_header 4;
+field_header = 0xf;
+while (field_header--) {
+ int bit = *order++;
+ field = 1;
+ field |= buf[bit 3] (bit 7) 1;
+}
+*data = field;
+}
I think that this is only guaranteed to work if you declare the AMRNBFrame
struct as packed, since the spec allows the compiler to add some padding
space inside structs otherwise.
the struct contains only uint16_t fields, so this would seem odd to me if
a compiler did it but iam not saying no compiler does ...
Is there any disadvantage in using packed?
-Vitor
___
FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Re: [FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c
On Sat, Jan 09, 2010 at 09:29:47AM -0500, Vitor Sessak wrote:
Michael Niedermayer wrote:
On Fri, Jan 08, 2010 at 10:19:54PM -0500, Vitor Sessak wrote:
cmcq wrote:
Author: cmcq
Date: Fri Jan 8 18:16:25 2010
New Revision: 5570
Log:
Save about 1KB of space using a field-to-bit lookup instead of
bit-to-field.
Modified:
amr/amrnbdata.h
amr/amrnbdec.c
Modified: amr/amrnbdata.h
==
--- amr/amrnbdata.hFri Jan 8 18:13:05 2010(r5569)
+++ amr/amrnbdata.hFri Jan 8 18:16:25 2010(r5570)
@@ -68,15 +68,6 @@ enum Mode {
#define LP_FILTER_ORDER 10/// linear predictive coding filter
order
[...]
+// Each field in AMRNBFrame is stored as:
+// one byte of 16 * index in AMRNBFrame relative to previous field
+// + number of bits in the field
+// then, one byte for each bit of the field (from most-significant to
least)
+// of the position of that bit in the AMR frame.
+static const uint8_t order_MODE_4k75[] = {
+0x28, 7, 6, 5, 4, 3, 2, 1, 0,
+0x18, 15, 14, 13, 12, 11, 10, 9, 8,
+0x17, 51, 35, 34, 50, 33, 49, 32,
+0x38, 23, 22, 21, 20, 19, 18, 43, 42,
[...]
if (mode = MODE_DTX) {
uint16_t *data = (uint16_t *)p-frame;
-const AMROrder *order = amr_unpacking_bitmaps_per_mode[mode];
-int i;
+const uint8_t *order = amr_unpacking_bitmaps_per_mode[mode];
+int field_header; // 16 * relative field index + number of
field bits
memset(p-frame, 0, sizeof(AMRNBFrame));
-for (i = 0; i mode_bits[mode]; i++)
-data[order[i].index] += get_bits1(gb) order[i].bit;
+buf++;
+while ((field_header = *order++)) {
+int field = 0;
+data += field_header 4;
+field_header = 0xf;
+while (field_header--) {
+ int bit = *order++;
+ field = 1;
+ field |= buf[bit 3] (bit 7) 1;
+}
+*data = field;
+}
I think that this is only guaranteed to work if you declare the
AMRNBFrame struct as packed, since the spec allows the compiler to add
some padding space inside structs otherwise.
the struct contains only uint16_t fields, so this would seem odd to me if
a compiler did it but iam not saying no compiler does ...
Is there any disadvantage in using packed?
dunno, benchmark
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
signature.asc
Description: Digital signature
___
FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Re: [FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c
Michael Niedermayer wrote:
On Sat, Jan 09, 2010 at 09:29:47AM -0500, Vitor Sessak wrote:
Michael Niedermayer wrote:
On Fri, Jan 08, 2010 at 10:19:54PM -0500, Vitor Sessak wrote:
cmcq wrote:
Author: cmcq
Date: Fri Jan 8 18:16:25 2010
New Revision: 5570
Log:
Save about 1KB of space using a field-to-bit lookup instead of
bit-to-field.
Modified:
amr/amrnbdata.h
amr/amrnbdec.c
Modified: amr/amrnbdata.h
==
--- amr/amrnbdata.h Fri Jan 8 18:13:05 2010(r5569)
+++ amr/amrnbdata.h Fri Jan 8 18:16:25 2010(r5570)
@@ -68,15 +68,6 @@ enum Mode {
#define LP_FILTER_ORDER 10/// linear predictive coding filter
order
[...]
+// Each field in AMRNBFrame is stored as:
+// one byte of 16 * index in AMRNBFrame relative to previous field
+// + number of bits in the field
+// then, one byte for each bit of the field (from most-significant to
least)
+// of the position of that bit in the AMR frame.
+static const uint8_t order_MODE_4k75[] = {
+0x28, 7, 6, 5, 4, 3, 2, 1, 0,
+0x18, 15, 14, 13, 12, 11, 10, 9, 8,
+0x17, 51, 35, 34, 50, 33, 49, 32,
+0x38, 23, 22, 21, 20, 19, 18, 43, 42,
[...]
if (mode = MODE_DTX) {
uint16_t *data = (uint16_t *)p-frame;
-const AMROrder *order = amr_unpacking_bitmaps_per_mode[mode];
-int i;
+const uint8_t *order = amr_unpacking_bitmaps_per_mode[mode];
+int field_header; // 16 * relative field index + number of
field bits
memset(p-frame, 0, sizeof(AMRNBFrame));
-for (i = 0; i mode_bits[mode]; i++)
-data[order[i].index] += get_bits1(gb) order[i].bit;
+buf++;
+while ((field_header = *order++)) {
+int field = 0;
+data += field_header 4;
+field_header = 0xf;
+while (field_header--) {
+ int bit = *order++;
+ field = 1;
+ field |= buf[bit 3] (bit 7) 1;
+}
+*data = field;
+}
I think that this is only guaranteed to work if you declare the
AMRNBFrame struct as packed, since the spec allows the compiler to add
some padding space inside structs otherwise.
the struct contains only uint16_t fields, so this would seem odd to me if
a compiler did it but iam not saying no compiler does ...
Is there any disadvantage in using packed?
dunno, benchmark
Unsurprisingly, the .o is unchanged if I add __attribute__((packed))
to the struct. I'm not really fan neither of adding gnu-extensions to
the code or of having a code with undefined behavior according to the
C99 standard...
-Vitor
___
FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Re: [FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c
Hi, On Fri, Jan 8, 2010 at 10:19 PM, Vitor Sessak vitor1...@gmail.com wrote: I think that this is only guaranteed to work if you declare the AMRNBFrame struct as packed, since the spec allows the compiler to add some padding space inside structs otherwise. I'm not sure that's true (at least in practice), since practically AMRNBFrame is a uint16_t[]. I'd say this is OK in principle (although I have to go through it a little to see if this is actually easier). Ronald ___ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Re: [FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c
cmcq wrote:
Author: cmcq
Date: Fri Jan 8 18:16:25 2010
New Revision: 5570
Log:
Save about 1KB of space using a field-to-bit lookup instead of bit-to-field.
Modified:
amr/amrnbdata.h
amr/amrnbdec.c
Modified: amr/amrnbdata.h
==
--- amr/amrnbdata.h Fri Jan 8 18:13:05 2010(r5569)
+++ amr/amrnbdata.h Fri Jan 8 18:16:25 2010(r5570)
@@ -68,15 +68,6 @@ enum Mode {
#define LP_FILTER_ORDER 10/// linear predictive coding filter order
[...]
+// Each field in AMRNBFrame is stored as:
+// one byte of 16 * index in AMRNBFrame relative to previous field
+// + number of bits in the field
+// then, one byte for each bit of the field (from most-significant to least)
+// of the position of that bit in the AMR frame.
+static const uint8_t order_MODE_4k75[] = {
+0x28, 7, 6, 5, 4, 3, 2, 1, 0,
+0x18, 15, 14, 13, 12, 11, 10, 9, 8,
+0x17, 51, 35, 34, 50, 33, 49, 32,
+0x38, 23, 22, 21, 20, 19, 18, 43, 42,
[...]
if (mode = MODE_DTX) {
uint16_t *data = (uint16_t *)p-frame;
-const AMROrder *order = amr_unpacking_bitmaps_per_mode[mode];
-int i;
+const uint8_t *order = amr_unpacking_bitmaps_per_mode[mode];
+int field_header; // 16 * relative field index + number of field bits
memset(p-frame, 0, sizeof(AMRNBFrame));
-for (i = 0; i mode_bits[mode]; i++)
-data[order[i].index] += get_bits1(gb) order[i].bit;
+buf++;
+while ((field_header = *order++)) {
+int field = 0;
+data += field_header 4;
+field_header = 0xf;
+while (field_header--) {
+ int bit = *order++;
+ field = 1;
+ field |= buf[bit 3] (bit 7) 1;
+}
+*data = field;
+}
I think that this is only guaranteed to work if you declare the
AMRNBFrame struct as packed, since the spec allows the compiler to add
some padding space inside structs otherwise.
Also I'm undecided if the extra complexity is worth the 1kb smaller
tables (of whose only 400 bytes at most would be used in a single
frame). Michael, since you are the one who suggested trying to make the
tables smaller, was that you had in mind?
-Vitor
___
FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
