[FFmpeg-devel] [PATCH] G.723.1 Decoder

[Mohamed Naufal]

On 12 August 2010 07:53, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>

[...]

>> +typedef struct {
>> + ? ?int16_t index; ? ?///< postfilter backward/forward lag
>> + ? ?int16_t opt_gain; ///< optimal gain
>> + ? ?int16_t sc_gain; ?///< scaling gain
>> +} PPFParam;
>
> Why are these not regular ints?
>


opt_gain and sc_gain are passed as int16_t arguments to
ff_acelp_weighted_vector_sum(). Changed the rest to int.

[...]

>> + ? ?for (i = 0; i < SUBFRAMES; i++)
>> + ? ? ? ?p->subframe[i].grid_index = get_bits(&gb, 1);
> [..]
>> + ? ?} else { /* Rate5k3 */
>> + ? ? ? ?for (i = 0; i < SUBFRAMES; i++)
>> + ? ? ? ? ? ?p->subframe[i].pulse_pos ?= get_bits(&gb, 12);
>> +
>> + ? ? ? ?for (i = 0; i < SUBFRAMES; i++)
>> + ? ? ? ? ? ?p->subframe[i].pulse_sign = get_bits(&gb, 4);
>> + ? ?}
>
> You should probably unroll these manually.
>


Done.

>> +static int16_t square_root(int val)
> [..]
>> + ? ?for (i = 0; i < 14; i ++) {
>> + ? ? ? ?temp = (res+exp) * (res+exp) << 1;
>
> int res_exp = res+exp; temp = res_exp*res_exp<<1
>
>> + ? ? ? ?if (val >= temp)
>> + ? ? ? ? ? ?res += exp;
>
> Also, since temp is only used once, you don't actually need it.
>


Fixed.

> Unimportant, but is res|exp faster than res+exp?
>


I believe both are comparable. I don't understand how this applies here though.

[...]

>> +static inline int dot_product(const int16_t *v1, const int16_t *v2, int length)
>
> I don't mind if you move this to celp_math.[ch], and you can probably
> extract this from other int-speech codecs also then.
>


Done.

>> + ? ?int64_t prod;
>> +
>> + ? ?for (i = 0; i < length; i++) {
>> + ? ? ? ?prod = av_clipl_int32((int64_t)v1[i] * v2[i] << 1);
>
> You should declare the int64_t with a local scope here so gcc knows it
> doesn't have to remember its value...
>

Done.

>> +static int16_t normalize_bits_int16(int16_t x)
[...]
>> +static int16_t normalize_bits_int32(int x)
>
> You can probably make a macro since these two are so similar.
>


Merged into a single function.

[...]

>> + ? ?if (!bad_frame) {
>> + ? ? ? ?min_dist = 0x100;
>> + ? ? ? ?pred = 12288;
>> + ? ?} else {
>> + ? ? ? ?min_dist = 0x200;
>> + ? ? ? ?pred = 23552;
>> + ? ? ? ?lsp_index[0] = lsp_index[1] = lsp_index[2] = 0;
>> + ? ?}
>
> Vertical align here.
>


Done.

>> +static void lsp2lpc(int16_t *lpc)
>
> How does this compare to ff_acelp_lsp2lpc()? A comment would be nice,
> sharing code better.
>


Added comment.

>> +static int16_t autocorr_max(G723_1_Context *p, int offset, int *ccr_max,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?int16_t pitch_lag, int length)
> [..]
>> +static int16_t autocorr_max_inv(G723_1_Context *p, int offset, int *ccr_max,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int16_t pitch_lag, int length)
>
> These two are highly similar, can they be combined?
>


Done.

>> +static void residual_interp(int16_t *buf, int16_t *out, int16_t lag,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?int16_t gain, int16_t *rseed)
> [..]
>> + ? ?if (lag) { /* Voiced */
>> + ? ? ? ?int16_t *vector_ptr = buf + PITCH_MAX;
>> + ? ? ? ?/* Attenuate */
>> + ? ? ? ?for (i = 0; i < lag; i++)
>> + ? ? ? ? ? ?vector_ptr[i - lag] = vector_ptr[i - lag] * 3 >> 2;
>> + ? ? ? ?for (i = 0; i < FRAME_LEN; i++)
>> + ? ? ? ? ? ?vector_ptr[i] = vector_ptr[i - lag];
>
> av_memcpy_backptr().
>


Fixed.

>> + ? ? ? ?memcpy(out, vector_ptr, FRAME_LEN * sizeof(int16_t));
>
> The temp buffer shouldn't be strictly necessary.
>


The contents of p->excitation(which is referenced by vector_ptr) are
modified in comp_interp_index().

> + ? ?} else { ?/* Unvoiced */
> + ? ? ? ?for (i = 0; i < FRAME_LEN; i++)
> + ? ? ? ? ? ?out[i] = gain * prand(rseed) >> 15;
> + ? ? ? ?memset(buf, 0, (FRAME_LEN + PITCH_MAX) * sizeof(int16_t));
>
> Given that this is the only use of prand(), I wonder if you could
> optimize it by integrating prand() in here, so that you don't have to
> keep de-de-derefing rseed within the loop. AFAICS, this is also the
> only use of the "random seed" anyway.
>


Done.

>> +static void iir_filter(int16_t *lpc, int16_t *src,
>> + ? ? ? ? ? ? ? ? ? ? ? int *dest)
>
> celp_filters.h calls this zero_synthesis_filter, can this be merged in there?
>


Macrofied iir_filter to support both 16bit & 32bit output(for use in
the encoder).

>> +static void gain_scale(G723_1_Context *p, int16_t * buf, int energy)
>> +{
>> + ? ?for (i = 0; i < SUBFRAME_LEN; i++) {
>> + ? ? ? ?temp ?= av_clipl_int32((int64_t)(buf[i] >> 2) * (buf[i] >> 2) << 1);
>> + ? ? ? ?denom = av_clipl_int32((int64_t)denom + temp);
>> + ? ?}
>
> I believe Mans once wrote a macro for simplifying 64-bit
> multiplications, these could also be optimized on archs supporting
> 32x32->64muls (such as muleax->eax/edx on x86, I believe Mans cared
> about it for Arm though), called MUL64()? Also applies to a few other
> places.
>


Fixed.

[...]


Thanks
Naufal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01_synth_filter.patch
Type: text/x-patch
Size: 2534 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101001/ba491df6/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02_dot_product.patch
Type: text/x-patch
Size: 1409 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101001/ba491df6/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 03_data_tables.patch
Type: text/x-patch
Size: 78180 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101001/ba491df6/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 03_decoder.patch
Type: text/x-patch
Size: 38029 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101001/ba491df6/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 03_demuxer.patch
Type: text/x-patch
Size: 4644 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101001/ba491df6/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 03_doc.patch
Type: text/x-patch
Size: 804 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101001/ba491df6/attachment-0005.bin>