On Sun, Aug 25, 2019 at 03:20:13 +0530, Shivam wrote: > The patch contains DICOM decoder. I have improved the code as suggested.
Hi Shivan, nice job. First of all, all your samples now appear to demux and decode properly. I didn't get a chance to look at their output, but I guess you have that covered. Most of my review remarks have been integrated. You did miss some which I believe are essential: > +static int extract_ed(AVCodecContext *avctx) > +{ > + DICOMopts *dcmopts = avctx->priv_data; > + uint8_t *ed = avctx->extradata; > + int ed_size = avctx->extradata_size; > + const uint8_t **b = &ed; > + > + dcmopts->interpret = 0x02; // Set defaults > + dcmopts->slope = 1; > + dcmopts->intcpt = 0; > + dcmopts->pixpad = 1 << 31; > + dcmopts->pixrep = 0; > + > + if (ed_size < DECODER_ED_SIZE + AV_INPUT_BUFFER_PADDING_SIZE) > + return -1; This return value isn't used anywhere (or ignored). That's fine if that's intentional, but a bit confusing for review. > +static uint8_t apply_transform(int64_t val, int64_t bitmask, int pix_pad, > + int slope, int intercept, int w, int l, int > interpret) Indentation is now off in the second quoted line. [...] > +// DICOM MONOCHROME1 and MONOCHROME2 decoder > +static int decode_mono( AVCodecContext *avctx, > + const uint8_t *buf, > + AVFrame *p) There's still a space too much here, after "decode_mono(", and the subsequent two lines need to adjust as well. [...] > diff --git a/libavcodec/version.h b/libavcodec/version.h > index e70ebc0c70..b4b79ef63a 100644 > --- a/libavcodec/version.h > +++ b/libavcodec/version.h > @@ -28,7 +28,7 @@ > #include "libavutil/version.h" > > #define LIBAVCODEC_VERSION_MAJOR 58 > -#define LIBAVCODEC_VERSION_MINOR 55 > +#define LIBAVCODEC_VERSION_MINOR 56 > #define LIBAVCODEC_VERSION_MICRO 101 When bumping MINOR, you need to reset MICRO to 100. But this part doesn't apply anymore anyway, since MICRO has changed on master since. You may need to rebase, but this will likely also be done by whoever pushes to master once your patch is acknowledged. Apart from that, I still get a memory leak when decoding 000017.dcm (with just "valgrind ./ffmpeg_g -i DICOM-samples/000017.dcm"). 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".