On Sun, Aug 23, 2020 at 00:09:32 +0530, Anamitra Ghorui wrote: > v2: Fix faulty patch > v3: Fix addressed errors, Add interlaced decoding support > v4: Fix Further cosmetics, C.Bucket Transform reading errors, Atomise patch > v5: Fix faulty patch > v6: Address pointed out errors, use av_freep everywhere, further cosmetics, > redundancies.
These comments don't belong in the commit or the e-mail corresponding to the commit. So either in the "0/4" e-mail, or below the "---" below. > Test files are available here: https://0x0.st/iYs_.zip > > Co-authored-by: Anamitra Ghorui <agho...@teknik.io> > Co-authored-by: Kartik K Khullar <kartikkhullar...@gmail.com> > > Signed-off-by: Anamitra Ghorui <agho...@teknik.io> > --- Place comments here. > + if (plane == 1 || plane == 2){ Please fix the bracket style -> ") {" > + if (plane != 2) { > + prop_ranges[top].min = mind; > + prop_ranges[top++].max = maxd; > + prop_ranges[top].min = mind; > + prop_ranges[top++].max = maxd; > + } Incorrect indentation. > + for(uint8_t i = 0; i < (lookback ? MAX_PLANES : num_planes); i++) { Bracket style -> "for (" > +/* > + * All constant plane pixel setting should be illegal in theory. settings > +static inline FLIF16ColorVal ff_flif16_pixel_get_fast(FLIF16Context *s, > + FLIF16PixelData *frame, > + uint8_t plane, > uint32_t row, > + uint32_t col) > +{ > + if (s->plane_mode[plane]) { > + return ((FLIF16ColorVal *) frame->data[plane])[row * > frame->s_r[plane] + col * frame->s_c[plane]]; > + } else > + return ((FLIF16ColorVal *) frame->data[plane])[0]; > + return 0; Isn't this "return 0" dead code? > + if(bytestream2_get_bytes_left(gb) < FLIF16_RAC_MAX_RANGE_BYTES) Bracket style -> "if (" > +uint8_t ff_flif16_rac_read_bit(FLIF16RangeCoder *rc, > + uint8_t *target) > +{ > + return ff_flif16_rac_get(rc, rc->range >> 1, target); > +} If this is called often, you may want to mark it inline. > +uint32_t ff_flif16_rac_read_chance(FLIF16RangeCoder *rc, > + uint64_t b12, uint8_t *target) > +{ > + uint32_t ret = ((rc->range) * b12 + 0x800) >> 12; I don't think rc->range needs to be bracketed. > + if(!ff_flif16_rac_renorm(rc)) Bracket style. > +#define RAC_NZ_GET(rc, ctx, chance, target) > \ > + if (!ff_flif16_rac_nz_read_internal((rc), (ctx), (chance), > \ > + (uint8_t *) (target))) { > \ > + goto need_more_data; > \ > + } Functions are usually defined with a do{} while(0) wrapper. See the style in libavutil/intreadwrite.h. > + return ret; > + > +} Drop the empty line. ;-) > + if(!ff_flif16_rac_renorm(rc)) Bracket style. > + while (rc->pos > 0) { > + rc->pos--; > + rc->left >>= 1; > + rc->minabs1 = rc->have | (1 << rc->pos); > + rc->maxabs0 = rc->have | rc->left; > + > + if (rc->minabs1 > rc->amax) { > + continue; > + } else if (rc->maxabs0 >= rc->amin) { > + case 3: > + RAC_NZ_GET(rc, ctx, NZ_INT_MANT(rc->pos), &temp); > + if (temp) > + rc->have = rc->minabs1; > + temp = 0; > + } else { > + rc->have = rc->minabs1; > + } A case label in the middle of an if() (in a while() loop) is not readable to me. ;-) > + m->stack_top = 0; > + rc->segment2 = 0; > + return 0; > + > + need_more_data: > + return AVERROR(EAGAIN); I believe the label needs to be left-aligned. > + if(!m->forest[channel]->leaves) Bracket style. > + if(!rc->curr_leaf) { Ditto. > + > + end: > + rc->curr_leaf = NULL; "goto" label left alignment > + * probability model. The other (simpler) model and this model ane non *are > +#define RAC_GET(rc, ctx, val1, val2, target, type) \ > + if (!ff_flif16_rac_process((rc), (ctx), (val1), (val2), (target), > (type))) { \ > + goto need_more_data; \ > + } do{} while(0) > +#define MANIAC_GET(rc, m, prop, channel, min, max, target) \ > + { \ > + int ret = ff_flif16_maniac_read_int((rc), (m), (prop), (channel), > (min), (max), (target)); \ > + if (ret < 0) { \ > + return ret; \ > + } else if (!ret) { \ > + goto need_more_data; \ > + } \ > + } Ditto. I give up here. There are further 4000+ lines to review. What an impressive patch. Just this: > +static void transform_palette_reverse(FLIF16Context *ctx, > + FLIF16TransformContext *t_ctx, > + FLIF16PixelData *frame, > + uint32_t stride_row, > + uint32_t stride_col) > +{ > + int r, c; > + int P; ffmpeg doesn't use capitals in variable names. 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".
