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

[Gregory Montoir]

Michael Niedermayer wrote:
> On Wed, Jul 02, 2008 at 07:04:50PM +0200, Gregory Montoir wrote:
> 
> [...]
>> +static void mp_read_changes_map(MotionPixelsContext *mp, GetBitContext *gb, int count, int bits_len, int read_color)
>> +{
>> +    uint16_t *pixels;
>> +    int offset, w, h, color = 0, x, y, i;
>> +
>> +    while (count--) {
>> +        offset = get_bits_long(gb, mp->offset_bits_len);
>> +        w      = get_bits(gb, bits_len) + 1;
>> +        h      = get_bits(gb, bits_len) + 1;
>> +        if (read_color)
>> +            color = get_bits(gb, 15);
>> +        x = offset % mp->avctx->width;
>> +        y = offset / mp->avctx->width;
>> +        if (x + w > mp->avctx->width) {
>> +            w = mp->avctx->width - x;
>> +            if (w <= 0)
>> +                continue;
>> +        }
>> +        if (y + h > mp->avctx->height) {
>> +            h = mp->avctx->height - y;
>> +            if (h <= 0)
>> +                continue;
>> +        }
> 
> While the tests might be enough, there are a little bit too many things
> that could go wrong for my taste, following is clearer:
> 
> unsigned int offset, w, h, color = 0, x, y, i;
>     while (count--) {
>         offset = get_bits_long(gb, mp->offset_bits_len);
>         w      = get_bits(gb, bits_len) + 1;
>         h      = get_bits(gb, bits_len) + 1;
>         if (read_color)
>             color = get_bits(gb, 15);
>         x = offset % mp->avctx->width;
>         y = offset / mp->avctx->width;
>         if(y>=mp->avctx->height)
>             continue;
>         w= FFMIN(w, mp->avctx->width  - x);
>         h= FFMIN(h, mp->avctx->height - y);

done.


> [...]
>> +static void mp_read_codes_table(MotionPixelsContext *mp, GetBitContext *gb)
>> +{
>> +    int i;
>> +
>> +    if (mp->codes_count == 1)
>> +        mp->codes[0].delta = get_bits(gb, 4);
>> +    else {
>> +        mp->max_codes_bits = get_bits(gb, 4);
>> +        for (i = 0; i < mp->codes_count; ++i)
>> +            mp->codes[i].delta = get_bits(gb, 4);
>> +        mp->current_codes_count = 0;
>> +        mp_get_code(mp, gb, 0, 0);
>> +   }
>> +}
> 
> nitpicks: the int i could be movedinto the else, and the if could have {}

done.


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

> Or if that would just lead to pics with artifacts ...
> Either way the function names need to be improved.

done.


> [...]
> 
>> +                d.q2 += mp_gradient(mp, 0, mp_get_vlc(mp, gb));
>> +                if ((x & 3) == 0) {
>> +                    d.q1 += mp_gradient(mp, 1, mp_get_vlc(mp, gb));
>> +                    d.q0 += mp_gradient(mp, 2, mp_get_vlc(mp, gb));
>> +                    mp->hdt[(y >> 2) * mp->avctx->width + x] = d;
>> +                }
>> +                mp_invquant(mp, x0 + x, &d);
>> +                ++x;
>> +            }
> [...]
>> +static void mp_decode_frame_helper(MotionPixelsContext *mp, GetBitContext *gb)
>> +{
>> +    int y, y0, x0;
>> +    DeltaAcc d;
>> +
>> +    for (y = 0; y < mp->avctx->height; ++y) {
>> +        x0 = y * mp->avctx->width;
>> +        if (mp->changes_map[x0] != 0) {
>> +            d = mp_quant(mp, x0);
>> +            memset(mp->gradient_scale, 1, sizeof(mp->gradient_scale));
>> +        } else {
> 
>> +            d.q2 += mp_gradient(mp, 0, mp_get_vlc(mp, gb));
>> +            if ((y & 3) == 0) {
>> +                d.q1 += mp_gradient(mp, 1, mp_get_vlc(mp, gb));
>> +                d.q0 += mp_gradient(mp, 2, mp_get_vlc(mp, gb));
>> +            }
> 
> this looks a duplicated ...

not sure how to factorize it without duplicating the if test (in one 
case I store the pixel value).


> [...]
>> +    count1 = get_bits(&gb, 12);
>> +    count2 = get_bits(&gb, 12);
>> +
>> +    if ((avctx->extradata[1] & 2) != 0) {
>> +        mp_read_changes_map(mp, &gb, count1, 8, 0);
>> +        mp_read_changes_map(mp, &gb, count2, 4, 0);
>> +        count1 = get_bits(&gb, 12);
>> +        count2 = get_bits(&gb, 12);
>> +    }
>> +    mp_read_changes_map(mp, &gb, count1, 8, 1);
>> +    mp_read_changes_map(mp, &gb, count2, 4, 1);
> 
> 
>     for(i=...; i<2; i++){
>         int count1 = get_bits(&gb, 12);
>         int count2 = get_bits(&gb, 12);
>         mp_read_changes_map(mp, &gb, count1, 8, i);
>         mp_read_changes_map(mp, &gb, count2, 4, i);
>     }

done.


> [...]
>> +static int read_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> +    int ret, count;
>> +    MviDemuxContext *mvi = s->priv_data;
>> +    ByteIOContext *pb = s->pb;
>> +
>> +    if (mvi->video_frame_size == 0) {
>> +        mvi->video_frame_size = (mvi->get_int)(pb);
> 
>> +        if (mvi->current_frame_counter == 0) {
>> +            count = (mvi->audio_size_counter + 512) >> MVI_FRAC_BITS;
>> +        } else {
>> +            if (mvi->audio_size_left == 0)
>> +                return AVERROR(EIO);
>> +            count = (mvi->audio_size_counter + mvi->audio_frame_size + 512) >> MVI_FRAC_BITS;
>> +            if (count > mvi->audio_size_left)
>> +                count = mvi->audio_size_left;
>> +            mvi->audio_size_counter += mvi->audio_frame_size;
>> +        }
> 
> if the first audio_size_counter would be audio_frame_size bytes smaller
> then the if() should become unneeded

done. removed current_frame_counter as well.

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