[FFmpeg-devel] lavf: Add DICOM demuxer
Shivam
shivgo at iitk.ac.in
Tue Aug 27 22:14:24 EEST 2019
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.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.
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
More information about the ffmpeg-devel
mailing list