Hello,
Thanks for the review. Please see replies below.
On 26/08/2020 18:44, Moritz Barsnick wrote:
> 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.
>
Will do as a separate E-Mail
>> + 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 ("
>
Will fix all above
>> +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.
>
On Nicolas George's suggestion, I had dropped the inline keyword because
the number of inline functions used may put stress on the memory cache.
I think I should only inine the "higher" functions and not the "lower"
functions like ff_flif16_rac_get in this case. Should I?
>> +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.
Will fix
>> +#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.
Please see the section below regarding intermittent packet resuming.
>> + return ret;
>> +
>> +}
>
> Drop the empty line. ;-)
>
>> + if(!ff_flif16_rac_renorm(rc))
>
> Bracket style.
>
Will fix
>> + 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. ;-)
Please see the section below regarding intermittent packet resuming.
>> + 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
>
Will fix
>> +#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.
>
The FLIF format has no way of predetermining the end of the bitstream
without actually decompressing it first and reading all frame data. In
addition to that, frame data has been organised in a non-contiguous
manner (See also: [1]). In short, a pixel is encoded in this manner
for non interlaced images:
for i in channels
for j in rows
for k in frames
for l in columns
encode(pixel, i, j, k ,l)
Similarly, for non interlaced images, the successive "quality" blocks or
zoomlevels are coded in order.
Please see [2] for a more visual representation.
Therefore there is no way to split the image data into frames without
reading in the whole of the image data first.
Hence what we had decided to do is write the decoder in a manner that it
will be able to take in any arbitrary packet size, and be able to resume
at any point the packet ends i.e. the range decoder says that it needs
more data.
To do that, decoding related functions which call the RAC have
FLIF16DecoderContext passed to them. The struct contains the member
segment. The function then contains a switch case which uses s->segment
to restore the function control to where it left off. All stack
allocated variables that may be otherwise lost are put in the context
instead. Similarly for other subsystem contexts.
I agree that the case statements between if statements and while
statements are not very good looking, but this allows us to extract the
function logic without having to set the, or look at the state
management variables and statements. Trying to make do without the
interleaving statements would cause us to set s->segment many times,
set multiple switch statements etc.
func(FLIF16XYZContext *s, ...)
{
// Init stuff
...
switch (s->segment) {
/* Note how cases are right before RAC calls */
case 1:
RAC_GET(...);
...
case 2:
RAC_GET(...);
...
...
}
// End stuff
...
return 0;
need more data:
...
return AVERROR(EAGAIN);
}
I get your point about using do {} while (0) to break the sequence, it
does make sense now to ditch the need_more_data entirely and hence free
it from having to using a specific goto label, however it introduces an
additional level of indentation. This is how majority of the decoder
logic and transform logic has been written, so it may make the code
look a bit bulkier. Should I do it anyway?
func(FLIF16XYZContext *s, ...)
{
// Init stuff
...
do {
switch (s->segment) {
/* Note how cases are right before RAC calls */
case 1:
RAC_GET(...);
case 2:
RAC_GET(...);
...
}
// End stuff
...
return 0;
} while (0);
// EAGAIN stuff
...
return AVERROR(EAGAIN);
}
>
> 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.
Will fix. Will correct any similar errors as noted above.
> Cheers,
> Moritz
[1]: http://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/258465.html
[2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-February/257797.html
--
Thanks,
Anamitra
_______________________________________________
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".
