[FFmpeg-devel] [PATCH] Mobotix .mxg demixer and MxPEG decoder basic implementation

Reimar Döffinger Reimar.Doeffinger
Sun Aug 8 14:47:16 CEST 2010


On Sun, Aug 08, 2010 at 01:07:07PM +0400, Anatoly Nenashev wrote:
> diff -r 02ffe42e2ae2 libavcodec/Makefile
> --- a/libavcodec/Makefile	Sun Aug 01 14:52:28 2010 +0400
> +++ b/libavcodec/Makefile	Sun Aug 08 12:56:11 2010 +0400
> @@ -255,6 +255,7 @@
>  OBJS-$(CONFIG_MSRLE_DECODER)           += msrle.o msrledec.o
>  OBJS-$(CONFIG_MSVIDEO1_DECODER)        += msvideo1.o
>  OBJS-$(CONFIG_MSZH_DECODER)            += lcldec.o
> +OBJS-$(CONFIG_MXPEG_DECODER)           += mjpegdec.o mjpeg.o
>  OBJS-$(CONFIG_NELLYMOSER_DECODER)      += nellymoserdec.o nellymoser.o
>  OBJS-$(CONFIG_NELLYMOSER_ENCODER)      += nellymoserenc.o nellymoser.o
>  OBJS-$(CONFIG_NUV_DECODER)             += nuv.o rtjpeg.o
> diff -r 02ffe42e2ae2 libavcodec/allcodecs.c
> --- a/libavcodec/allcodecs.c	Sun Aug 01 14:52:28 2010 +0400
> +++ b/libavcodec/allcodecs.c	Sun Aug 08 12:56:11 2010 +0400
> @@ -148,6 +148,7 @@
>      REGISTER_DECODER (MSRLE, msrle);
>      REGISTER_DECODER (MSVIDEO1, msvideo1);
>      REGISTER_DECODER (MSZH, mszh);
> +    REGISTER_DECODER (MXPEG, mxpeg);
>      REGISTER_DECODER (NUV, nuv);
>      REGISTER_ENCDEC  (PAM, pam);
>      REGISTER_ENCDEC  (PBM, pbm);
> diff -r 02ffe42e2ae2 libavcodec/avcodec.h
> --- a/libavcodec/avcodec.h	Sun Aug 01 14:52:28 2010 +0400
> +++ b/libavcodec/avcodec.h	Sun Aug 08 12:56:11 2010 +0400
> @@ -73,6 +73,7 @@
>      CODEC_ID_MJPEG,
>      CODEC_ID_MJPEGB,
>      CODEC_ID_LJPEG,
> +    CODEC_ID_MXPEG,
>      CODEC_ID_SP5X,
>      CODEC_ID_JPEGLS,
>      CODEC_ID_MPEG4,
> diff -r 02ffe42e2ae2 libavcodec/mjpegdec.c
> --- a/libavcodec/mjpegdec.c	Sun Aug 01 14:52:28 2010 +0400
> +++ b/libavcodec/mjpegdec.c	Sun Aug 08 12:56:11 2010 +0400
> @@ -1560,3 +1560,18 @@
>      .max_lowres = 3,
>      .long_name = NULL_IF_CONFIG_SMALL("Nintendo Gamecube THP video"),
>  };
> +
> +AVCodec mxpeg_decoder = {
> +    "mxpeg",
> +    AVMEDIA_TYPE_VIDEO,
> +    CODEC_ID_MXPEG,
> +    sizeof(MJpegDecodeContext),
> +    ff_mjpeg_decode_init,
> +    NULL,
> +    ff_mjpeg_decode_end,
> +    ff_mjpeg_decode_frame,
> +    CODEC_CAP_DR1,
> +    NULL,
> +    .max_lowres = 3,
> +    .long_name = NULL_IF_CONFIG_SMALL("Mobotix MxPEG video"),
> +};

This belongs into one patch together with the actual decoder changes.

> +static int mxg_read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> +    AVStream *video_st = 0, *audio_st = 0;
> +
> +    /* video parameters will be extracted from the compressed bitstream */
> +    video_st = av_new_stream(s, VIDEO_STREAM_INDEX);
> +    if (!video_st)
> +        return AVERROR(ENOMEM);
> +    video_st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
> +    video_st->codec->codec_id = CODEC_ID_MXPEG;
> +    video_st->time_base.num = 1;
> +    video_st->time_base.den = 100;

This is wrong, use av_set_pts_info for this.

> +    audio_st->codec->sample_rate = 8000;
> +    audio_st->time_base.num = 1;
> +    audio_st->time_base.den = 1000000;

Same, but in addition time_base and sample_rate
different almost certainly means you set the
time_base wrong.

> +    mx_marker = get_le32(s->pb);
> +
> +    if (url_feof(s->pb))
> +        return AV_NOPTS_VALUE;

I suspect it would make sense to check for EOF after
all reads, not only right in the middle.

> +    if ( (AVDISCARD_ALL == s->streams[VIDEO_STREAM_INDEX]->discard)
> +         || (MKTAG('M', 'X', 'F', 0) != mx_marker ) ){

That doesn't fit into FFmpeg style at all.
First, there are too many () and spaces, the variable
should come first and the constant later in comparisons
and comparing "discard" with == or != almost
always is wrong, it should be >, >= etc.
I see that r3d, rtsp and mov do that wrong as well,
but it's still wrong (I still have a plan to add
something beyond AVDISCARD_ALL, since currently
AVDISCARD_ALL streams still need to/are processed
to a degree to make it possible to re-enable
them e.g. when switching the audio stream).

> +    dts = get_le64(s->pb); /* get dts in useconds */
> +    dts /= 10000; /*convert dts to 0.01 seconds timebase*/

That's wrong, set the time_base correctly instead.

> +static int mxg_create_video_packet(AVFormatContext *s, int64_t start_pos,
> +                                   int is_key, int64_t dts, AVPacket *pkt)
> +{
> +    int64_t end_pos, size;
> +    int ret;
> +
> +    if (AVDISCARD_ALL == s->streams[VIDEO_STREAM_INDEX]->discard)
> +        return -1;

That doesn't make sense to me. How is it possible
to distinguish an error from a discarded video stream like that?

> +    end_pos = url_ftell(s->pb);
> +    size = end_pos - start_pos;
> +
> +    url_fseek(s->pb, -size, SEEK_CUR);

This is a really bad idea and will break with streaming,
e.g.
cat file | ffmpeg -i -

> +    if (!s->streams[AUDIO_STREAM_INDEX])
> +        return -1;

Huh? Can this actually be true?
Even then, this would be the wrong place to check it.

> +    size = get_be16(s->pb);
> +    if (size <= 14)
> +        return -1;

AVERROR_INVALIDDATA I guess.

> +        case SOI:
> +            if (VIDEO_STREAM_INDEX == stream_index) {
> +                av_log(s, AV_LOG_WARNING, "EOI missing, emulating\n");
> +                url_fseek(s->pb, -2, SEEK_CUR);
> +                ret = mxg_create_video_packet(s, start_pos, is_key,
> +                                              video_dts, pkt);
> +                break;
> +            }
> +            start_pos = url_ftell(s->pb) - 2;
> +            stream_index = VIDEO_STREAM_INDEX;
> +            continue;
[...]
> +        case EOI:
> +            if (VIDEO_STREAM_INDEX == stream_index) {
> +                ret = mxg_create_video_packet(s, start_pos, is_key,
> +                                              video_dts, pkt);
> +                break;
> +            }
> +            continue;

Splitting into individual frames really does not belong into the demuxer,
it should be done in a parser.
A demuxer should only do things like splitting audio and video,
and if available process pts, dts, metadata, keyframe flags etc.
This is doubly true for JPEG where I think there are still issues
with interlaced JPEG and this could not benefit from
any fixes to that.

> +        case COM:
> +            if (AV_NOPTS_VALUE == video_dts) {
> +                video_dts = mxg_get_video_dts(s, pkt);
> +            } else {
> +                url_fskip(s->pb, get_be16(s->pb) - 2);
> +            }
> +            continue;

Also a demuxer should not drop data unless it has to.

> +        case MXAUDIO:
> +            ret = mxg_create_audio_packet(s, pkt);
> +            break;
> +
> +        default: continue;
> +        }
> +
> +        if (ret < 0) {
> +            is_key = 0;
> +            stream_index = -1;
> +            video_dts = AV_NOPTS_VALUE;
> +            continue;

Huh? So if there is a read error (e.g. due to a scratched
disk) this will be stuck in an endless loop??

> diff -r 8c73b33be5dd libavcodec/mjpegdec.c
> --- a/libavcodec/mjpegdec.c	Sun Aug 08 12:45:20 2010 +0400
> +++ b/libavcodec/mjpegdec.c	Sun Aug 08 12:56:18 2010 +0400
> @@ -108,6 +108,11 @@
>      if (avctx->codec->id == CODEC_ID_AMV)
>          s->flipped = 1;
>  
> +    if (CODEC_ID_MXPEG == s->avctx->codec_id) {
> +        avctx->time_base.num = 1;
> +        avctx->time_base.den = 1000000;
> +    }

What? A decoder has no business fiddling with the time base.

> @@ -331,18 +336,28 @@
>              s->avctx->pix_fmt = PIX_FMT_GRAY16;
>      }
>  
> -    if(s->picture.data[0])
> -        s->avctx->release_buffer(s->avctx, &s->picture);
> +    if (CODEC_ID_MXPEG == s->avctx->codec_id) {
> +        if (s->avctx->reget_buffer(s->avctx, &s->picture) < 0) {
> +            av_log(s->avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
> +            return -1;
> +        }
> +    } else {
> +        if(s->picture.data[0])
> +            s->avctx->release_buffer(s->avctx, &s->picture);
>  
> -    s->picture.reference= 0;
> -    if(s->avctx->get_buffer(s->avctx, &s->picture) < 0){
> -        av_log(s->avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> -        return -1;
> +        s->picture.reference= 0;
> +        if(s->avctx->get_buffer(s->avctx, &s->picture) < 0){
> +            av_log(s->avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +            return -1;
> +        }
>      }
> -    s->picture.pict_type= FF_I_TYPE;
> -    s->picture.key_frame= 1;
>      s->got_picture = 1;
>  
> +    if (!s->mxctx.mxm_bitmask) {
> +        s->picture.pict_type= FF_I_TYPE;
> +        s->picture.key_frame= 1;
> +    }

Do not reindent code that is not otherwise changed, it just
makes the patch hard to read.

> @@ -789,6 +804,14 @@
>  
>      for(mb_y = 0; mb_y < s->mb_height; mb_y++) {
>          for(mb_x = 0; mb_x < s->mb_width; mb_x++) {
> +            if ((CODEC_ID_MXPEG == s->avctx->codec_id) &&
> +                 !s->picture.key_frame) {
> +                int mb_index = mb_y * s->mb_width + mb_x;
> +                int mb_selected = (s->mxctx.mxm_bitmask[mb_index >> 3]) & (0x80 >> (mb_index & 7));

Needlessly many ()
Also
> +                int mb_selected = (s->mxctx.mxm_bitmask[mb_index >> 3] << (mb_index & 7)) 0x80;
is likely to be faster.

> +        int mb_width, mb_height, bitmask_size;
> +
> +        mb_width  = AV_RL16(&buf[4]);
> +        mb_height = AV_RL16(&buf[6]);
> +
> +        bitmask_size = ((mb_width * mb_height) + 7) >> 3;

This lacks a check against an integer overflow.

> @@ -1453,6 +1519,10 @@
>                              picture->quality*= FF_QP2LAMBDA;
>                          }
>  
> +                        if (CODEC_ID_MXPEG == s->avctx->codec_id) {
> +                            picture->pts = s->mxctx.current_timestamp;
> +                        }

I doubt this is correct, there should be no need to set this
for codecs where the demuxer sets PTS and there is no
reordering.



More information about the ffmpeg-devel mailing list