[FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder
Shivam
shivgo at iitk.ac.in
Thu Aug 22 08:44:43 EEST 2019
On 8/21/19 7:27 PM, Moritz Barsnick wrote:
> On Tue, Aug 20, 2019 at 20:53:43 +0530, Shivam wrote:
>> Please review,
> (Untested, just visual code inspection.)
>
>> +- DICOM decoder and demxer
> Typo -> demuxer. Also, when splitting the commits, also split the
> changes to the Changelog. (Can still be one line eventually.)
>
>> + .props = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_LOSSLESS,
> Possibly also AV_CODEC_PROP_INTRA_ONLY? (PNG doesn't have that either,
> but other still image formats do.) Is DICOM a still image format, or
> does it have multiple images and a sense of I-frames?
>
>> +static int extract_ed(AVCodecContext *avctx)
> The return value is never used anywhere.
>
>> + int ed_s = avctx->extradata_size;
> Feel free to name the variable with something containing "size", makes
> the code somewhat easier to understand.
>
>> +static uint8_t apply_transfm(int64_t val, int64_t bitmask, int pix_pad,
> ^ I see no reason to save two letters in this name. ;-)
>
>> +static int decode_mono( AVCodecContext *avctx,
>> + const uint8_t *buf,
>> + AVFrame *p)
> ^ spurious space
>
>> + switch (bitsalloc) {
>> + case 8:
> ffmpeg uses the same indentation level for "case" as for "switch".
>
>> + for (i = 0; i < size; i++) {
>> + if (pixrep)
>> + pix = (int8_t)bytestream_get_byte(&buf);
>> + else
>> + pix = (uint8_t)bytestream_get_byte(&buf);
>> + out[i] = pix;
>> + }
> What is this doing? Is the cast to uint8_t an implicit "abs()"?
> Could it just be:
> pix = pixrep ? bytestream_get_byte(&buf) : FFABS(bytestream_get_byte(&buf));
> out[i] = ...
>
> Or in a somewhat different style:
> uintXY_t val = bytestream_get_byte(&buf);
> pix = pixrep ? byte : FFABS(byte);
> out[i] = ...
>
>> + default:
>> + av_log(avctx, AV_LOG_ERROR, "Bits allocated %d not supported\n", bitsalloc);
>> + return AVERROR_INVALIDDATA;
> avctx->bits_per_coded_sample is a constant per file, right?
> This "default" could be avoided if avctx->bits_per_coded_sample were
> checked in init(), not in decode_frame().
>
>> + av_log(avctx, AV_LOG_ERROR, "Required buffer size is %d but recieved only %d\n", req_buf_size, buf_size);
> typo: received
>
>> + void* bytes;
> void *bytes
>
>> + char* desc; // description (from dicom dictionary)
> char *desc;
>
>> + const uint8_t *d = p->buf;
>> +
>> + if (d[DICOM_PREAMBLE_SIZE] == 'D' &&
>> + d[DICOM_PREAMBLE_SIZE + 1] == 'I' &&
>> + d[DICOM_PREAMBLE_SIZE + 2] == 'C' &&
>> + d[DICOM_PREAMBLE_SIZE + 3] == 'M') {
>> + return AVPROBE_SCORE_MAX;
> Would:
> if (!strncmp(p->buf, "DICM", 4)
> also work? Seems much simpler to me. (Also skipping the intermediate
> "d" variable.)
>
>> + if (de->bytes)
>> + av_free(de->bytes);
>> + if (de->desc)
>> + av_free(de->desc);
> As Michael said, av_free() includes the NULL checks already.
> Additionally, I believe the use of av_freep() is preferred for these
> pointers. (Provokes a segfault on use after free, instead of "silently"
> writing into / reading from that memory.)
>
>> +// detects transfer syntax
>> +static TransferSyntax get_transfer_sytax (const char *ts) {
> ^ typo: syntax
>
> Could you also please not name the variable "ts", as that already has
> too many other meanings. ;-) (Use e.g. "syntax".)
>
>> +static void set_dec_extradata(AVFormatContext *s, AVStream *st) {
>> + DICOMContext *dicom = s->priv_data;
>> + uint8_t *edp;
>> + int size = 20 + AV_INPUT_BUFFER_PADDING_SIZE;
>> +
>> + st->codecpar->extradata = (uint8_t *)av_malloc(size);
>> + edp = st->codecpar->extradata;
>> + bytestream_put_le32(&st->codecpar->extradata, dicom->interpret);
>> + bytestream_put_le32(&st->codecpar->extradata, dicom->pixrep);
>> + bytestream_put_le32(&st->codecpar->extradata, dicom->pixpad);
>> + bytestream_put_le32(&st->codecpar->extradata, dicom->slope);
>> + bytestream_put_le32(&st->codecpar->extradata, dicom->intcpt);
>> + st->codecpar->extradata = edp;
>> + st->codecpar->extradata_size = size;
>> +}
> I'm not sure you're doing anything with edp here. Did you mean to use:
> bytestream_put_le32(&edp, dicom->interpret);
> ?
>
>> + sprintf(key, "(%04x,%04x) %s", de->GroupNumber, de->ElementNumber, de->desc);
> As Michael said, this can overflow "key". *Always* use snprintf.
>
>> + switch(de->VR) {
>> + case AT:
> Indentation.
>
>> +static int set_multiframe_data(AVFormatContext *s, AVStream* st, DataElement *de)
>> +{
>> + 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;
>> + }
>> + return 0;
>> +}
> Always returns 0.
>
> Is this a switch/case, so that it can be expanded in the future?
>
>> + av_log(s, AV_LOG_WARNING,"Data Element Value length: %ld can't be odd\n", de->VL);
> de->VL is int64_t, so the correct printf format is "%" PRIi64.
>
>> + if (de->VL < 0)
>> + return AVERROR_INVALIDDATA;
> You could put this check before the odd check.
>
>> +static int read_implicit_seq_item(AVFormatContext *s, DataElement *de)
>> +{
>> + int ret, f = -2, i = 0;
>> +
>> + de->bytes = av_malloc(MAX_UNDEF_LENGTH);
>> + for(; i < MAX_UNDEF_LENGTH; ++i) {
> Unusual to initialize the "int i" above and not here, but okay.
>
>> + if (ret == SEQ_GR_NB)
>> + f = i;
>> + if (ret == SEQ_ITEM_DEL_EL_NB && f == i - 1) {
> else if
>
>> +static int read_seq(AVFormatContext *s, DataElement *de) {
>> + int i = 0, ret;
>> + DICOMContext *dicom = s->priv_data;
>> + DataElement *seq_data = (DataElement *)av_malloc(sizeof(DataElement) * MAX_SEQ_LENGTH);
>> + de->bytes = seq_data;
>> + dicom->inseq = 1;
>> + for (;i < MAX_SEQ_LENGTH; ++i) {
> Unusual to initialize the "int i" above and not here, but okay.
>
>> + 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){
> Nit: Missing space before curly bracket.
>
>> + ret = i;
>> + goto exit;
>> + }
>> + if (seq_data[i].VL == UNDEFINED_VL)
>> + ret = read_implicit_seq_item(s, seq_data + i);
>> + else
>> + ret = read_de(s, seq_data + i);
>> + if (ret < 0)
>> + goto exit;
>> + }
>> +
>> +exit:
> How about just "break" instead of "goto exit", and drop the label? If
> this is the only use of the label, goto is too confusing.
>
>> +static int read_de_valuefield(AVFormatContext *s, DataElement *de) {
>> + if (de->VL == UNDEFINED_VL)
>> + return read_seq(s, de);
>> + else return read_de(s, de);
> Line break after else, please.
>
>> + ret = read_de_metainfo(s, de);
>> + ret = read_de_valuefield(s, de);
>> + if (ret < 0) {
>> + free_de(de);
>> + return ret;
>> + }
> The first assignment of ret ("ret = read_de_metainfo(s, de);") is
> ignored. It can even return an error.
>
>> + av_log(s, AV_LOG_WARNING,"First data element is not \'File MetaInfo Group Length\', may fail to demux");
> Missing space after ",". I believe the "'" shouldn't be escaped. And
> missing a "\n" at the end.
>
>> + bytes_read += ret;
>> + value = get_val_str(de);
> This allocates a pointer via get_val_str()...
>
>> + if (de->GroupNumber == TS_GR_NB && de->ElementNumber == TS_EL_NB)
>> + dicom->transfer_syntax = get_transfer_sytax(value);
>> +
>> + av_dict_set(m, key, value, AV_DICT_DONT_STRDUP_KEY | AV_DICT_DONT_STRDUP_VAL);
>> + free_de(de);
> ... but doesn't free it.
>
>
>> + if (dicom->frame_nb > 1 && dicom->frame_nb <= dicom->nb_frames)
>> + goto inside_pix_data;
> What's the effect (and readability) of jumping into the inside of a
> for() loop's block? Can't this be made more readable and logical?
> (Sorry, don't have a suggestion, just very confused by this code.)
>
>> + ret = read_de_metainfo(s,de);
>> + if (ret < 0)
>> + goto exit;
> Again, since this is the body of a for() loop, wouldn't it be correct
> to use "break" instread of "goto", achieving the same jump? "goto" is
> used to avoid code duplication, not to avoid standard C code style. ;-)
>
>> + av_packet_unref(pkt);
>> + goto exit;
> Same for allthe other "exit"s, presumably.
>
>> + if (ret < 0)
>> + goto end_de;
> This on the other hand may be legitimate.
>
>> + end_de:
>> + free_de(de);
>> + }
>> +exit:
>> + free_de(de);
>> + if (ret < 0)
>> + return ret;
>> + return AVERROR_EOF;
>> +}
>> +
>> +static const AVOption options[] = {
>> + { "window", "override default window found in file", offsetof(DICOMContext, window), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 99999, AV_OPT_FLAG_DECODING_PARAM },
>> + { "level", "override default level found in file", offsetof(DICOMContext, level), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 99999, AV_OPT_FLAG_DECODING_PARAM },
>> + { "metadata", "skip or decode metadata", offsetof(DICOMContext, metadata) , AV_OPT_TYPE_BOOL,{.i64 = 0}, 0, 1, AV_OPT_FLAG_DECODING_PARAM },
> "true" is skip, "false" is decode? Or vice versa? Better just say what
> happens when true "whether to decode metadata". (The default is
> automatically documented here.)
>
> Apropros: You should add documentation, also separately for demuxer and
> decoder (but in their respective commits).
>
>> +static const AVClass dicom_class = {
>> + .class_name = "dicomdec",
> Looking at the other demuxers, this should probably be "DICOM demuxer".
>
>> +static int dcm_dict_comp(const void *vde, const void *vDd) {
>> + DataElement *de = (DataElement *) vde;
>> + DICOMDictionary *Dd = (DICOMDictionary *) vDd;
>> +
>> + if (de->GroupNumber > Dd->GroupNumber)
>> + return 1;
>> + else if (de->GroupNumber < Dd->GroupNumber)
>> + return -1;
>> + else {
>> + if (de->ElementNumber > Dd->ElementNumber)
>> + return 1;
>> + else if (de->ElementNumber < Dd->ElementNumber)
>> + return -1;
>> + else
>> + return 0;
> We have the FFDIFFSIGN() macro for this.
> :-)
>
> This should work:
>
> int ret;
> ret = FFDIFFSIGN(de->GroupNumber, Dd->GroupNumber);
> if (!ret)
> ret = FFDIFFSIGN(de->ElementNumber, Dd->ElementNumber);
> return ret;
>
>> + if (!de->GroupNumber || !de->ElementNumber)
>> + return -2;
> This is eventually returned by dicom_read_packet(). What is this "-2"
> interpreted as by the caller of this .read_packet callback?
>
>> +}
>> \ No newline at end of file
> Please add a newline to the last line of this file. ;-)
Ok, i will apply all the improvements.
Thanks for the review
Shivam Goyal
More information about the ffmpeg-devel
mailing list