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

[Gregory Montoir]

Michael Niedermayer wrote:
> On Thu, Jul 03, 2008 at 11:40:16PM +0200, Gregory Montoir wrote:
> 
> [...]
>> +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

removed.


> [...]
>> +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

added missing check.


> [...]
>> +static YuvPixel mp_get_yuv_from_rgb(MotionPixelsContext *mp, int y, int x)
> 
> i think x, y is more intuitive than y, x

changed.


> [...]
>> +static void mp_decode_line(MotionPixelsContext *mp, GetBitContext *gb, int y)
> 
> 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;
>     }
> }

ok, switched to your version (and reordered the p.u/p.v related lines 
for consistency)


> [...]
>> +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 ...

well... this wasn't intended (I used tiertexseqv.c as 'reference', but 
it seems the av_cold attributes are wrong - I'll submit patches for this 
file along with some other minor modifications)


> 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)

yes, changed.


> [...]
>> +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

removed.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ffmpeg-mvi-5.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080705/efc8c38d/attachment.txt>