On 8/27/19 2:05 AM, Moritz Barsnick wrote:
On Sun, Aug 25, 2019 at 03:22:02 +0530, Shivam wrote:
The patch contains DICOM demuxer. I have improved the code as suggested.
Second part of my review:
From: Shivam Goyal <1998.goyal.shi...@gmail.com>
Date: Sun, 25 Aug 2019 02:57:35 +0530
Subject: [PATCH] lavf: Add DICOM demuxer
---
Changelog | 1 +
libavformat/Makefile | 1 +
libavformat/allformats.c | 1 +
libavformat/dicom.h | 109 ++++++
libavformat/dicomdec.c | 617 +++++++++++++++++++++++++++++++++
libavformat/dicomdict.c | 711 +++++++++++++++++++++++++++++++++++++++
libavformat/version.h | 2 +-
7 files changed, 1441 insertions(+), 1 deletion(-)
create mode 100644 libavformat/dicom.h
create mode 100644 libavformat/dicomdec.c
create mode 100644 libavformat/dicomdict.c
You still need to document the options in doc/*.texi.
Ok, i would add this.
diff --git a/Changelog b/Changelog
index 52096eed0e..5e5a8c5c6c 100644
--- a/Changelog
+++ b/Changelog
@@ -5,6 +5,7 @@ version <next>:
- v360 filter
- Intel QSV-accelerated MJPEG decoding
- Intel QSV-accelerated VP9 decoding
+- DICOM demuxer
Here, this patch doesn't apply in top of the DICOM decoder, even though
it requires the decoder, because the decoder patch already adds another
line to the Changelog.
--- /dev/null
+++ b/libavformat/dicomdec.c
[...]
+static void free_seq(DataElement *de) {
+ int i = 0;
+ DataElement *seq_data = de->bytes;
+ for(; i < MAX_UNDEF_LENGTH; ++i) {
BTW, ffmpeg prefers the "i++" style.
ok , would change this
[...]
+// detects transfer syntax
+static TransferSyntax get_transfer_sytax (const char *ts) {
There's still a typo in the name of the function, sytax -> syntax.
Please fix.
Ok,l would change this
[...]
+static int set_multiframe_data(AVFormatContext *s, AVStream* st, DataElement
*de)
+{
The return value still is always 0 and isn't being used for anything.
(I'm saying, this function could return void, unless it can be expanded
to something more later.)
Ok, would change this too
+ 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;
+ }
Again, here, I expect this to be a switch/case with one case only if it
can be expanded later, i.e. de->ElementNumber has multiple meanings
which aren't covered here.
This would be expanded in my next patch, so, i thought this may be correct.
+ return 0;
+}
+
+
+static int read_de_metainfo(AVFormatContext *s, DataElement *de)
+{
[...]
+ if (de->VL < 0)
+ return AVERROR_INVALIDDATA;
+ if (de->VL != UNDEFINED_VL && de->VL % 2)
+ av_log(s, AV_LOG_WARNING,"Data Element Value length: %"PRIi64" can't be
odd\n", de->VL);
^
Still missing a space here.
Ok, I would change this
+ return bytes_read;
+}
+
+static int read_de(AVFormatContext *s, DataElement *de)
+{
+ int ret;
+ uint32_t len = de->VL;
+ de->bytes = av_malloc(len);
+ ret = avio_read(s->pb, de->bytes, len);
+ return ret;
+}
+
+static int read_implicit_seq_item(AVFormatContext *s, DataElement *de)
+{
+ int ret, f = -2, i = 0;
+ uint8_t *bytes = de->bytes;
+
+ bytes = av_malloc(MAX_UNDEF_LENGTH);
You're still not checking the return value and returning an error on
failure.
+ for(; i < MAX_UNDEF_LENGTH; ++i) {
ffmpeg prefers the "i++" style.
[...]
+static int read_seq(AVFormatContext *s, DataElement *de) {
+ int i = 0, ret;
+ DICOMContext *dicom = s->priv_data;
+ DataElement *seq_data = av_malloc_array(MAX_SEQ_LENGTH,
sizeof(DataElement));
+
+ de->bytes = seq_data;
+ dicom->inseq = 1;
+ for (;i < MAX_SEQ_LENGTH; ++i) {
ffmpeg prefers the "i++" style. (And missing a space after the first
semicolon.)
+ 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;
+ break;
+ }
+ if (seq_data[i].VL == UNDEFINED_VL)
+ ret = read_implicit_seq_item(s, seq_data + i);
I believe these array elements are still not freed.
These should be freed as i have debugged it and changed the free_de()
function, it detects if the DE is an array or not and handles it that way.
+ else
+ ret = read_de(s, seq_data + i);
+ if (ret < 0)
+ break;
+ }
+
+ dicom->inseq = 0;
+ return ret;
+}
[...]
+static int dicom_read_header(AVFormatContext *s)
+{
+ AVIOContext *pb = s->pb;
+ AVDictionary **m = &s->metadata;
+ DICOMContext *dicom = s->priv_data;
+ DataElement *de;
+ char *key, *value;
+ uint32_t header_size, bytes_read = 0;
+ int ret;
+
+ ret = avio_skip(pb, DICOM_PREAMBLE_SIZE + DICOM_PREFIX_SIZE);
+ if (ret < 0)
+ return ret;
+ dicom->inheader = 1;
+ de = alloc_de();
+ if (!de)
+ return AVERROR(ENOMEM);
+ ret = read_de_metainfo(s, de);
+ if (ret < 0) {
+ free_de(de);
+ return ret;
+ }
+
+ ret = read_de_valuefield(s, de);
+ if (ret < 0) {
+ free_de(de);
+ return ret;
+ }
+ if (de->GroupNumber != 0x02 || de->ElementNumber != 0x00) {
+ av_log(s, AV_LOG_WARNING, "First data element is not File MetaInfo Group
Length, may fail to demux\n");
+ header_size = 200; // Fallback to default length
+ } else
+ header_size = AV_RL32(de->bytes);
+
+ free_de(de);
+ while (bytes_read < header_size) {
+ de = alloc_de();
This can fail, like 20 lines above, so you also need to check for error
and return:
if (!de)
return AVERROR(ENOMEM);
Ok, would change this too
+ ret = read_de_metainfo(s, de);
+ if (ret < 0) {
+ free_de(de);
+ return ret;
+ }
+ bytes_read += ret;
+ dicom_dict_find_elem_info(de);
+ key = get_key_str(de);
+ ret = read_de_valuefield(s, de);
+ if (ret < 0) {
+ free_de(de);
+ return ret;
+ }
+ bytes_read += ret;
+ value = get_val_str(de);
a) get_val_str() tries to allocate memory, which may fail. Its return
value needs to be checked, and you need to return on failure.
b) This call of get_val_str() allocates memory assigned to "value", so
you need to av_free "value" on every possible exit path of this function.
I am using the av_dict_set function with "AV_DICT_DONT_STRDUP_KEY" below
so, should i still need to free the key and value at the bottom.
(I mean i understood that i should free them on every error or Invalid
data.)
+ if (de->GroupNumber == TS_GR_NB && de->ElementNumber == TS_EL_NB) {
+ dicom->transfer_syntax = get_transfer_sytax(value);
+ if (dicom->transfer_syntax == UNSUPPORTED_TS) {
+ free_de(de);
+ av_log(s, AV_LOG_ERROR, "Provided Transfer syntax is not
supported\n");
+ return AVERROR_INVALIDDATA;
+ }
+ }
+
+ av_dict_set(m, key, value, AV_DICT_DONT_STRDUP_KEY |
AV_DICT_DONT_STRDUP_VAL);
+ free_de(de);
+ }
+ set_context_defaults(dicom);
+ s->ctx_flags |= AVFMTCTX_NOHEADER;
+ s->start_time = 0;
+ return 0;
+}
+
+static int dicom_read_packet(AVFormatContext *s, AVPacket *pkt)
+{
[...]
+ inside_pix_data:
+ if (av_new_packet(pkt, dicom->frame_size) < 0)
+ return AVERROR(ENOMEM);
+ 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);
+ break;
+ }
+ dicom->frame_nb ++;
+ return ret;
The pkt still needs to be av_packet_unref()'d here, I believe
should the packet needs to cleaned after filling the pixel data inside it ?
[...]
diff --git a/libavformat/version.h b/libavformat/version.h
index 57002d25c8..c8f27cebf9 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -32,7 +32,7 @@
// Major bumping may affect Ticket5467, 5421, 5451(compatibility with
Chromium)
// Also please add any ticket numbers that you believe might be affected here
#define LIBAVFORMAT_VERSION_MAJOR 58
-#define LIBAVFORMAT_VERSION_MINOR 31
+#define LIBAVFORMAT_VERSION_MINOR 32
#define LIBAVFORMAT_VERSION_MICRO 102
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.
Ok,
Thanks for the review
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-requ...@ffmpeg.org with subject "unsubscribe".
