[FFmpeg-devel] lavf: Add DICOM demuxer
Moritz Barsnick
barsnick at gmx.net
Mon Aug 26 23:35:16 EEST 2019
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.shivam at 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 <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.
[...]
> +// 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 = &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);
> + 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.
> + 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.
[...]
> 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.
Regards,
Moritz
More information about the ffmpeg-devel
mailing list