[FFmpeg-devel] lavf: Add DICOM demuxer

[Moritz Barsnick]

On Wed, Aug 28, 2019 at 00:44:24 +0530, Shivam wrote:
> > Again, here, I expect this to be a switch/case with one case only if it
> > can be expanded later, i.e. de->ElementNumber has multiple meanings
> > which aren't covered here.
> This would be expanded in my next patch, so, i thought this may be correct.

It's okay then, as far as I'm concerned.
> >> +        if (seq_data[i].VL == UNDEFINED_VL)
> >> +            ret = read_implicit_seq_item(s, seq_data + i);
> > I believe these array elements are still not freed.
> These should be freed as i have debugged it and changed the free_de()
> function, it detects if the DE is an array or not and handles it that way.

Okay, I didn't see that. Fine then.

> > a) get_val_str() tries to allocate memory, which may fail. Its return
> > value needs to be checked, and you need to return on failure.
> >
> > b) This call of get_val_str() allocates memory assigned to "value", so
> > you need to av_free "value" on every possible exit path of this function.
>
> I am using the av_dict_set function with "AV_DICT_DONT_STRDUP_KEY" below
> so, should i still need to free the key and value at the bottom.

I didn't see the use of AV_DICT_DONT_STRDUP_KEY. I assume it's fine
then.

> >> +            }
> >> +            dicom->frame_nb ++;
> >> +            return ret;
> > The pkt still needs to be av_packet_unref()'d here, I believe
> should the packet needs to cleaned after filling the pixel data inside it ?

Hmm, not sure.

> Thanks for the review

Please do make sure you get some other reviews. I'm not always 100 %
right, obviously, and not always 100 % sure.

Cheers,
Moritz