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

Gregory Montoir cyx
Tue Jul 1 22:55:04 CEST 2008


Michael Niedermayer wrote:
> [...]
>> +typedef struct MotionPixelsContext {
>> +    AVCodecContext *avctx;
>> +    AVFrame frame;
> 
>> +    DeltaAcc *quant_table;
> 
> as that table is constant it should be static const and not duplicated for
> each instance

Yes, that should be static ; but not sure about the 'const' as it is 
currently (the table is quite big - 32k*3 -, defining in the code all 
the init values doesn't sound too good to me). Maybe you have an idea to 
reduce it ?

so, changed to static only for the moment.


> [...]
>> +static void mp_read_changes_map1(MotionPixelsContext *mp, GetBitContext *gb, int count, int bits_len)
>> +{
>> +    int offset, w, h;
>> +
>> +    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;
>> +        while (h--) {
>> +            mp->changes_map[offset] = w;
>> +            offset += mp->avctx->width;
>> +        }
>> +    }
>> +}
> 
> This code is exploitable

added missing clippings/checks.


>> +
>> +static void mp_read_changes_map2(MotionPixelsContext *mp, GetBitContext *gb, int count, int bits_len)
>> +{
>> +    int offset, w, h, color, i;
>> +    uint16_t *pixels;
>> +
>> +    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;
>> +        color  = get_bits(gb, 15);
>> +        while (h--) {
>> +            mp->changes_map[offset] = w;
>> +            pixels = (uint16_t *)&mp->frame.data[0][(offset / mp->avctx->width) * mp->frame.linesize[0] + (offset % mp->avctx->width) * 2];
>> +            for (i = 0; i < w; ++i)
>> +                pixels[i] = color;
>> +            offset += mp->avctx->width;
>> +        }
>> +    }
>> +}
> 
> this one too, besides the 2 maybe could be merged

merged.


>> +
>> +static void mp_add_code(MotionPixelsContext *mp, int code, int size)
>> +{
>> +    mp->codes[mp->current_codes_count].code = code;
>> +    mp->codes[mp->current_codes_count].size = size;
>> +    ++mp->current_codes_count;
>> +}
>> +
>> +static void mp_get_code(MotionPixelsContext *mp, GetBitContext *gb, int b1, int code_size, int code)
>> +{
>> +    int b0;
>> +
>> +    while (b1 != 0) {
>> +        ++code_size;
>> +        code <<= 1;
>> +        b0 = get_bits1(gb);
>> +        if (b0 == 0)
>> +            mp_add_code(mp, code + 1, code_size);
>> +        else
>> +            mp_get_code(mp, gb, b0, code_size, code + 1);
>> +        b1 = get_bits1(gb);
>> +    }
>> +    mp_add_code(mp, code, code_size);
>> +}
> 
> I think the following does the same (and is simpler)
> 
> static void mp_get_code(MotionPixelsContext *mp, GetBitContext *gb, int code_size, int code)
> {
>     while(get_bits1(gb)){
>         ++code_size;
>         code <<= 1;
>         mp_get_code(mp, gb, code_size, code + 1);
>     }
>     mp->codes[mp->current_codes_count  ].code = code_code;
>     mp->codes[mp->current_codes_count++].size = code_size;
> }

indeed, changed.

> 
> [...]
>> +static int mp_gradient(MotionPixelsContext *mp, int level, int i)
>> +{
>> +    int delta;
>> +
>> +    delta = (mp->gradient_level & level) != 0 ? (i - 7) * 2 : (i - 7);
>> +    mp->gradient_level = (i == 0 || i == 14) ? (mp->gradient_level | level) : (mp->gradient_level & ~level);
>> +    return delta;
>> +}
> 
> Following is IMHO more readable (the function & variable names likely can
> be improved further)
> 
> static int mp_gradient(MotionPixelsContext *mp, int component, int v)
> {
>     int delta= (v - 7) * mp->gradient_scale[component];
>     mp->gradient_scale[component]= (v == 0 || v == 14) ? 2 : 1;
>     return delta;
> }

indeed, changed too.


> [...]
>> +                if ((x & 3) == 0) {
>> +                    d.q2 += mp_gradient(mp, 1, mp_get_vlc(mp, gb));
>> +                    d.q1 += mp_gradient(mp, 2, mp_get_vlc(mp, gb));
>> +                    d.q0 += mp_gradient(mp, 4, mp_get_vlc(mp, gb));
>> +                    mp->hdt[(y >> 2) * mp->avctx->width + x] = d;
>> +                } else {
>> +                    d.q2 += mp_gradient(mp, 1, mp_get_vlc(mp, gb));
>> +                }
> 
> the first line can be factored out of the if/else

Yes, did the same with the other occurence.


> [...]
>> +    for (y = 0; y < mp->avctx->height; y += 2) {
>> +        mp_decode_line(mp, gb, y);
>> +    }
>> +    for (y = 1; y < mp->avctx->height; y += 2) {
>> +        mp_decode_line(mp, gb, y);
>> +    }
> 
> for(y0=0; y0<2; y0++)
>     for(y=y0; ...

done.

> 
> [...]
>> +    mp->codes_count = get_bits(&gb, 4);
>> +    if (mp->codes_count == 0)
>> +        goto end;
>> +
>> +	    if (mp->changes_map[0] == 0) {
> 
> tabs are forbidden in ffmpeg svn

done.


> [...]
>> +static av_cold int mp_decode_end(AVCodecContext *avctx)
>> +{
>> +    MotionPixelsContext *mp = avctx->priv_data;
>> +
>> +    av_free(mp->quant_table);
>> +    av_free(mp->changes_map);
>> +    av_free(mp->vdt);
>> +    av_free(mp->hdt);
> 
> please use av_freep()

done.


> [...]
>> Index: libavcodec/bitstream.h
>> ===================================================================
>> --- libavcodec/bitstream.h	(r?vision 14030)
>> +++ libavcodec/bitstream.h	(copie de travail)
> 
> maybe it would be better to bswap32 the input into a temporary buffer
> iam not sure, i need to think about this more.
> Is this faster than a bswap32 and a temp buffer?

good idea.
using read_time() in mp_decode_frame gives (on a pentium m 1.7) :

// without bswap32

mp_decode_frame frame 0 buf_size 12450 read_time_diff 7629288
mp_decode_frame frame 1 buf_size 11073 read_time_diff 7312308
mp_decode_frame frame 2 buf_size 16083 read_time_diff 9276496
mp_decode_frame frame 3 buf_size 17760 read_time_diff 9964689
mp_decode_frame frame 4 buf_size 19134 read_time_diff 10460068
mp_decode_frame frame 5 buf_size 22608 read_time_diff 10403061
mp_decode_frame frame 6 buf_size 22543 read_time_diff 10257956
mp_decode_frame frame 7 buf_size 22563 read_time_diff 10297344
mp_decode_frame frame 8 buf_size 23597 read_time_diff 10942003
mp_decode_frame frame 9 buf_size 22142 read_time_diff 11048147
mp_decode_frame frame 10 buf_size 19539 read_time_diff 9871354
mp_decode_frame frame 11 buf_size 18573 read_time_diff 9215151
mp_decode_frame frame 12 buf_size 24697 read_time_diff 11025281
mp_decode_frame frame 13 buf_size 22667 read_time_diff 10949507
mp_decode_frame frame 14 buf_size 22610 read_time_diff 10095744
mp_decode_frame frame 15 buf_size 22828 read_time_diff 10267899
mp_decode_frame frame 16 buf_size 24220 read_time_diff 11974254
mp_decode_frame frame 17 buf_size 22011 read_time_diff 9830328
mp_decode_frame frame 18 buf_size 22542 read_time_diff 11350735

// with bswap32+temp buffer

mp_decode_frame frame 0 buf_size 12450 read_time_diff 6982294
mp_decode_frame frame 1 buf_size 11073 read_time_diff 6718341
mp_decode_frame frame 2 buf_size 16083 read_time_diff 8469052
mp_decode_frame frame 3 buf_size 17760 read_time_diff 9178506
mp_decode_frame frame 4 buf_size 19134 read_time_diff 9600160
mp_decode_frame frame 5 buf_size 22608 read_time_diff 9771795
mp_decode_frame frame 6 buf_size 22543 read_time_diff 9303674
mp_decode_frame frame 7 buf_size 22563 read_time_diff 9401548
mp_decode_frame frame 8 buf_size 23597 read_time_diff 10213605
mp_decode_frame frame 9 buf_size 22142 read_time_diff 10772057
mp_decode_frame frame 10 buf_size 19539 read_time_diff 9086869
mp_decode_frame frame 11 buf_size 18573 read_time_diff 8446915
mp_decode_frame frame 12 buf_size 24697 read_time_diff 10059835
mp_decode_frame frame 13 buf_size 22667 read_time_diff 9180342
mp_decode_frame frame 14 buf_size 22610 read_time_diff 9268893
mp_decode_frame frame 15 buf_size 22828 read_time_diff 9403484
mp_decode_frame frame 16 buf_size 24220 read_time_diff 10191136
mp_decode_frame frame 17 buf_size 22011 read_time_diff 9065103
mp_decode_frame frame 18 buf_size 22542 read_time_diff 10581791

so, as it's faster and less ugly code wise, I changed the code to bswap 
the bitstream.



> 
> [...]
> 
>> +    uint32_t msecs_per_frame;
> 
> unused

actually changed to use it, removing the 1/15 contant.


>> +    uint16_t video_frame_w, video_frame_h;
>> +    uint16_t audio_freq;
>> +    uint32_t audio_data_size;
>> +    uint32_t player_version;
>> +} MviFileHeader;
>> +
>> +typedef struct MviDemuxContext {
>> +    MviFileHeader hdr;
>> +    unsigned int (*get_int)(ByteIOContext *);
>> +    uint64_t audio_size_counter;
>> +    uint64_t audio_frame_size;
>> +    int audio_size_left;
>> +    int video_frame_size;
>> +    int audio_stream_index;
>> +    int video_stream_index;
>> +    int current_frame_counter;
>> +} MviDemuxContext;
>> +
>> +static int mvi_read_file_header(AVFormatContext *s, MviFileHeader *hdr) {
> 
> static functions in a file named mvi do not benefit from mvi_ in their name.

changed.


> [...]
>> +static int mvi_read_header(AVFormatContext *s, AVFormatParameters *ap)
>> +{
>> +    int ret;
>> +    MviDemuxContext *mvi = s->priv_data;
>> +    AVStream *st;
>> +
>> +    if ((ret = mvi_read_file_header(s, &mvi->hdr)) < 0)
>> +        return ret;
>> +
>> +    mvi->get_int = (mvi->hdr.video_frame_w * mvi->hdr.video_frame_h < (1 << 16)) ? get_le16 : get_le24;
>> +
>> +    mvi->audio_size_counter = 0;
> 
>> +    mvi->audio_frame_size = ((uint64_t)mvi->hdr.audio_data_size << MVI_FRAC_BITS) / mvi->hdr.frames_count;
> 
> div by zero bug with "damaged" input

added check.


>> +    mvi->audio_size_left = mvi->hdr.audio_data_size;
>> +
>> +    st = av_new_stream(s, 0);
>> +    if (!st)
>> +        return AVERROR(ENOMEM);
>> +
>> +    av_set_pts_info(st, 64, 1, mvi->hdr.audio_freq);
> 
>> +    mvi->audio_stream_index    = st->index;
> 
> is guranteed to be 0 so this isnt needed, use a #define or enum if you
> dont want a litteral 0

added define, removed var.


>> +    st->codec->codec_type      = CODEC_TYPE_AUDIO;
>> +    st->codec->codec_id        = CODEC_ID_PCM_U8;
>> +    st->codec->channels        = 1;
>> +    st->codec->sample_rate     = mvi->hdr.audio_freq;
>> +    st->codec->bits_per_sample = 8;
>> +    st->codec->bit_rate        = mvi->hdr.audio_freq * 8;
>> +
>> +    st = av_new_stream(s, 0);
>> +    if (!st)
>> +        return AVERROR(ENOMEM);
>> +
>> +    av_set_pts_info(st, 64, 1, 15);
> 
>> +    mvi->video_stream_index = st->index;
> 
> same as with audio this will be 1

same.


> 
> 
>> +    st->codec->codec_type   = CODEC_TYPE_VIDEO;
>> +    st->codec->codec_id     = CODEC_ID_MOTIONPIXELS;
> 
>> +    st->codec->width        = mvi->hdr.video_frame_w;
>> +    st->codec->height       = mvi->hdr.video_frame_h;
> 
> these (and others) can be read directly into width/height
> no need for hdr.video_frame*

done.


>> +    st->codec->pix_fmt      = PIX_FMT_RGB555;
>> +
>> +    return 0;
>> +}
>> +
>> +static int mvi_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) {
>> +            /* first audio frame */
>> +            mvi->audio_size_counter = (mvi->hdr.audio_freq * 830 / mvi->audio_frame_size) * mvi->audio_frame_size;
>> +            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;
>> +        }
>> +        pkt->stream_index = mvi->audio_stream_index;
>> +        pkt->pts = mvi->hdr.audio_data_size - mvi->audio_size_left;
>> +        if ((ret = av_get_packet(pb, pkt, count)) < 0)
>> +            return ret;
>> +        mvi->audio_size_left -= count;
>> +        mvi->audio_size_counter -= count << MVI_FRAC_BITS;
>> +    } else {
>> +        if (av_new_packet(pkt, 2 + mvi->video_frame_size))
>> +            return AVERROR(ENOMEM);
>> +        pkt->stream_index = mvi->video_stream_index;
> 
>> +        pkt->pts = mvi->current_frame_counter++;
> 
> libavformat should be able to count on its own. So if you have no real
> timestamps then setting this shouldnt make a difference. The same applies
> to audio.

changed.


>> +        pkt->data[0] = mvi->hdr.type;
>> +        pkt->data[1] = mvi->hdr.flags;
> 
> These look like they belong more in extradata than each frame

yes, changed it too.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ffmpeg-mvi-2.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080701/19cedc05/attachment.txt>



More information about the ffmpeg-devel mailing list