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