[FFmpeg-devel] [PATCH] MVI demuxer / Motion Pixels decoder
Michael Niedermayer
michaelni
Tue Jul 1 02:39:34 CEST 2008
On Sun, Jun 29, 2008 at 11:23:10PM +0200, Gregory Montoir wrote:
>
> Attached patch adds basic support for motion pixels .mvi files. I wanted to
> be able to transcode my old files ; so here are the changes, if there's
> interest.
>
> I also uploaded a sample (intro.mvi) to /incoming.
>
> Regards,
> Gregory
[...]
> +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
[...]
> +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
> +
> +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
> +
> +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;
}
[...]
> +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;
}
[...]
> + 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
[...]
> + 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; ...
[...]
> + 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
[...]
> +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()
[...]
> 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?
[...]
> + uint32_t msecs_per_frame;
unused
> + 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.
[...]
> +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
> + 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
> + 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
> + 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*
> + 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.
> + pkt->data[0] = mvi->hdr.type;
> + pkt->data[1] = mvi->hdr.flags;
These look like they belong more in extradata than each frame
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Old school: Use the lowest level language in which you can solve the problem
conveniently.
New school: Use the highest level language in which the latest supercomputer
can solve the problem without the user falling asleep waiting.
-------------- 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/20080701/4cce7a72/attachment.pgp>
More information about the ffmpeg-devel
mailing list