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 <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 _______________________________________________ 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".