On Tue, Oct 01, 2013 at 01:02:38AM +0200, Maxim Polijakowski wrote:
> @Derek && @Diego: Thank you for the review!
>
> From 085cfcbfeada7736c54406573e5fe2c56ab11016 Mon Sep 17 00:00:00 2001
> From: Maxim Poliakovski <max_p...@gmx.de>
> Date: Mon, 30 Sep 2013 23:14:51 +0200
> Subject: [PATCH 1/2] atrac cosmetics: move doxygen comments to the header
> file, break long lines, improve file description and
> update copyright messages.
The Linux kernel and Git have standardized on a certain format for log
messages that we and many other projects have adopted:
tag: short title
<empty line>
An extended description of the commit that can go into any amount of
detail and span several lines or even multiple paragraphs.
For this particular case I would suggest
atrac: Move doxygen comments to the header
Also update copyright info and file description.
> --- a/libavcodec/atrac.c
> +++ b/libavcodec/atrac.c
> @@ -66,20 +62,8 @@ void ff_atrac_generate_tables(void)
> -void ff_atrac_iqmf (float *inlo, float *inhi, unsigned int nIn, float *pOut,
> float *delayBuf, float *temp)
> +void ff_atrac_iqmf (float *inlo, float *inhi, unsigned int nIn, float *pOut,
> + float *delayBuf, float *temp)
slightly unrelated cosmetics
Also, K&R does not leave a space between the function name and the opening (.
> --- a/libavcodec/atrac.h
> +++ b/libavcodec/atrac.h
> @@ -30,7 +30,22 @@
>
> -void ff_atrac_iqmf (float *inlo, float *inhi, unsigned int nIn, float *pOut,
> float *delayBuf, float *temp);
> +void ff_atrac_iqmf (float *inlo, float *inhi, unsigned int nIn, float *pOut,
> + float *delayBuf, float *temp);
same
> From aade9868608a65231a57dd36f203a647a9db906b Mon Sep 17 00:00:00 2001
> From: Maxim Poliakovski <max_p...@gmx.de>
> Date: Tue, 1 Oct 2013 00:58:41 +0200
> Subject: [PATCH 2/2] atrac3: Generalize gain compensation code Move it to
> atrac.c, so it can be reused within the upcoming
> ATRAC3+ decoder.
see above about log message formatting
> --- a/libavcodec/atrac.c
> +++ b/libavcodec/atrac.c
> @@ -62,6 +62,66 @@ void ff_atrac_generate_tables(void)
>
> +av_cold void ff_atrac_init_gain_compensation(AtracGCContext *gctx, int
> id2exp_offset,
> + int loc_scale)
Indentation is off.
> + gain_inc = gctx->gain_tab2[(i + 1 < gc_now->num_points ?
> + gc_now->levcode[i + 1] :
> gctx->id2exp_offset)
> + - gc_now->levcode[i] + 15];
imo more readable:
gain_inc = gctx->gain_tab2[(i + 1 < gc_now->num_points
? gc_now->levcode[i + 1]
: gctx->id2exp_offset)
- gc_now->levcode[i] + 15];
> + /* copy the overlapping part into delay buffer */
> + memcpy(prev, &in[num_samples], num_samples * sizeof(float));
into the delay buffer
> --- a/libavcodec/atrac.h
> +++ b/libavcodec/atrac.h
> @@ -28,6 +28,26 @@
> +typedef struct AtracGainInfo {
> + int num_points; ///< number of gain control points
> + int levcode[7]; ///< level at corresponding control point
> + int loccode[7]; ///< location of gain control points
> +} AtracGainInfo;
Why is there "code" in these names and not "point" or "points"?
> --- a/libavcodec/atrac3.c
> +++ b/libavcodec/atrac3.c
> @@ -417,90 +412,32 @@ static int decode_tonal_components(GetBitContext *gb,
> static int decode_gain_control(GetBitContext *gb, GainBlock *block,
> int num_bands)
> {
> - int i, cf, num_data;
> + int i, b;
> int *level, *loc;
>
> - for (i = 0; i <= num_bands; i++) {
> - num_data = get_bits(gb, 3);
> - gain[i].num_gain_data = num_data;
> - level = gain[i].lev_code;
> - loc = gain[i].loc_code;
> + for (b = 0; b <= num_bands; b++) {
> + gain[b].num_points = get_bits(gb, 3);
> + level = gain[b].levcode;
> + loc = gain[b].loccode;
>
> - for (cf = 0; cf < gain[i].num_gain_data; cf++) {
> - level[cf] = get_bits(gb, 4);
> - loc [cf] = get_bits(gb, 5);
> - if (cf && loc[cf] <= loc[cf - 1])
> + for (i = 0; i < gain[b].num_points; i++) {
> + level[i] = get_bits(gb, 4);
> + loc [i] = get_bits(gb, 5);
> + if (i && loc[i] <= loc[i-1])
> return AVERROR_INVALIDDATA;
> }
> }
>
> - /* Clear the unused blocks. */
> - for (; i < 4 ; i++)
> - gain[i].num_gain_data = 0;
> + /* Clear unused blocks. */
> + for (; b < 4 ; b++)
> + gain[b].num_points = 0;
Is there a reason to rename the counter variables? It seems rather
arbitrary and complicates the diff.
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
