> It can, but that will hurt readability while achieving nothing.
I think it looks vastly more readable without the redundant lines of
code, but I suppose that's just me.
>> > + if (FFABS(t - tl) <= FFABS(l - tl))
>> > + dc += l;
>> > + else
>> > + dc += t;
>>
>> This seems weird; we're using the one that's farther away? Okay if
>> it's right, but maybe a comment explaining this DC prediction scheme?
>
> "M$ choosed it this way".
Yes, but you can explain how it works.
> Should I remind you who designed that codec?
It's not an issue of how badly designed the codec is; we didn't accept
this for Lagarith either, which had floating point in the reference
entropy decoder. Unless you have a good argument for why this is
okay, I don't see why we can't do the same here as for Lagarith.
If there's a relatively limited number of quality options, we could
just hardcode a table.
Regardless, even if we change nothing, there should be some sort of
comment justifying why this won't be a problem.
> It was easier to track for me.
It seems wildly inconsistent with the rest of the code in the file,
which uses return codes.
>> > + dct_put(c->imgbuf[i], 8, c->block);
>> > + out = dst[i] + mb_x * 16;
>> > + for (j = 0; j < 16; j++) {
>> > + for (k = 0; k < 8; k++)
>> > + AV_WN16A(out + k * 2, c->imgbuf[i][k + (j & ~1) * 4] *
>> > 0x101);
>> > + out += c->pic.linesize[i];
>> > + }
>>
>> Explain what's going on here?
>
> It's rather simple - the codec has two types of image blocks, one (vector
> quantisation kinda) codes 16x16 macroblocks in YUV 4:4:4, another one (DCT)
> codes 16x16 macroblock in YUV 4:2:0. Thus either chroma should be downsampled
> in one case or upsampled in the other. I picked the latter and did it here.
Do we have a DSP function for this? Also, can you comment this at the
very least?
> // I have no idea how it works.
> This seems to be some kind of vector quantisation coding all three components
> at once (but still independently). And depending on flags it might decide to
> keep filling blocks with the same values or change it from one of three
> possible sources. In two words - it's magic.
Try at the very least to comment it? Currently it's rather
impenetrable, and just because it's RE'd code doesn't mean we can't
make some attempt to explain it or at least say how it works.
Jason
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
