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
_______________________________________________
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".
