[FFmpeg-devel] [RFC PATCH 4/4] libavcodec/j2kenc: Support for multiple layers

[Moritz Barsnick]

On Wed, Aug 19, 2020 at 17:51:02 +0530, gautamramk at gmail.com wrote:

Minor nits:

> +static void compute_rates(Jpeg2000EncoderContext* s)
> +{
> +    int i, j;
> +    int layno, compno;
> +    for (i = 0; i < s->numYtiles; i++) {
> +        for (j = 0; j < s->numXtiles; j++) {
> +            Jpeg2000Tile *tile = &s->tile[s->numXtiles * i + j];
> +            for (compno = 0; compno < s->ncomponents; compno++) {
> +                int tilew = tile->comp[compno].coord[0][1] - tile->comp[compno].coord[0][0];
> +                int tileh = tile->comp[compno].coord[1][1] - tile->comp[compno].coord[1][0];
> +                int scale = (compno?1 << s->chroma_shift[0]:1) * (compno?1 << s->chroma_shift[1]:1);
> +                for (layno = 0; layno < s->nlayers; layno++) {
> +                    if (s->layer_rates[layno] > 0.0f) {

Isn't this left hand an array of ints? Why compare an int against a
float? (And even if it were a double: Why compare a double against a
float?)

> +                        tile->layer_rates[layno] = 0.0f;

This left hand is a double. Why assign an explicit float, which the
compiler (or preprocessor?) needs to convert back to double? (I.e. just
"0.0", that's double.)

> +        for (bandno = 0; bandno < rlevel->nbands; bandno++){

Missing space before bracket.

> +            &&  rlevel->band[bandno].coord[1][0] < rlevel->band[bandno].coord[1][1]){

Missing space before bracket.

 +        Jpeg2000Band *band = rlevel->band + bandno;

Couldn't this also be "= rlevel->band[bandno]", as above?

> +                if (layno == nlayers - 1 && cblk->layers->cum_passes){

Missing space before bracket.

> +                if (cblk->layers[layno].npasses){

Missing space before bracket.

>      // lay-rlevel-comp-pos progression
> -    switch (s->prog) {
> +        switch (s->prog) {
>      case JPEG2000_PGOD_LRCP:

Stray incorrect indentation change.

> +                    if ((ret = encode_packet(s, reslevel, layno, precno, qntsty->expn + (reslevelno ? 3*reslevelno-2 : 0),
> +                                qntsty->nguardbits, packetno++, nlayers)) < 0)

Peculiar indentation, and consider shorting the first line even more.

> +                for (precno = 0; precno < reslevel->num_precincts_x * reslevel->num_precincts_y; precno++){
> +                    if ((ret = encode_packet(s, reslevel, layno, precno, qntsty->expn + (reslevelno ? 3*reslevelno-2 : 0),
> +                                qntsty->nguardbits, packetno++, nlayers)) < 0)

Ditto.

> +                    for (layno = 0; layno < nlayers; layno++){
> +                        if ((ret = encode_packet(s, reslevel, layno, precno, qntsty->expn + (reslevelno ? 3*reslevelno-2 : 0),
> +                                qntsty->nguardbits, packetno++, nlayers)) < 0)

Ditto.

> +                        for (layno = 0; layno < nlayers; layno++){
> +                            if ((ret = encode_packet(s, reslevel, layno, precno, qntsty->expn + (reslevelno ? 3*reslevelno-2 : 0),
> +                                    qntsty->nguardbits, packetno++, nlayers)) < 0)

Ditto.
Also missing space before bracket.
Also please use whitespace around operators.

(This functionality seems repetetive, perhaps this can be refactored?
Just wondering.)

> +                        for (layno = 0; layno < nlayers; layno++){
> +                            if ((ret = encode_packet(s, reslevel, layno, precno, qntsty->expn + (reslevelno ? 3*reslevelno-2 : 0),
> +                                    qntsty->nguardbits, packetno++, nlayers)) < 0)

Again. ;)

> +                                if (n == 0) {
> +                                    dr = pass->rate;
> +                                    dd = (double)pass->disto;

Redundant typecast from int32_t to double.

> +                                } else {
> +                                    dr = pass->rate - cblk->passes[n - 1].rate;
> +                                    dd = (double)pass->disto - (double)cblk->passes[n-1].disto;

Are you casting these ints to double before subtracting them? why not
afterwards (and make the typecast implicit again on assignment)?

> +                                if (!dr) {
> +                                    if (dd) {
> +                                        n = passno + 1;

You're comparing a double against 0.0, in this case it might be
appropriate to make this explicit.

> +                                dr = (int32_t)pass->rate;
> +                                dd = (double)pass->disto;
> +                            } else {
> +                                dr = (int32_t)(pass->rate) - cblk->passes[passno - 1].rate;
> +                                dd = (double)pass->disto - (double)cblk->passes[passno - 1].disto;

Ditto.

> +        double lo = min;
> +        double hi = max;
> +        double stable_thresh = 0;
> +        double good_thresh = 0;

Cosmetic: 0.0

> +            good_thresh = -1;

Cosmetic: -1.0 (but I think I see your intent of marking this as
invalid, right?).

> +        if (good_thresh >= 0)

Cosmetic: 0.0

> +            good_thresh = stable_thresh == 0 ? thresh : stable_thresh;

Cosmetic: 0.0

> +            s->layer_rates[0] = rate <= 1 ? 0:rate;

Use whitespace to separate operators (also ':').

> +            s->layer_rates[nlayers] = rate <= 1 ? 0:rate;

Ditto.

> +    if (parse_layer_rates(s)) {
> +        av_log(s, AV_LOG_WARNING, "Layer rates invalid. Shall encode with 1 layer.\n");

I suggest "Encoding with" instead of "Shall encode with". Not important
though.

> +    { "layer_rates",   "Layer Rates",       OFFSET(lr_str),        AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, VE },

I'm not happy with the capitalization, but that's what the rest uses,
so *sigh*.

Cheers,
Moritz