[FFmpeg-devel] [PATCH] AMR-WB Decoder

Mon Sep 20 20:10:05 CEST 2010

On 20 September 2010 07:46, Diego Biurrun <diego at biurrun.de> wrote:
> On Mon, Sep 20, 2010 at 11:21:53AM +0200, Vitor Sessak wrote:
>> On 09/20/2010 09:09 AM, Diego Biurrun wrote:
>>> On Sun, Sep 19, 2010 at 11:25:17PM +0200, Vitor Sessak wrote:
>>>> On 09/19/2010 10:54 PM, Diego Biurrun wrote:
>>>>> On Fri, Sep 17, 2010 at 05:15:26PM -0300, Marcelo Galv?o P?voa wrote:
>>>>>>
>>>>>> The code has already been reviewed mostly by Vitor Sessak, discussed
>>>>>> and tested at the ffmpeg-soc list.
>>>>>
>>>>> Then why does this patch not even follow everything listed in the
>>>>> New codecs or formats checklist:
>>>>>
>>>>> http://www.ffmpeg.org/developer.html#SEC7
>>>>>
>>>>> Vitor?
>>>>
>>>> I saw a couple of things that didn't followed the guidelines, but I
>>>> focused on other more important things (code duplication, mostly) and
>>>> forgot to point out the nits later.
>>>
>>> BTW, sorry if I sound like I'm picking on you in particular, this has
>>> been an issue with our SoC processes basically since day one. ?Anyway..
>>>
>>> IMNSHO patches should be cooked into shape on ffmpeg-soc before they
>>> are proposed here.
>>
>> I think this patch is in a very good shape (much better than the average
>> patch posted to -devel), and that's the reason I *explicitly* asked
>> Marcelo to post it here. I don't think a couple of nits proves me wrong.
>
> I'm not talking about the nits. ?Here is the list:
>
> ? 1. ?Did you use av_cold for codec initialization and close functions?
> ? 2. Did you add a long_name under NULL_IF_CONFIG_SMALL to the AVCodec or AVInputFormat/AVOutputFormat struct?
> ? 3. Did you bump the minor version number in `avcodec.h' or `avformat.h'?
> ? 4. Did you register it in `allcodecs.c' or `allformats.c'?
> ? 5. Did you add the CodecID to `avcodec.h'?
> ? 6. If it has a fourcc, did you add it to `libavformat/riff.c', even if it is only a decoder?
> ? 7. Did you add a rule to compile the appropriate files in the Makefile? Remember to do this even if you're just adding a format to a file that is already being compiled by some other rule, like a raw demuxer.
> ? 8. Did you add an entry to the table of supported formats or codecs in `doc/general.texi'?
> ? 9. Did you add an entry in the Changelog?
> ?10. If it depends on a parser or a library, did you add that dependency in configure?
> ?11. Did you "svn add" the appropriate files before commiting?
>
> The patch fails 8. and 9. and I suspect 10. as well.
>

Ok, I think I've fixed these. Please check it out.

On 19 September 2010 17:54, Diego Biurrun <diego at biurrun.de> wrote:
> On Fri, Sep 17, 2010 at 05:15:26PM -0300, Marcelo Galv?o P?voa wrote:
>>
>> The code has already been reviewed mostly by Vitor Sessak, discussed
>> and tested at the ffmpeg-soc list.
>
> Then why does this patch not even follow everything listed in the
> New codecs or formats checklist:
>
> http://www.ffmpeg.org/developer.html#SEC7
>
> Vitor?
>
> Does the Doxygen build without errors or warnings?
>

Yes

> .. some nits below ..
>
>> --- /dev/null
>> +++ b/libavcodec/amrwbdec.c
>> @@ -0,0 +1,1237 @@
>> +/**
>> + * Decodes quantized ISF vectors using 36-bit indices (6K60 mode only)
>
> We write "indexes" in most other places.
>

Fixed

>> + * @param[in] ind ? ? ? ? ? ? ? ? ?Array of 5 indices
>> + * @param[out] isf_q ? ? ? ? ? ? ? Buffer for isf_q[LP_ORDER]
>> + *
>> + */
>> +static void decode_isf_indices_36b(uint16_t *ind, float *isf_q)
>> +{
>> + ? ?int i;
>> +
>> + ? ?for (i = 0; i < 9; i++) {
>> + ? ? ? ?isf_q[i] = dico1_isf[ind[0]][i] * (1.0f / (1 << 15));
>> + ? ?}
>> + ? ?for (i = 0; i < 7; i++) {
>> + ? ? ? ?isf_q[i + 9] = dico2_isf[ind[1]][i] * (1.0f / (1 << 15));
>> + ? ?}
>> + ? ?for (i = 0; i < 5; i++) {
>> + ? ? ? ?isf_q[i] += dico21_isf_36b[ind[2]][i] * (1.0f / (1 << 15));
>> + ? ?}
>> + ? ?for (i = 0; i < 4; i++) {
>> + ? ? ? ?isf_q[i + 5] += dico22_isf_36b[ind[3]][i] * (1.0f / (1 << 15));
>> + ? ?}
>> + ? ?for (i = 0; i < 7; i++) {
>> + ? ? ? ?isf_q[i + 9] += dico23_isf_36b[ind[4]][i] * (1.0f / (1 << 15));
>> + ? ?}
>
> You can drop the {} like you do in most other places I think.
>
> Also, I think the following is more readable.
>
> ? for (i = 0; i < 9; i++)
> ? ? ? isf_q[i] ? ? ?= dico1_isf[ind[0]][i] ? ? ?* (1.0f / (1 << 15));
> ? for (i = 0; i < 7; i++)
> ? ? ? isf_q[i + 9] ?= dico2_isf[ind[1]][i] ? ? ?* (1.0f / (1 << 15));
> ? for (i = 0; i < 5; i++)
> ? ? ? isf_q[i] ? ? += dico21_isf_36b[ind[2]][i] * (1.0f / (1 << 15));
> ? for (i = 0; i < 4; i++)
> ? ? ? isf_q[i + 5] += dico22_isf_36b[ind[3]][i] * (1.0f / (1 << 15));
> ? for (i = 0; i < 7; i++)
> ? ? ? isf_q[i + 9] += dico23_isf_36b[ind[4]][i] * (1.0f / (1 << 15));
>
> same below
>

Ok, I've kept the blank lines though

>> +static void decode_pitch_lag_low(int *lag_int, int *lag_frac, int pitch_index,
>> + ? ? ? ? ? ? ? ? ? ? ? ?uint8_t *base_lag_int, int subframe, enum Mode mode)
>
> Indentation is off.
>

Fixed

>> +static void decode_4p_track(int *out, int code, int m, int off)
>> +{ ? ///code: 4m bits
>
> This looks weird, the { is usually left alone.
>

I want that these comments appear in doxygen for each function. So, I
moved them to the end of the above lines (except the first).

>> + ? ?switch (BIT_STR(code, 4*m - 2, 2)) /* case ID (2 bits) */
>> + ? ?{
>> + ? ? ? ?case 0: /* 0 pulses in A, 4 pulses in B or vice versa */
>
> switch ( ?) {
> Don't indent the case, same below.
>

Fixed

>> +/**
>> + * Decode the algebraic codebook index to pulse positions and signs,
>> + * then construct the algebraic codebook vector
>> + *
>> + * @param[out] fixed_vector ? ? ? ?Buffer for the fixed codebook excitation
>> + * @param[in] pulse_hi ? ? ? ? ? ? MSBs part of the pulse index array (higher modes only)
>> + * @param[in] pulse_lo ? ? ? ? ? ? LSBs part of the pulse index array
>> + * @param[in] mode ? ? ? ? ? ? ? ? Mode of the current frame
>> + */
>
> Here and in other places, the variable names look slightly nicer aligned.
>

Fixed everywhere

>> + ? ?double p_ener = (double) ff_dot_productf(p_vector, p_vector,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? AMRWB_SFR_SIZE) * p_gain * p_gain;
>> + ? ?double f_ener = (double) ff_dot_productf(f_vector, f_vector,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? AMRWB_SFR_SIZE) * f_gain * f_gain;
>
> Indentation is off.
>

Fixed

>> +AVCodec amrwb_decoder =
>
> AVCodec amrwb_decoder = {
>

Fixed

-- 
Marcelo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: amrwb.patch
Type: application/octet-stream
Size: 153155 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100920/872d28d9/attachment.obj>