diff mbox

[FFmpeg-devel] lavc: Add DICOM decoder

Message ID 213548c7-6318-8e5d-bd5a-7a8a14675649@iitk.ac.in
State New
Headers show

Commit Message

Shivam Goyal Aug. 24, 2019, 9:50 p.m. UTC
The patch contains DICOM decoder. I have improved the code as suggested.

Please, push it as soon as possible, as i have to share the commit in 
GSoC final evaluation. if anything remain i would add it after that.


Thank you,

Shivam Goyal

Comments

Paul B Mahol Aug. 25, 2019, 8:17 a.m. UTC | #1
On Sat, Aug 24, 2019 at 11:53 PM Shivam <shivgo@iitk.ac.in> wrote:

> The patch contains DICOM decoder. I have improved the code as suggested.
>
> Please, push it as soon as possible, as i have to share the commit in
> GSoC final evaluation. if anything remain i would add it after that.
>

You do not share ffmpeg commit into final evaluation but your work
repository.


>
> Thank you,
>
> Shivam Goyal
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Shivam Goyal Aug. 25, 2019, 9:31 a.m. UTC | #2
On 8/25/19 1:47 PM, Paul B Mahol wrote:
> On Sat, Aug 24, 2019 at 11:53 PM Shivam <shivgo@iitk.ac.in> wrote:
>
>> The patch contains DICOM decoder. I have improved the code as suggested.
>>
>> Please, push it as soon as possible, as i have to share the commit in
>> GSoC final evaluation. if anything remain i would add it after that.
>>
> You do not share ffmpeg commit into final evaluation but your work
> repository.

Okay

Thank you,

Shivam Goyal

>
>
>> Thank you,
>>
>> Shivam Goyal
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Moritz Barsnick Aug. 26, 2019, 8:34 p.m. UTC | #3
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
Shivam Goyal Aug. 27, 2019, 6:59 p.m. UTC | #4
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
diff mbox

Patch

From 29f25f6650822ed8de8e51fa0510cf4ac91288aa Mon Sep 17 00:00:00 2001
From: Shivam Goyal <1998.goyal.shivam@gmail.com>
Date: Sun, 25 Aug 2019 03:12:56 +0530
Subject: [PATCH] lavc: Add DICOM decoder

---
 Changelog               |   1 +
 libavcodec/Makefile     |   1 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/avcodec.h    |   1 +
 libavcodec/codec_desc.c |   7 ++
 libavcodec/dicom.c      | 187 ++++++++++++++++++++++++++++++++++++++++
 libavcodec/version.h    |   2 +-
 7 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 libavcodec/dicom.c

diff --git a/Changelog b/Changelog
index 52096eed0e..5f419880f1 100644
--- a/Changelog
+++ b/Changelog
@@ -5,6 +5,7 @@  version <next>:
 - v360 filter
 - Intel QSV-accelerated MJPEG decoding
 - Intel QSV-accelerated VP9 decoding
+- DICOM decoder
 
 version 4.2:
 - tpad filter
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index e49188357b..799da8aef7 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -263,6 +263,7 @@  OBJS-$(CONFIG_DCA_DECODER)             += dcadec.o dca.o dcadata.o dcahuff.o \
 OBJS-$(CONFIG_DCA_ENCODER)             += dcaenc.o dca.o dcadata.o dcahuff.o \
                                           dcaadpcm.o
 OBJS-$(CONFIG_DDS_DECODER)             += dds.o
+OBJS-$(CONFIG_DICOM_DECODER)           += dicom.o
 OBJS-$(CONFIG_DIRAC_DECODER)           += diracdec.o dirac.o diracdsp.o diractab.o \
                                           dirac_arith.o dirac_dwt.o dirac_vlc.o
 OBJS-$(CONFIG_DFA_DECODER)             += dfa.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index 22985325e0..02a1afa7e8 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -83,6 +83,7 @@  extern AVCodec ff_cscd_decoder;
 extern AVCodec ff_cyuv_decoder;
 extern AVCodec ff_dds_decoder;
 extern AVCodec ff_dfa_decoder;
+extern AVCodec ff_dicom_decoder;
 extern AVCodec ff_dirac_decoder;
 extern AVCodec ff_dnxhd_encoder;
 extern AVCodec ff_dnxhd_decoder;
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index d234271c5b..756e168c75 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -410,6 +410,7 @@  enum AVCodecID {
     AV_CODEC_ID_SCREENPRESSO,
     AV_CODEC_ID_RSCC,
     AV_CODEC_ID_AVS2,
+    AV_CODEC_ID_DICOM,
 
     AV_CODEC_ID_Y41P = 0x8000,
     AV_CODEC_ID_AVRP,
diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
index 4d033c20ff..ae9abdb2ba 100644
--- a/libavcodec/codec_desc.c
+++ b/libavcodec/codec_desc.c
@@ -1403,6 +1403,13 @@  static const AVCodecDescriptor codec_descriptors[] = {
         .long_name = NULL_IF_CONFIG_SMALL("AVS2-P2/IEEE1857.4"),
         .props     = AV_CODEC_PROP_LOSSY,
     },
+    {
+        .id        = AV_CODEC_ID_DICOM,
+        .type      = AVMEDIA_TYPE_VIDEO,
+        .name      = "dicom",
+        .long_name = NULL_IF_CONFIG_SMALL("DICOM (Digital Imaging and Communications in Medicine)"),
+        .props     = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_LOSSLESS,
+    },
     {
         .id        = AV_CODEC_ID_Y41P,
         .type      = AVMEDIA_TYPE_VIDEO,
diff --git a/libavcodec/dicom.c b/libavcodec/dicom.c
new file mode 100644
index 0000000000..7ba9107691
--- /dev/null
+++ b/libavcodec/dicom.c
@@ -0,0 +1,187 @@ 
+/*
+ * DICOM decoder
+ * Copyright (c) 2019 Shivam Goyal
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <inttypes.h>
+#include "libavutil/dict.h"
+#include "libavformat/dicom.h"
+#include "avcodec.h"
+#include "bytestream.h"
+#include "internal.h"
+
+typedef struct DICOMopts {
+    const AVClass *class;
+    int interpret; // streams extradata
+    int pixrep;
+    int pixpad;
+    int slope;
+    int intcpt;
+} DICOMopts;
+
+
+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;
+    dcmopts->interpret = bytestream_get_le32(b);
+    dcmopts->pixrep = bytestream_get_le32(b);
+    dcmopts->pixpad = bytestream_get_le32(b);
+    dcmopts->slope = bytestream_get_le32(b);
+    dcmopts->intcpt = bytestream_get_le32(b);
+    return 0;
+}
+
+static uint8_t apply_transform(int64_t val, int64_t bitmask, int pix_pad,
+                             int slope, int intercept, int w, int l, int interpret)
+{
+    int max, min;
+    uint8_t ret;
+
+    if (val == pix_pad)
+        return 0;
+    if (val > 0)
+        val = (val & bitmask);
+    val = slope * val + intercept; // Linear Transformation
+
+    max = l + w / 2 - 1;
+    min = l - w / 2;
+    if (val > max)
+        ret = 255;
+    else if (val <= min)
+        ret = 0;
+    else
+        ret = (val - min) * 255 / (max - min);
+    if (interpret == 0x01) // Monochrome1
+        ret = 255 - ret;
+    return ret;
+}
+
+// DICOM MONOCHROME1 and MONOCHROME2 decoder
+static int decode_mono( AVCodecContext *avctx,
+                        const uint8_t *buf,
+                        AVFrame *p)
+{
+    int w, l, bitsalloc, pixrep, pixpad, slope, intcpt, interpret, ret;
+    DICOMopts *dcmopts = avctx->priv_data;
+    uint8_t *out;
+    int64_t bitmask, pix;
+    uint64_t i, size;
+
+    avctx->pix_fmt = AV_PIX_FMT_GRAY8;
+    if ((ret = ff_get_buffer(avctx, p, 0)) < 0)
+        return ret;
+    p->pict_type = AV_PICTURE_TYPE_I;
+    p->key_frame = 1;
+    out = p->data[0];
+
+    w = avctx->profile;
+    l = avctx->level;
+    size = avctx->width * avctx->height;
+    bitsalloc = avctx->bits_per_raw_sample;
+    bitmask = (1 << avctx->bits_per_coded_sample) - 1;
+    interpret = dcmopts->interpret;
+    pixrep = dcmopts->pixrep;
+    pixpad = dcmopts->pixpad;
+    slope = dcmopts->slope;
+    intcpt = dcmopts->intcpt;
+
+    switch (bitsalloc) {
+    case 8:
+        for (i = 0; i < size; i++) {
+            pix = bytestream_get_byte(&buf);
+            pix = pixrep ? pix : FFABS(pix);
+            out[i] = pix;
+        }
+        break;
+    case 16:
+        for (i = 0; i < size; i++) {
+            if (pixrep)
+                pix = (int16_t)bytestream_get_le16(&buf);
+            else
+                pix = (uint16_t)bytestream_get_le16(&buf);
+            out[i] = apply_transform(pix, bitmask, pixpad, slope, intcpt, w, l, interpret);
+        }
+        break;
+    case 32:
+        for (i = 0; i < size; i++) {
+            if (pixrep)
+                pix = (int32_t)bytestream_get_le32(&buf);
+            else
+                pix = (uint32_t)bytestream_get_le32(&buf);
+            out[i] = apply_transform(pix, bitmask, pixpad, slope, intcpt, w, l, interpret);
+        }
+        break;
+    default:
+        av_log(avctx, AV_LOG_ERROR, "Bits allocated %d not supported\n", bitsalloc);
+        return AVERROR_INVALIDDATA;
+    }
+    return 0;
+}
+
+static int dicom_decode_frame(AVCodecContext *avctx,
+                            void *data, int *got_frame,
+                            AVPacket *avpkt)
+{
+    DICOMopts *dcmopts = avctx->priv_data;
+    const uint8_t *buf = avpkt->data;
+    int buf_size       = avpkt->size;
+    AVFrame *p         = data;
+    int ret, req_buf_size;
+
+    req_buf_size = avctx->width * avctx->height * avctx->bits_per_raw_sample / 8;
+    if (buf_size < req_buf_size) {
+        av_log(avctx, AV_LOG_ERROR, "Required buffer size is %d but received only %d\n", req_buf_size, buf_size);
+        return AVERROR_INVALIDDATA;
+    }
+    extract_ed(avctx);
+    switch (dcmopts->interpret) {
+    case 0x01: // MONOCHROME1
+    case 0x02: // MONOCHROME2
+        ret = decode_mono(avctx, buf, p);
+        if (ret < 0)
+            return ret;
+        break;
+    default:
+        av_log(avctx, AV_LOG_ERROR, "Provided photometric interpretation not supported\n");
+        return AVERROR_INVALIDDATA;
+    }
+    *got_frame = 1;
+    return buf_size;
+}
+
+AVCodec ff_dicom_decoder = {
+    .name           = "dicom",
+    .long_name      = NULL_IF_CONFIG_SMALL("DICOM (Digital Imaging and Communications in Medicine)"),
+    .type           = AVMEDIA_TYPE_VIDEO,
+    .priv_data_size = sizeof(DICOMopts),
+    .id             = AV_CODEC_ID_DICOM,
+    .decode         = dicom_decode_frame,
+};
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
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
-- 
2.22.0