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