[FFmpeg-devel] [PATCH] lavf: Add DICOM demuxer, lavc: Add DICOM decoder

[Moritz Barsnick]

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