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

[Gautam Ramakrishnan]

On Thu, Aug 20, 2020 at 3:36 PM Moritz Barsnick <barsnick at gmx.net> wrote:
>
> 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.)
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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

Thanks a lot for the suggestions Moritz, I'll make these changes and resubmit.

-- 
-------------
Gautam |