[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:

One more. Some more nitpicking around this. Please use valgrind with
all your samples and some demuxing and decoding command lines.

> +static int dicom_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    DICOMContext *dicom = s->priv_data;
> +    int metadata = dicom->metadata, ret;
> +    AVDictionary **st_md;
> +    char *key, *value;
> +    AVStream  *st;
> +    DataElement *de;
> +
> +    if (s->nb_streams < 1) {
> +        st = avformat_new_stream(s, NULL);
> +        if (!st)
> +            return AVERROR(ENOMEM);
> +        st->codecpar->codec_id   = AV_CODEC_ID_DICOM;
> +        st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> +        st->start_time = 0;
> +        avpriv_set_pts_info(st, 64, 1, 1000);
> +        if (dicom->window != -1)
> +            st->codecpar->profile = dicom->window;
> +        if (dicom->level != -1)
> +            st->codecpar->level = dicom->level;
> +    } else
> +        st = s->streams[0];
> +
> +    st_md = &st->metadata;
> +    dicom->inheader = 0;
> +    if (dicom->frame_nb > 1 && dicom->frame_nb <= dicom->nb_frames)
> +        goto inside_pix_data;
> +
> +    for (;;) {
> +        ret = avio_feof(s->pb);
> +        if (ret)
> +            return AVERROR_EOF;
> +
> +        de = alloc_de();

Allocation failure needs to be checked:
        if (!de)
            return AVERROR(ENOMEM);

> +        ret = read_de_metainfo(s,de);
> +        if (ret < 0)
> +            goto exit;
> +
> +        if (de->GroupNumber == PIXEL_GR_NB && de->ElementNumber == PIXELDATA_EL_NB) {
> +            dicom->frame_size = de->VL / dicom->nb_frames;
> +            set_dec_extradata(s, st);
> +        inside_pix_data:
> +            if (av_new_packet(pkt, dicom->frame_size) < 0)
> +                return AVERROR(ENOMEM);

This leaks de. Instead:
            if (av_new_packet(pkt, dicom->frame_size) < 0) {
                ret = AVERROR(ENOMEM);
                goto exit;
            }

> +            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);
> +                goto exit;
> +            }
> +            dicom->frame_nb ++;
> +            return ret;

This leaks everything (i.e. de and pkt). Instead:
            dicom->frame_nb ++;
            av_packet_unref(pkt);
            goto exit;


> +        } else if (de->GroupNumber == IMAGE_GR_NB || de->GroupNumber == MF_GR_NB) {
> +            ret = read_de_valuefield(s, de);
> +            if (ret < 0)
> +                goto exit;
> +            set_imagegroup_data(s, st, de);
> +            set_multiframe_data(s, st, de);
> +        } else {
> +            if (metadata || de->VL == UNDEFINED_VL)
> +                ret = read_de_valuefield(s, de);
> +            else
> +                ret = avio_skip(s->pb, de->VL); // skip other elements
> +            if (ret < 0)
> +                goto exit;
> +        }
> +        if (metadata) {
> +            ret = dicom_dict_find_elem_info(de);
> +            if (ret < 0)
> +                goto end_de;
> +            key = get_key_str(de);
> +            value = get_val_str(de);
> +            av_dict_set(st_md, key, value, AV_DICT_DONT_STRDUP_KEY | AV_DICT_DONT_STRDUP_VAL);
> +        }
> +    end_de:
> +        free_de(de);
> +    }
> +exit:
> +    free_de(de);
> +    if (ret < 0)
> +        return ret;
> +    return AVERROR_EOF;
> +}

And, as mentioned, "goto exit" can probably be a simple "break", but
that's up to you to verify that it still does the right thing.

Cheers,
Moritz