[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