Em Mon, 2 Mar 2020 15:30:39 +0100
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> escreveu:
> On Mon, 2 Mar 2020 14:57:46 +0100
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
>
> > > +#define M_N(ctxidx, idc0_m, idc0_n, idc1_m, idc1_n, \
> > > + idc2_m, idc2_n, intra_m, intra_n) \
> > > + [0][(ctxidx)] = {idc0_m, idc0_n}, \
> > > + [1][(ctxidx)] = {idc1_m, idc1_n}, \
> > > + [2][(ctxidx)] = {idc2_m, idc2_n}, \
> > > + [3][(ctxidx)] = {intra_m, intra_n}
> >
> > Hmm... I can't even imagine what a macro named "M_N" would do.
> > Please use a better name for it.
>
> Well, the meaning of those fields is explained in the spec, and the
> name itself has been chosen so it's short enough to not have lines
> exceeding 80 chars while still keeping the number of lines used for the
> cabac_table[] definition acceptable. But, I'm open to any other
> suggestion.
Well, code reviewers may not have the specs on their hands when
reviewing patches :-)
Keep 80 columns is something we desire, but not at the expense of
making the code harder to maintain or understand. Yet, I suspect
that increasing the name by a few extra bytes will still allow it to
sit at the 80 columns space[1].
[1] This macro passes 9 parameters. If each parameter consumes 4 chars,
and they're preceded by a tab, that would mean 44 columns.
Perhaps something like CABAC_ENTRY or even MN_VALUES would be better.
>
> >
> > -
> >
> > With regards to the macro itself, at least for my eyes, it looked bad,
> > from long-term maintenance PoV, to have a first argument (ctxidx) whose
> > value is just a monotonic linearly-incremented counter.
>
> It's not, we have holes in the middle, hence the explicit indexing. I
> also tried to have something as close as possible to the spec, so
> people can easily see where it comes from.
>
> >
> > I mean, the way it is, it sounds risky, as one might miss a number
> > and one entire line of the array would be filled with zeros.
>
> That's exactly why I used explicit indexing: I want specific portions
> of the table to be 0-filled :-).
Ah, OK! Implementation makes sense then.
>
> >
> > > +
> > > +/*
> > > + * Constant CABAC table.
> > > + * Built from the tables described in section '9.3.1.1 Initialisation process
> > > + * for context variables' of the H264 spec.
> > > + */
> > > +static const s8 rkvdec_h264_cabac_table[4][464][2] = {
> > > + /* Table 9-12 – Values of variables m and n for ctxIdx from 0 to 10 */
> > > + M_N(0, 20, -15, 20, -15, 20, -15, 20, -15),
> >
> > So, (maybe except if the ctxidx value has some real meaning),
> > perhaps you could, instead, switch the array order at the tables,
> > and get rid of ctxidx parameter for good, so the above code would
> > be like:
>
> I can't switch the array order since the HW expects things to be
> organized this way (that table is directly copied to a memory region
> that's passed to the HW).
>
> >
> > #define INIT_MN_PAIRS(idc0_m, idc0_n, idc1_m, idc1_n, \
> > idc2_m, idc2_n, intra_m, intra_n) \
> > { \
> > [0] = {idc0_m, idc0_n}, \
> > [1] = {idc1_m, idc1_n}, \
> > [2] = {idc2_m, idc2_n}, \
> > [3] = {intra_m, intra_n} \
> > },
> >
> > static const s8 rkvdec_h264_cabac_table[464][4][2] = {
> > /* Table 9-12 – Values of variables m and n for ctxIdx from 0 to 10 */
> > INIT_MN_PAIRS(20, -15, 20, -15, 20, -15, 20, -15),
> > ...
>
Thanks,
Mauro
