[FFmpeg-devel] [PATCH] MVI demuxer / Motion Pixels decoder

Michael Niedermayer michaelni
Fri Jul 4 01:25:45 CEST 2008


On Thu, Jul 03, 2008 at 11:40:16PM +0200, Gregory Montoir wrote:
> Michael Niedermayer wrote:
>> On Wed, Jul 02, 2008 at 07:04:50PM +0200, Gregory Montoir wrote:
[...]
>> [...]
>>> +
>>> +static void mp_invquant(MotionPixelsContext *mp, int offset, const 
>>> DeltaAcc *d)
>>> +{
>>> +    int r, g, b, color;
>>> +
>>> +    mp_iquant(d->q2, d->q1, d->q0, &r, &g, &b);
>>> +    r = av_clip(r, 0, 31);
>>> +    g = av_clip(g, 0, 31);
>>> +    b = av_clip(b, 0, 31);
>>> +    color = (r << 10) | (g << 5) | b;
>>> +    *(uint16_t *)&mp->frame.data[0][(offset / mp->avctx->width) * 
>>> mp->frame.linesize[0] + (offset % mp->avctx->width) * 2] = color;
>>> +}
>> and that converts back
>> I wonder if it would be possible to skip this back and forth convertion?
>
> I tested it this way : made the codec output to PIX_FMT_YUV444, and 
> directly converted the pixels from RGB15 to "YUV" in read_changes_map 
> (using the inverse coefs of mp_yuv_to_rgb). The decode functions then only 
> have to apply the gradient deltas and rescale uv from -31..31 to 0..255. 
> But this leads to color artifacts in some areas (probably because of the 
> rounding errors in the inverse coefs being used).
>
> Do you have another idea on how to skip this ?

no, its a pitty that the codec is designed so that this convertion is
required


[...]
> +static av_cold int mp_decode_init(AVCodecContext *avctx)
> +{
> +    MotionPixelsContext *mp = avctx->priv_data;
> +
> +    if (!mp_rgb_yuv_table_init) {
> +        mp_rgb_yuv_table_init = 1;
> +        mp_build_rgb_yuv_table(mp_rgb_yuv_table);
> +    }
> +    mp->avctx = avctx;

> +    mp->frame.data[0] = NULL;

should not be needed


[...]
> +static void mp_get_code(MotionPixelsContext *mp, GetBitContext *gb, int size, int code)
> +{
> +    while (get_bits1(gb)) {
> +        ++size;
> +        code <<= 1;
> +        mp_get_code(mp, gb, size, code + 1);
> +    }
> +    mp->codes[mp->current_codes_count  ].code = code;
> +    mp->codes[mp->current_codes_count++].size = size;
> +}

exploitable
sorry ive missed that when i simplified it


[...]
> +static YuvPixel mp_get_yuv_from_rgb(MotionPixelsContext *mp, int y, int x)

i think x, y is more intuitive than y, x


[...]
> +static void mp_decode_line(MotionPixelsContext *mp, GetBitContext *gb, int y)
> +{
> +    YuvPixel p;
> +    const int y0 = y * mp->avctx->width;
> +    int w, i, x = 0, xp = -1;
> +
> +    p = mp->vpt[y];
> +    if (mp->changes_map[y0 + x] == 0) {
> +        memset(mp->gradient_scale, 1, sizeof(mp->gradient_scale));
> +        ++x;
> +    }

> +    if ((y & 3) == 0) {
> +        while (x < mp->avctx->width) {
> +            w = mp->changes_map[y0 + x];
> +            if (w != 0) {
> +                if (mp->changes_map[y0 + x + mp->avctx->width] < w ||
> +                    mp->changes_map[y0 + x + mp->avctx->width * 2] < w ||
> +                    mp->changes_map[y0 + x + mp->avctx->width * 3] < w) {
> +                    for (i = (x + 3) & ~3; i < x + w; i += 4) {
> +                        mp->hpt[((y / 4) * mp->avctx->width + i) / 4] = mp_get_yuv_from_rgb(mp, y, i);
> +                    }
> +                }
> +                x += w;
> +                xp = x - 1;
> +            } else {
> +                if (xp != -1) {
> +                    memset(mp->gradient_scale, 1, sizeof(mp->gradient_scale));
> +                    p = mp_get_yuv_from_rgb(mp, y, xp);
> +                    xp = -1;
> +                }
> +                p.y += mp_gradient(mp, 0, mp_get_vlc(mp, gb));
> +                if ((x & 3) == 0) {
> +                    p.v += mp_gradient(mp, 1, mp_get_vlc(mp, gb));
> +                    p.u += mp_gradient(mp, 2, mp_get_vlc(mp, gb));
> +                    mp->hpt[((y / 4) * mp->avctx->width + x) / 4] = p;
> +                }
> +                mp_set_rgb_from_yuv(mp, y, x, &p);
> +                ++x;
> +            }
> +        }
> +    } else {
> +        while (x < mp->avctx->width) {
> +            w = mp->changes_map[y0 + x];
> +            if (w != 0) {
> +                x += w;
> +                xp = x - 1;
> +            } else {
> +                if (xp != -1) {
> +                    memset(mp->gradient_scale, 1, sizeof(mp->gradient_scale));
> +                    p = mp_get_yuv_from_rgb(mp, y, xp);
> +                    xp = -1;
> +                }
> +                if ((x & 3) == 0) {
> +                    p.u = mp->hpt[((y / 4) * mp->avctx->width + x) / 4].u;
> +                    p.v = mp->hpt[((y / 4) * mp->avctx->width + x) / 4].v;
> +                    /* no change to p.y */
> +                }
> +                p.y += mp_gradient(mp, 0, mp_get_vlc(mp, gb));
> +                mp_set_rgb_from_yuv(mp, y, x, &p);
> +                ++x;
> +            }
> +        }
> +    }
> +}

below is simpler (and untested ...)

while (x < mp->avctx->width) {
    w = mp->changes_map[y0 + x];
    if (w) {
        if(!(y & 3)){
            if (mp->changes_map[y0 + x + mp->avctx->width] < w ||
                mp->changes_map[y0 + x + mp->avctx->width * 2] < w ||
                mp->changes_map[y0 + x + mp->avctx->width * 3] < w) {
                for (i = (x + 3) & ~3; i < x + w; i += 4) {
                    mp->hpt[((y / 4) * mp->avctx->width + i) / 4] = mp_get_yuv_from_rgb(mp, y, i);
                }
            }
        }
        x += w;
        xp = x - 1;
    } else {
        if (xp != -1) {
            memset(mp->gradient_scale, 1, sizeof(mp->gradient_scale));
            p = mp_get_yuv_from_rgb(mp, y, xp);
            xp = -1;
        }
        p.y += mp_gradient(mp, 0, mp_get_vlc(mp, gb));
        if (!(x & 3)) {
            if(!(y & 3)){
                p.v += mp_gradient(mp, 1, mp_get_vlc(mp, gb));
                p.u += mp_gradient(mp, 2, mp_get_vlc(mp, gb));
                mp->hpt[((y / 4) * mp->avctx->width + x) / 4] = p;
            }else{
                p.u = mp->hpt[((y / 4) * mp->avctx->width + x) / 4].u;
                p.v = mp->hpt[((y / 4) * mp->avctx->width + x) / 4].v;
            }
        }
        mp_set_rgb_from_yuv(mp, y, x, &p);
        ++x;
    }
}


[...]
> +static av_cold int mp_decode_frame(AVCodecContext *avctx,

iam not sure if the av_cold is a good idea here, other codecs use it just for
init/free not for once per frame code ...
If you or someone else wants av_cold on all per frame code that probably
should be benchmarked, iam thinking here about the issues of
1. never called code can be swapped out, once per frame code cannot
2. what does gcc do if some more critical code gets inlined in such once
per frame function ...



[...]
> +    for (i = (avctx->extradata[1] & 2) != 0 ? 0 : 1; i < 2; ++i) {

!(avctx->extradata[1] & 2)


[...]
> +AVInputFormat mvi_demuxer = {
> +    "mvi",
> +    NULL_IF_CONFIG_SMALL("Motion Pixels MVI format"),
> +    sizeof(MviDemuxContext),
> +    NULL,
> +    read_header,
> +    read_packet,

> +    NULL,

the last NULL is unneeded


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080704/5f3bfb50/attachment.pgp>



More information about the ffmpeg-devel mailing list