On Thu, Aug 20, 2020 at 3:36 PM Moritz Barsnick <barsn...@gmx.net> wrote:
>
> 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.)
This was a part of a previous patch. I'll try working on refactoring
this separately.
>
> > + 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".
Thanks a lot for the suggestions Moritz, I'll make these changes and resubmit.
--
-------------
Gautam |
_______________________________________________
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".
