On Wed, Aug 19, 2020 at 17:51:02 +0530, gautamr...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".