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. ;-)
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".
