Re: [FFmpeg-devel] lavf: Add DICOM demuxer

[Moritz Barsnick]

On Wed, Aug 28, 2019 at 00:44:24 +0530, Shivam wrote:
> > 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.

It's okay then, as far as I'm concerned.
> >> +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.

Okay, I didn't see that. Fine then.

> > 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 didn't see the use of AV_DICT_DONT_STRDUP_KEY. I assume it's fine
then.

> >> +}
> >> +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 ?

Hmm, not sure.

> Thanks for the review

Please do make sure you get some other reviews. I'm not always 100 %
right, obviously, and not always 100 % sure.

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] lavf: Add DICOM demuxer

[Shivam]

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 :
  - 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 = >metadata;
+DICOMContext *dicom = s->priv_data;
+DataElement *de;
+char *key, *value;
+uint32_t header_size, bytes_read = 0;

Re: [FFmpeg-devel] lavf: Add DICOM demuxer

[Moritz Barsnick]

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.

> diff --git a/Changelog b/Changelog
> index 52096eed0e..5e5a8c5c6c 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -5,6 +5,7 @@ version :
>  - 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.

[...]
> +// 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.

[...]
> +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.)

> +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.

> +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.

> +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.

> +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 = >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)
> +