Re: [FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
On 8/21/19 7:27 PM, Moritz Barsnick wrote:
On Tue, Aug 20, 2019 at 20:53:43 +0530, Shivam wrote:
Please review,
(Untested, just visual code inspection.)
+- DICOM decoder and demxer
Typo -> demuxer. Also, when splitting the commits, also split the
changes to the Changelog. (Can still be one line eventually.)
+.props = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_LOSSLESS,
Possibly also AV_CODEC_PROP_INTRA_ONLY? (PNG doesn't have that either,
but other still image formats do.) Is DICOM a still image format, or
does it have multiple images and a sense of I-frames?
+static int extract_ed(AVCodecContext *avctx)
The return value is never used anywhere.
+int ed_s = avctx->extradata_size;
Feel free to name the variable with something containing "size", makes
the code somewhat easier to understand.
+static uint8_t apply_transfm(int64_t val, int64_t bitmask, int pix_pad,
^ I see no reason to save two letters in this
name. ;-)
+static int decode_mono( AVCodecContext *avctx,
+const uint8_t *buf,
+AVFrame *p)
^ spurious space
+switch (bitsalloc) {
+case 8:
ffmpeg uses the same indentation level for "case" as for "switch".
+for (i = 0; i < size; i++) {
+if (pixrep)
+pix = (int8_t)bytestream_get_byte();
+else
+pix = (uint8_t)bytestream_get_byte();
+out[i] = pix;
+}
What is this doing? Is the cast to uint8_t an implicit "abs()"?
Could it just be:
pix = pixrep ? bytestream_get_byte() :
FFABS(bytestream_get_byte());
out[i] = ...
Or in a somewhat different style:
uintXY_t val = bytestream_get_byte();
pix = pixrep ? byte : FFABS(byte);
out[i] = ...
+default:
+av_log(avctx, AV_LOG_ERROR, "Bits allocated %d not supported\n",
bitsalloc);
+return AVERROR_INVALIDDATA;
avctx->bits_per_coded_sample is a constant per file, right?
This "default" could be avoided if avctx->bits_per_coded_sample were
checked in init(), not in decode_frame().
+av_log(avctx, AV_LOG_ERROR, "Required buffer size is %d but recieved only
%d\n", req_buf_size, buf_size);
typo: received
+void* bytes;
void *bytes
+char* desc; // description (from dicom dictionary)
char *desc;
+const uint8_t *d = p->buf;
+
+if (d[DICOM_PREAMBLE_SIZE] == 'D' &&
+d[DICOM_PREAMBLE_SIZE + 1] == 'I' &&
+d[DICOM_PREAMBLE_SIZE + 2] == 'C' &&
+d[DICOM_PREAMBLE_SIZE + 3] == 'M') {
+return AVPROBE_SCORE_MAX;
Would:
if (!strncmp(p->buf, "DICM", 4)
also work? Seems much simpler to me. (Also skipping the intermediate
"d" variable.)
+if (de->bytes)
+av_free(de->bytes);
+if (de->desc)
+av_free(de->desc);
As Michael said, av_free() includes the NULL checks already.
Additionally, I believe the use of av_freep() is preferred for these
pointers. (Provokes a segfault on use after free, instead of "silently"
writing into / reading from that memory.)
+// detects transfer syntax
+static TransferSyntax get_transfer_sytax (const char *ts) {
^ typo: syntax
Could you also please not name the variable "ts", as that already has
too many other meanings. ;-) (Use e.g. "syntax".)
+static void set_dec_extradata(AVFormatContext *s, AVStream *st) {
+DICOMContext *dicom = s->priv_data;
+uint8_t *edp;
+int size = 20 + AV_INPUT_BUFFER_PADDING_SIZE;
+
+st->codecpar->extradata = (uint8_t *)av_malloc(size);
+edp = st->codecpar->extradata;
+bytestream_put_le32(>codecpar->extradata, dicom->interpret);
+bytestream_put_le32(>codecpar->extradata, dicom->pixrep);
+bytestream_put_le32(>codecpar->extradata, dicom->pixpad);
+bytestream_put_le32(>codecpar->extradata, dicom->slope);
+bytestream_put_le32(>codecpar->extradata, dicom->intcpt);
+st->codecpar->extradata = edp;
+st->codecpar->extradata_size = size;
+}
I'm not sure you're doing anything with edp here. Did you mean to use:
bytestream_put_le32(, dicom->interpret);
?
+sprintf(key, "(%04x,%04x) %s", de->GroupNumber, de->ElementNumber,
de->desc);
As Michael said, this can overflow "key". *Always* use snprintf.
+switch(de->VR) {
+case AT:
Indentation.
+static int set_multiframe_data(AVFormatContext *s, AVStream* st, DataElement
*de)
+{
+DICOMContext *dicom = s->priv_data;
+
+if (de->GroupNumber != MF_GR_NB)
+return 0;
+
+switch (de->ElementNumber) {
+case 0x1063: // frame time
+dicom->delay = conv_DS(de);
+dicom->duration = dicom->delay * dicom->nb_frames;
+break;
+}
+return 0;
+}
Always returns 0.
Is this a switch/case, so that it can be expanded in the future?
+av_log(s, AV_LOG_WARNING,"Data
Re: [FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
On Thu, Aug 22, 2019 at 00:28:28 +0530, Shivam wrote: > >> + dicom->height = *(uint16_t *)bytes; > > does this work on big endian ? > > maybe you where looking for AV_RL16() > > Big endian DICOM files are retired and no longer supported by the standard. What Michael means: What happens if you use this to decode a (little endian) DICOM file on big endian hardware? In other words: "dicom->height = *(uint16_t *)bytes" will map the bytes into the big endian int in the wrong order, resulting in an incorrect integer value. AV_RL16() (e.g.) handles this properly for you. @Michael, do you have some short instructions on how to set up a big endian system like your MIPS+qemu? Thanks. Moritz ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
Forwarded Message
Subject: Re: [FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add
DICOM decoder
Date: Thu, 22 Aug 2019 00:27:55 +0530
From: Shivam
To: Michael Niedermayer
On 8/21/19 2:00 AM, Michael Niedermayer wrote:
On Tue, Aug 20, 2019 at 08:53:43PM +0530, Shivam wrote:
Sorry, for my previous mail, i forgot to attach the patch.
The patch contains support for Dicom files. The below features are
supported
yet:-
Uncompressed DICOM files using any of the Implicit and Explicit VR
formats.
Multiframe files are also supported.
To extract the metadata or info about the procedure, I have added an
option
"-metadata." (The tags present in Dicom dictionary would be demuxed).
I have also uploaded DICOM samples here.
https://1drv.ms/u/s!AosJ9_SrP4Tsh2WoPfQAI8cSsqUs?e=ZMd5fR
Please review,
Thank you,
Shivam Goyal
Changelog | 1
libavcodec/Makefile | 1
libavcodec/allcodecs.c | 1
libavcodec/avcodec.h | 1
libavcodec/codec_desc.c | 7
libavcodec/dicom.c | 189
libavcodec/version.h | 2
libavformat/Makefile | 1
libavformat/allformats.c | 1
libavformat/dicom.h | 108 +++
libavformat/dicomdec.c | 594 ++
libavformat/dicomdict.c | 716
+++
libavformat/version.h | 2
13 files changed, 1622 insertions(+), 2 deletions(-)
d47b7ad6a9f16ce4e29a67a99800183f9056062d add_dicom.patch
From cd7ebe834278b29b0429b3f5b377154490ee159f Mon Sep 17 00:00:00 2001
From: Shivam Goyal <1998.goyal.shi...@gmail.com>
Date: Tue, 20 Aug 2019 20:03:02 +0530
Subject: [PATCH] lavc: add DICOM decoder, lavf: add DICOM demuxer
---
Changelog | 1 +
libavcodec/Makefile | 1 +
libavcodec/allcodecs.c | 1 +
libavcodec/avcodec.h | 1 +
libavcodec/codec_desc.c | 7 +
libavcodec/dicom.c | 189 +++
libavcodec/version.h | 2 +-
libavformat/Makefile | 1 +
libavformat/allformats.c | 1 +
libavformat/dicom.h | 108 ++
libavformat/dicomdec.c | 594
libavformat/dicomdict.c | 716 +++
libavformat/version.h | 2 +-
libavformat and libavcodec changes should be in seperate patches
13 files changed, 1622 insertions(+), 2 deletions(-)
create mode 100644 libavcodec/dicom.c
create mode 100644 libavformat/dicom.h
create mode 100644 libavformat/dicomdec.c
create mode 100644 libavformat/dicomdict.c
diff --git a/Changelog b/Changelog
index c1f1237770..e04c3aa5f6 100644
--- a/Changelog
+++ b/Changelog
@@ -4,6 +4,7 @@ releases are sorted from youngest to oldest.
version :
- Intel QSV-accelerated MJPEG decoding
- Intel QSV-accelerated VP9 decoding
+- DICOM decoder and demxer
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 00..eaa378d944
--- /dev/null
+++ b/libavcodec/dicom.c
@@ -0,0 +1,189 @@
+/*
+ * 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 term
Re: [FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
On Tue, Aug 20, 2019 at 20:53:43 +0530, Shivam wrote:
One more. Some more nitpicking around this. Please use valgrind with
all your samples and some demuxing and decoding command lines.
> +static int dicom_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +DICOMContext *dicom = s->priv_data;
> +int metadata = dicom->metadata, ret;
> +AVDictionary **st_md;
> +char *key, *value;
> +AVStream *st;
> +DataElement *de;
> +
> +if (s->nb_streams < 1) {
> +st = avformat_new_stream(s, NULL);
> +if (!st)
> +return AVERROR(ENOMEM);
> +st->codecpar->codec_id = AV_CODEC_ID_DICOM;
> +st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> +st->start_time = 0;
> +avpriv_set_pts_info(st, 64, 1, 1000);
> +if (dicom->window != -1)
> +st->codecpar->profile = dicom->window;
> +if (dicom->level != -1)
> +st->codecpar->level = dicom->level;
> +} else
> +st = s->streams[0];
> +
> +st_md = >metadata;
> +dicom->inheader = 0;
> +if (dicom->frame_nb > 1 && dicom->frame_nb <= dicom->nb_frames)
> +goto inside_pix_data;
> +
> +for (;;) {
> +ret = avio_feof(s->pb);
> +if (ret)
> +return AVERROR_EOF;
> +
> +de = alloc_de();
Allocation failure needs to be checked:
if (!de)
return AVERROR(ENOMEM);
> +ret = read_de_metainfo(s,de);
> +if (ret < 0)
> +goto exit;
> +
> +if (de->GroupNumber == PIXEL_GR_NB && de->ElementNumber ==
> PIXELDATA_EL_NB) {
> +dicom->frame_size = de->VL / dicom->nb_frames;
> +set_dec_extradata(s, st);
> +inside_pix_data:
> +if (av_new_packet(pkt, dicom->frame_size) < 0)
> +return AVERROR(ENOMEM);
This leaks de. Instead:
if (av_new_packet(pkt, dicom->frame_size) < 0) {
ret = AVERROR(ENOMEM);
goto exit;
}
> +pkt->pos = avio_tell(s->pb);
> +pkt->stream_index = 0;
> +pkt->size = dicom->frame_size;
> +pkt->duration = dicom->delay;
> +ret = avio_read(s->pb, pkt->data, dicom->frame_size);
> +if (ret < 0) {
> +av_packet_unref(pkt);
> +goto exit;
> +}
> +dicom->frame_nb ++;
> +return ret;
This leaks everything (i.e. de and pkt). Instead:
dicom->frame_nb ++;
av_packet_unref(pkt);
goto exit;
> +} else if (de->GroupNumber == IMAGE_GR_NB || de->GroupNumber ==
> MF_GR_NB) {
> +ret = read_de_valuefield(s, de);
> +if (ret < 0)
> +goto exit;
> +set_imagegroup_data(s, st, de);
> +set_multiframe_data(s, st, de);
> +} else {
> +if (metadata || de->VL == UNDEFINED_VL)
> +ret = read_de_valuefield(s, de);
> +else
> +ret = avio_skip(s->pb, de->VL); // skip other elements
> +if (ret < 0)
> +goto exit;
> +}
> +if (metadata) {
> +ret = dicom_dict_find_elem_info(de);
> +if (ret < 0)
> +goto end_de;
> +key = get_key_str(de);
> +value = get_val_str(de);
> +av_dict_set(st_md, key, value, AV_DICT_DONT_STRDUP_KEY |
> AV_DICT_DONT_STRDUP_VAL);
> +}
> +end_de:
> +free_de(de);
> +}
> +exit:
> +free_de(de);
> +if (ret < 0)
> +return ret;
> +return AVERROR_EOF;
> +}
And, as mentioned, "goto exit" can probably be a simple "break", but
that's up to you to verify that it still does the right thing.
Cheers,
Moritz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
On Tue, Aug 20, 2019 at 20:53:43 +0530, Shivam wrote:
> I have also uploaded DICOM samples here.
> https://1drv.ms/u/s!AosJ9_SrP4Tsh2WoPfQAI8cSsqUs?e=ZMd5fR
I found something else, with valgrind:
> +static int read_implicit_seq_item(AVFormatContext *s, DataElement *de)
> +{
> +int ret, f = -2, i = 0;
> +
> +de->bytes = av_malloc(MAX_UNDEF_LENGTH);
This return value needs to be checked!
> +for(; i < MAX_UNDEF_LENGTH; ++i) {
> +ret = avio_rl16(s->pb);
> +if (ret < 0)
> +return ret;
> +
> +if (ret == SEQ_GR_NB)
> +f = i;
> +if (ret == SEQ_ITEM_DEL_EL_NB && f == i - 1) {
> +avio_skip(s->pb, 4);
> +break;
> +}
> +bytestream_put_le16((uint8_t **)>bytes, ret);
> +}
> +de->VL = (i - 1) * 2;
> +return 0;
> +}
> +
> +static int read_seq(AVFormatContext *s, DataElement *de) {
> +int i = 0, ret;
> +DICOMContext *dicom = s->priv_data;
> +DataElement *seq_data = (DataElement *)av_malloc(sizeof(DataElement) *
> MAX_SEQ_LENGTH);
This is an array, right? There's a special av_malloc*() function for
allocating an array. (And the typecast is not required, IIUC.)
Furthermore, it is never freed.
Fixing free_de() as suggested alone is not sufficient either, the array
elements remain unfreed - those allocated here as seq_data[i]:
> +de->bytes = seq_data;
> +dicom->inseq = 1;
> +for (;i < MAX_SEQ_LENGTH; ++i) {
> +seq_data[i].bytes = NULL;
> +seq_data[i].desc = NULL;
> +seq_data[i].is_found = 0;
> +read_de_metainfo(s, seq_data + i);
> +if (seq_data[i].GroupNumber == SEQ_GR_NB
> +&& seq_data[i].ElementNumber == SEQ_DEL_EL_NB){
> +ret = i;
> +goto exit;
> +}
> +if (seq_data[i].VL == UNDEFINED_VL)
> +ret = read_implicit_seq_item(s, seq_data + i);
^ Here.
> +else
> +ret = read_de(s, seq_data + i);
> +if (ret < 0)
> +goto exit;
> +}
valgrind extract:
==20223== 10,000 bytes in 1 blocks are definitely lost in loss record 16 of 16
==20223==at 0x483AE4C: memalign (vg_replace_malloc.c:908)
==20223==by 0x483AF59: posix_memalign (vg_replace_malloc.c:1072)
==20223==by 0x5146F9: av_malloc (mem.c:87)
==20223==by 0x454C80: read_implicit_seq_item (dicomdec.c:371)
==20223==by 0x454C80: read_seq (dicomdec.c:410)
==20223==by 0x454C80: read_de_valuefield (dicomdec.c:424)
==20223==by 0x455312: dicom_read_packet (dicomdec.c:549)
==20223==by 0x4649C3: ff_read_packet (utils.c:856)
==20223==by 0x46594B: read_frame_internal (utils.c:1582)
==20223==by 0x46A001: avformat_find_stream_info (utils.c:3781)
==20223==by 0x4170BE: open_input_file (ffmpeg_opt.c:1126)
==20223==by 0x418A95: open_files (ffmpeg_opt.c:3275)
==20223==by 0x418A95: ffmpeg_parse_options (ffmpeg_opt.c:3315)
==20223==by 0x411875: main (ffmpeg.c:4872)
Furthermore, all your samples 000*.dcm give me the following error:
[dicom @ 0xfc26c0] Could not find codec parameters for stream 0 (Video: dicom,
none): unspecified size
Consider increasing the value for the 'analyzeduration' and 'probesize' options
Example:
[barsnick@paradise ffmpeg]$ ./ffmpeg_g -i 28.dcm -f null -
ffmpeg version N-94609-g406b3e902f Copyright (c) 2000-2019 the FFmpeg developers
built with gcc 8 (GCC)
configuration: --disable-doc --disable-everything --disable-network
--disable-vdpau --enable-indev=lavfi --enable-demuxer=dicom
--enable-muxer='null,hash,framehash,md5,framemd5'
--enable-encoder='wrapped_avframe,rawvideo,pcm_s16le'
--enable-decoder='rawvideo,pcm_f64le,dicom'
--enable-filter='color,testsrc,anoisesrc,null,aresample'
--enable-protocol='pipe,file'
libavutil 56. 33.100 / 56. 33.100
libavcodec 58. 56.100 / 58. 56.100
libavformat58. 32.101 / 58. 32.101
libavdevice58. 9.100 / 58. 9.100
libavfilter 7. 58.100 / 7. 58.100
libswscale 5. 6.100 / 5. 6.100
libswresample 3. 6.100 / 3. 6.100
[dicom @ 0xfc76c0] Could not find codec parameters for stream 0 (Video: dicom,
none): unspecified size
Consider increasing the value for the 'analyzeduration' and 'probesize' options
Input #0, dicom, from '28.dcm':
Metadata:
(0002,0001) File Meta Information Version: [Binary data]
(0002,0002) Media Storage SOP Class UID: 1.2.840.10008.5.1.4.1.1.2
(0002,0003) Media Storage SOP Inst UID:
1.3.6.1.4.1.14519.5.2.1.7014.4598.325035o92754234010375598120031
(0002,0010) Transfer Syntax UID: 1.2.840.10008.1.2.1
(0002,0012) Implementation Class UID: 1.2.40.0.13.1.1.1
(0002,0013) Implementation Version Name: dcm4che-1.4.35
Duration: N/A, start: 0.00, bitrate: N/A
Stream #0:0: Video: dicom, none, 1k tbr, 1k tbn
Output #0, null, to 'pipe:':
Output file #0 does not contain any stream
Having compiled the demuxer and decoder, I do now understand that DICOM
images can have multiple "frames".
Re: [FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
On Tue, Aug 20, 2019 at 20:53:43 +0530, Shivam wrote:
> Please review,
(Untested, just visual code inspection.)
> +- DICOM decoder and demxer
Typo -> demuxer. Also, when splitting the commits, also split the
changes to the Changelog. (Can still be one line eventually.)
> +.props = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_LOSSLESS,
Possibly also AV_CODEC_PROP_INTRA_ONLY? (PNG doesn't have that either,
but other still image formats do.) Is DICOM a still image format, or
does it have multiple images and a sense of I-frames?
> +static int extract_ed(AVCodecContext *avctx)
The return value is never used anywhere.
> +int ed_s = avctx->extradata_size;
Feel free to name the variable with something containing "size", makes
the code somewhat easier to understand.
> +static uint8_t apply_transfm(int64_t val, int64_t bitmask, int pix_pad,
^ I see no reason to save two letters in this
name. ;-)
> +static int decode_mono( AVCodecContext *avctx,
> +const uint8_t *buf,
> +AVFrame *p)
^ spurious space
> +switch (bitsalloc) {
> +case 8:
ffmpeg uses the same indentation level for "case" as for "switch".
> +for (i = 0; i < size; i++) {
> +if (pixrep)
> +pix = (int8_t)bytestream_get_byte();
> +else
> +pix = (uint8_t)bytestream_get_byte();
> +out[i] = pix;
> +}
What is this doing? Is the cast to uint8_t an implicit "abs()"?
Could it just be:
pix = pixrep ? bytestream_get_byte() :
FFABS(bytestream_get_byte());
out[i] = ...
Or in a somewhat different style:
uintXY_t val = bytestream_get_byte();
pix = pixrep ? byte : FFABS(byte);
out[i] = ...
> +default:
> +av_log(avctx, AV_LOG_ERROR, "Bits allocated %d not supported\n",
> bitsalloc);
> +return AVERROR_INVALIDDATA;
avctx->bits_per_coded_sample is a constant per file, right?
This "default" could be avoided if avctx->bits_per_coded_sample were
checked in init(), not in decode_frame().
> +av_log(avctx, AV_LOG_ERROR, "Required buffer size is %d but recieved
> only %d\n", req_buf_size, buf_size);
typo: received
> +void* bytes;
void *bytes
> +char* desc; // description (from dicom dictionary)
char *desc;
> +const uint8_t *d = p->buf;
> +
> +if (d[DICOM_PREAMBLE_SIZE] == 'D' &&
> +d[DICOM_PREAMBLE_SIZE + 1] == 'I' &&
> +d[DICOM_PREAMBLE_SIZE + 2] == 'C' &&
> +d[DICOM_PREAMBLE_SIZE + 3] == 'M') {
> +return AVPROBE_SCORE_MAX;
Would:
if (!strncmp(p->buf, "DICM", 4)
also work? Seems much simpler to me. (Also skipping the intermediate
"d" variable.)
> +if (de->bytes)
> +av_free(de->bytes);
> +if (de->desc)
> +av_free(de->desc);
As Michael said, av_free() includes the NULL checks already.
Additionally, I believe the use of av_freep() is preferred for these
pointers. (Provokes a segfault on use after free, instead of "silently"
writing into / reading from that memory.)
> +// detects transfer syntax
> +static TransferSyntax get_transfer_sytax (const char *ts) {
^ typo: syntax
Could you also please not name the variable "ts", as that already has
too many other meanings. ;-) (Use e.g. "syntax".)
> +static void set_dec_extradata(AVFormatContext *s, AVStream *st) {
> +DICOMContext *dicom = s->priv_data;
> +uint8_t *edp;
> +int size = 20 + AV_INPUT_BUFFER_PADDING_SIZE;
> +
> +st->codecpar->extradata = (uint8_t *)av_malloc(size);
> +edp = st->codecpar->extradata;
> +bytestream_put_le32(>codecpar->extradata, dicom->interpret);
> +bytestream_put_le32(>codecpar->extradata, dicom->pixrep);
> +bytestream_put_le32(>codecpar->extradata, dicom->pixpad);
> +bytestream_put_le32(>codecpar->extradata, dicom->slope);
> +bytestream_put_le32(>codecpar->extradata, dicom->intcpt);
> +st->codecpar->extradata = edp;
> +st->codecpar->extradata_size = size;
> +}
I'm not sure you're doing anything with edp here. Did you mean to use:
bytestream_put_le32(, dicom->interpret);
?
> +sprintf(key, "(%04x,%04x) %s", de->GroupNumber, de->ElementNumber,
> de->desc);
As Michael said, this can overflow "key". *Always* use snprintf.
> +switch(de->VR) {
> +case AT:
Indentation.
> +static int set_multiframe_data(AVFormatContext *s, AVStream* st, DataElement
> *de)
> +{
> +DICOMContext *dicom = s->priv_data;
> +
> +if (de->GroupNumber != MF_GR_NB)
> +return 0;
> +
> +switch (de->ElementNumber) {
> +case 0x1063: // frame time
> +dicom->delay = conv_DS(de);
> +dicom->duration = dicom->delay * dicom->nb_frames;
> +break;
> +}
> +return 0;
> +}
Always returns 0.
Is this a switch/case, so that it can be
Re: [FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
On Tue, Aug 20, 2019 at 08:53:43PM +0530, Shivam wrote:
> Sorry, for my previous mail, i forgot to attach the patch.
>
> The patch contains support for Dicom files. The below features are supported
> yet:-
> Uncompressed DICOM files using any of the Implicit and Explicit VR formats.
> Multiframe files are also supported.
> To extract the metadata or info about the procedure, I have added an option
> "-metadata." (The tags present in Dicom dictionary would be demuxed).
>
> I have also uploaded DICOM samples here.
> https://1drv.ms/u/s!AosJ9_SrP4Tsh2WoPfQAI8cSsqUs?e=ZMd5fR
>
> Please review,
>
> Thank you,
> Shivam Goyal
>
> Changelog|1
> libavcodec/Makefile |1
> libavcodec/allcodecs.c |1
> libavcodec/avcodec.h |1
> libavcodec/codec_desc.c |7
> libavcodec/dicom.c | 189
> libavcodec/version.h |2
> libavformat/Makefile |1
> libavformat/allformats.c |1
> libavformat/dicom.h | 108 +++
> libavformat/dicomdec.c | 594 ++
> libavformat/dicomdict.c | 716
> +++
> libavformat/version.h|2
> 13 files changed, 1622 insertions(+), 2 deletions(-)
> d47b7ad6a9f16ce4e29a67a99800183f9056062d add_dicom.patch
> From cd7ebe834278b29b0429b3f5b377154490ee159f Mon Sep 17 00:00:00 2001
> From: Shivam Goyal <1998.goyal.shi...@gmail.com>
> Date: Tue, 20 Aug 2019 20:03:02 +0530
> Subject: [PATCH] lavc: add DICOM decoder, lavf: add DICOM demuxer
>
> ---
> Changelog| 1 +
> libavcodec/Makefile | 1 +
> libavcodec/allcodecs.c | 1 +
> libavcodec/avcodec.h | 1 +
> libavcodec/codec_desc.c | 7 +
> libavcodec/dicom.c | 189 +++
> libavcodec/version.h | 2 +-
> libavformat/Makefile | 1 +
> libavformat/allformats.c | 1 +
> libavformat/dicom.h | 108 ++
> libavformat/dicomdec.c | 594
> libavformat/dicomdict.c | 716 +++
> libavformat/version.h| 2 +-
libavformat and libavcodec changes should be in seperate patches
> 13 files changed, 1622 insertions(+), 2 deletions(-)
> create mode 100644 libavcodec/dicom.c
> create mode 100644 libavformat/dicom.h
> create mode 100644 libavformat/dicomdec.c
> create mode 100644 libavformat/dicomdict.c
>
> diff --git a/Changelog b/Changelog
> index c1f1237770..e04c3aa5f6 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -4,6 +4,7 @@ releases are sorted from youngest to oldest.
> version :
> - Intel QSV-accelerated MJPEG decoding
> - Intel QSV-accelerated VP9 decoding
> +- DICOM decoder and demxer
>
> 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,
> +
[FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
Sorry, for my previous mail, i forgot to attach the patch.
The patch contains support for Dicom files. The below features are
supported yet:-
Uncompressed DICOM files using any of the Implicit and Explicit VR formats.
Multiframe files are also supported.
To extract the metadata or info about the procedure, I have added an
option "-metadata." (The tags present in Dicom dictionary would be demuxed).
I have also uploaded DICOM samples here.
https://1drv.ms/u/s!AosJ9_SrP4Tsh2WoPfQAI8cSsqUs?e=ZMd5fR
Please review,
Thank you,
Shivam Goyal
>From cd7ebe834278b29b0429b3f5b377154490ee159f Mon Sep 17 00:00:00 2001
From: Shivam Goyal <1998.goyal.shi...@gmail.com>
Date: Tue, 20 Aug 2019 20:03:02 +0530
Subject: [PATCH] lavc: add DICOM decoder, lavf: add DICOM demuxer
---
Changelog| 1 +
libavcodec/Makefile | 1 +
libavcodec/allcodecs.c | 1 +
libavcodec/avcodec.h | 1 +
libavcodec/codec_desc.c | 7 +
libavcodec/dicom.c | 189 +++
libavcodec/version.h | 2 +-
libavformat/Makefile | 1 +
libavformat/allformats.c | 1 +
libavformat/dicom.h | 108 ++
libavformat/dicomdec.c | 594
libavformat/dicomdict.c | 716 +++
libavformat/version.h| 2 +-
13 files changed, 1622 insertions(+), 2 deletions(-)
create mode 100644 libavcodec/dicom.c
create mode 100644 libavformat/dicom.h
create mode 100644 libavformat/dicomdec.c
create mode 100644 libavformat/dicomdict.c
diff --git a/Changelog b/Changelog
index c1f1237770..e04c3aa5f6 100644
--- a/Changelog
+++ b/Changelog
@@ -4,6 +4,7 @@ releases are sorted from youngest to oldest.
version :
- Intel QSV-accelerated MJPEG decoding
- Intel QSV-accelerated VP9 decoding
+- DICOM decoder and demxer
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 00..eaa378d944
--- /dev/null
+++ b/libavcodec/dicom.c
@@ -0,0 +1,189 @@
+/*
+ * 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
