[FFmpeg-devel] [PATCH] lavc: Add DICOM decoder

[Moritz Barsnick]

On Sun, Aug 25, 2019 at 03:20:13 +0530, Shivam wrote:
> The patch contains DICOM decoder. I have improved the code as suggested.

Hi Shivan,
nice job.

First of all, all your samples now appear to demux and decode properly.
I didn't get a chance to look at their output, but I guess you have
that covered.

Most of my review remarks have been integrated. You did miss
some which I believe are essential:

> +static int extract_ed(AVCodecContext *avctx)
> +{
> +    DICOMopts *dcmopts =  avctx->priv_data;
> +    uint8_t *ed = avctx->extradata;
> +    int ed_size = avctx->extradata_size;
> +    const uint8_t **b = &ed;
> +
> +    dcmopts->interpret = 0x02; // Set defaults
> +    dcmopts->slope = 1;
> +    dcmopts->intcpt = 0;
> +    dcmopts->pixpad = 1 << 31;
> +    dcmopts->pixrep = 0;
> +
> +    if (ed_size < DECODER_ED_SIZE + AV_INPUT_BUFFER_PADDING_SIZE)
> +        return -1;

This return value isn't used anywhere (or ignored). That's fine if
that's intentional, but a bit confusing for review.

> +static uint8_t apply_transform(int64_t val, int64_t bitmask, int pix_pad,
> +                             int slope, int intercept, int w, int l, int interpret)

Indentation is now off in the second quoted line.

[...]
> +// DICOM MONOCHROME1 and MONOCHROME2 decoder
> +static int decode_mono( AVCodecContext *avctx,
> +                        const uint8_t *buf,
> +                        AVFrame *p)

There's still a space too much here, after "decode_mono(", and the
subsequent two lines need to adjust as well.

[...]
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index e70ebc0c70..b4b79ef63a 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
>  #include "libavutil/version.h"
>
>  #define LIBAVCODEC_VERSION_MAJOR  58
> -#define LIBAVCODEC_VERSION_MINOR  55
> +#define LIBAVCODEC_VERSION_MINOR  56
>  #define LIBAVCODEC_VERSION_MICRO 101

When bumping MINOR, you need to reset MICRO to 100. But this part
doesn't apply anymore anyway, since MICRO has changed on master since.
You may need to rebase, but this will likely also be done by whoever
pushes to master once your patch is acknowledged.

Apart from that, I still get a memory leak when decoding 000017.dcm
(with just "valgrind ./ffmpeg_g -i DICOM-samples/000017.dcm").

Cheers,
Moritz