On Sat, Jul 20, 2013 at 05:45:50PM +0200, Vitor Sessak wrote:
> On Jul 20, 2013 12:26 AM, "Diego Biurrun" <di...@biurrun.de> wrote:
> >
> > ---
> >
> > I removed parts of the changes to twinvq_data.h, as adding 4 spaces of
> > indentation to all those tables makes for a 1.5MB patch. I kid you not ..
>
> If what you didn't like about twinvq_data.h is that the huge tables are not
> indented in the beginning of the lines, this was done deliberately: the
> file was already huge enough without that, and making it even bigger for an
> arguably gain in readability looked a bad idea.
In general I think this is not a good idea, but the size of these tables
is so absurd that one might think of making an exception. Before 679K,
after 734K. Do we care about the size of our files in this regard? No
lines get added, so this file does not become harder to handle. Probably
not worth making an exception for IMO.
> > @@ -363,8 +367,7 @@ static void dequant(TwinContext *tctx, GetBitContext
> *gb, float *out,
> > enum FrameType ftype,
> > const int16_t *cb0, const int16_t *cb1, int cb_len)
> > {
> > - int pos = 0;
> > - int i, j;
> > + int i, j, pos = 0;
>
> I personally don't like merging multiple declarations with initialization.
> This goes a little beyond k&r.
I will drop it.
> > +static const uint8_t wtype_to_wsize[] = { 0, 0, 2, 2, 2, 1, 0, 1, 1 };
> > +
>
> Why?
The static table is visible outside of the function. I see no reason
to have it inside of the function. We don't do it in other places
either...
> > av_cold int twin_decode_init(AVCodecContext *avctx)
> > return AVERROR_INVALIDDATA;
> > }
> > switch (isampf) {
> > - case 44: avctx->sample_rate = 44100; break;
> > - case 22: avctx->sample_rate = 22050; break;
> > - case 11: avctx->sample_rate = 11025; break;
> > - default: avctx->sample_rate = isampf * 1000; break;
> > + case 44:
> > + avctx->sample_rate = 44100;
> > + break;
> > + case 22:
> > + avctx->sample_rate = 22050;
> > + break;
> > + case 11:
> > + avctx->sample_rate = 11025;
> > + break;
> > + default:
> > + avctx->sample_rate = isampf * 1000;
> > + break;
> > }
>
> I think readability is more important here than consistency with the rest
> of the project, and I find the original more readable.
Readability is arguable. I don't think idiosyncratic formatting is
worth the trouble.
> > -
> > static const uint8_t tab7[][35] = {
> > -
> {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,1,0,0,0},
> > -
> {0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0},
> > -
> {0,0,0,1,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0},
> > -
> {0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0},
> > -
> {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,1,0,0,0},
> > -
> {0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0},
> > -
> {0,0,0,1,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0},
> > -
> {0,0,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0,0,0},
> > -
> {0,0,1,0,1,0,0,1,0,1,0,0,1,0,1,0,0,1,0,1,0,0,1,0,1,0,0,1,0,1,0,0,1,0,1},
> > -
> {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0,0,0,0,0,0,0,0},
> > -
> {0,0,0,0,0,0,1,0,0,1,0,0,0,0,0,0,0,0,0,0,0,1,0,0,1,0,0,0,0,0,0,0,0,0,0}
> > + { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1,
> > + 0, 0, 0 },
> > + { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1,
> > + 0, 0, 0 },
> > + { 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0 },
> > + { 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0 },
> > + { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1,
> > + 0, 0, 0 },
> > + { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1,
> > + 0, 0, 0 },
> > + { 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0 },
> > + { 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0 },
> > + { 0, 0, 1, 0, 1, 0, 0, 1, 0, 1, 0, 0, 1, 0, 1, 0,
> > + 0, 1, 0, 1, 0, 0, 1, 0, 1, 0, 0, 1, 0, 1, 0, 0,
> > + 1, 0, 1 },
> > + { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0,
> > + 0, 0, 0 },
> > + { 0, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0,
> > + 0, 0, 0 }
> > };
>
> Here and in a few similar ones that follows also I think it was more
> readable before.
The lines get rather long with spaces after the comma.
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
