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

[Shivam]

On 8/27/19 2:04 AM, Moritz Barsnick wrote:
> 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.
I thought this may be used, I was thinking to extend the decoder. but i 
think i should make this Void for now and should change it in my next 
patch, if needed.
>
>> +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.
>
> [...]
I will correct it
>> +// 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.
>
> [...]
Would correct it too.
>> 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").

I have changed those two memory leaks i would test it with valgrind on 
000017.dcm, and would debug it as needed.

Thanks for the review

Shivam Goyal