[FFmpeg-devel] MxPEG decoder

[Diego Biurrun]

On Mon, Feb 14, 2011 at 02:46:28PM +0300, Anatoly Nenashev wrote:
> The previous version of this patch got lost in all the noise. Also it  
> needs some changes to new API. Here is the last version of MxPEG decoder  
> patch.

How much sense would it make to move the mxpeg decoding code into
a separate file?

> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -794,12 +864,15 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, i
>  
>      for(mb_y = 0; mb_y < s->mb_height; mb_y++) {
>          for(mb_x = 0; mb_x < s->mb_width; mb_x++) {
> +            const int copy_mb = use_mxm_bitmask && !get_bits1(&mxm_bitmask_gb);
> +            if (copy_mb && !use_reference) continue;

Please place the statement on the next line.

> @@ -1260,6 +1413,23 @@ found:
>  
> +static int mxpeg_check_dimensions(MXpegDecodeContext *mxctx)
> +{
> +    AVFrame *picture_ptr = &mxctx->pictures[mxctx->picture_index],
> +        *reference_ptr = &mxctx->pictures[mxctx->picture_index ^ 1];
> +    int i;
> +
> +    if (mxctx->mxm_bitmask && reference_ptr->data[0]) {
> +        for (i = 0; i < MAX_COMPONENTS; ++i) {
> +            if ( (!reference_ptr->data[i] ^ !picture_ptr->data[i]) ||

nit: Drop the space after '('.

> @@ -1267,11 +1437,14 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx,
>  
>      s->got_picture = 0; // picture from previous image can not be reused
> +    if (mxctx) mxctx->mxm_bitmask = 0;

Please break this line as well.

> @@ -1565,3 +1773,18 @@ AVCodec ff_thp_decoder = {
> +
> +AVCodec ff_mxpeg_decoder = {
> +    "mxpeg",
> +    AVMEDIA_TYPE_VIDEO,
> +    CODEC_ID_MXPEG,
> +    sizeof(MXpegDecodeContext),
> +    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"),
> +};

I think this declaration should be #ifdeffed to guard against the case
that mxpeg is disabled.

The same goes for the mxpeg-specific functions/code.  I'm not sure
how much work it would be to split this off more cleanly into a
separate file, but it might be worth investigating.

> --- a/libavcodec/mjpegdec.h
> +++ b/libavcodec/mjpegdec.h
> @@ -107,6 +107,19 @@ typedef struct MJpegDecodeContext {
>  
> +typedef struct MXpegDecodeContext {
> +    MJpegDecodeContext jpgctx;
> +    int got_sof_data; /* true if SOF data successfully parsed */
> +    uint8_t *comment_buffer; /* temporary comment buffer */
> +    uint8_t *mxm_bitmask; /* bitmask buffer */
> +    int32_t bitmask_size; /* size of bitmask */
> +    uint8_t has_complete_frame; /* true if has complete frame */
> +    uint8_t *completion_bitmask; /* completion bitmask of macroblocks */
> +    int mb_width, mb_height; /* size of picture in MB's from MXM header */
> +    AVFrame pictures[2]; /* pictures array */
> +    int picture_index; /* index of current picture, other picture is reference */
> +} MXpegDecodeContext;

IIUC this is only used in mjpegdec.c, no need to place it in the
header then.

Diego