[FFmpeg-devel] a64 encoder 7th round

Bitbreaker/METALVOTZE bitbreaker
Mon Jan 26 08:33:05 CET 2009


> nitpick:
> int height = FFMIN(avctx->height,C64YRES);
> int width  = FFMIN(avctx->width ,C64XRES);

Fixed.


>> +    uint8_t *src = p->data[0];
>> +
>> +    for (blocky = 0; blocky<height; blocky += 8) {
>> +        for (blockx = 0; blockx<C64XRES; blockx += 8) {
>> +            for (y = blocky; y < blocky+8 && y<height; y++) {
>> +                for (x = blockx; x < blockx+8 && x<C64XRES; x += 2) {
>> +                    if(x<width) {
>> +                        /* build average over 2 pixels */
>> +                       luma = (src[(x + 0 + y * p->linesize[0])] +
>> +                               src[(x + 1 + y * p->linesize[0])]) / 2;
>> +                       /* write blocks as linear data now so they are suitable for elbg */
>> +                       dest[0] = luma;
> 
> the indention depth looks odd

Fixed.

>> +    static const uint8_t vals[] = { 3, 2, 1, 0, 3 };
> 
> could be replaced by (3-x)&3
> (no iam not suggesting that it should be replaced, iam just mentioning it
>  could)

I might do so. It is more or less a remain of testing and i don't think 
the order will change anymore. It just was very handy in times i was not 
sure about the final order of the colors.

>> +        if (pix > maxsteps)
>> +            pix = maxsteps;
> 
> does this if() do anything at all?
> if not, the rounding might also not be ideal, i mean
> 0..(255/maxsteps) will be 0
> but only
> 255 will be maxsteps
> that is, if 255 is the largest best_cb[]

I guess i wanted to be extra secure, (coz i am using that values for 
building an index later on), nothing else :-)

> index2= FFMIN(index1 + 1, maxindex);
[...]
 > FFMAX/FFMIN can simplify these

Made more use of FFMAX/FFMIN.

> as this seems to never be anything else it could as well be a #define
> simpifying the code ...

Did a define for that. I kept this so far, as i thought of maybe having 
an option for that once, and enable the use to choose between different 
ditherpatterns/depths. But can be inplemented easily again if needed.

> i dont think this should be av_cold but then i must admit its not clear what
> gcc exactly does with the cold attribute. Fact is other encoders dont mark
> their encode_frame cold though ...

I was too eager and accidently also added it to the encode function. 
However, i am also not sure what this does, i simply did how others did :-)

>> +    CODEC_ID_A64_MULTI,
>> +    CODEC_ID_A64_MULTI5,
> 
> What differnce is there between the 2 codecs?
> Either way i dont think it justifies 2 coedec ids.

The difference is not really big. The normal multicol mode works with a 
fixed color in the colram, thus having a range of 4 colors (3 multicol 
registers + colorram). That covers a luminance range from black up to 
light gray.
The 5col mode extends the dynamic range, by displaying the brightest and 
the darkest color with the color defined by the colorram. That means i 
need to add the colorram for each frame and therefore i am able to 
display a range from black to white (however black and white may not 
occur at once in a 8x8 block, else i scale either white down or black up 
if that happens).
I have chosen the way of two codecs, as i lacked an appropriate option 
to set it. As for the charset lifetime i now use qscale, maybe there is 
also a commandline option that suites that behaviour well? Then i can of 
course just use a single codec ID.

>> +/* dither patterns */
>> +static const uint8_t prep_dither_patterns[9][4][4] = {
> 
> comment is not doxygen compatible

Fixed.

Kindest regards,

Toby

-- 
      style)KURVE - Fotografie mit Biss.
   www.style-kurve.de - info at style-kurve.de
Tobias Bindhammer, Uhlandstrasse 8, 89278 Stra?
        		01577/1751761
-------------- next part --------------
A non-text attachment was scrubbed...
Name: a64_7.diff
Type: text/x-diff
Size: 14808 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090126/d7929589/attachment.diff>



More information about the ffmpeg-devel mailing list