On 8/27/19 2:04 AM, Moritz Barsnick wrote:
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.
I thought this may be used, I was thinking to extend the decoder. but i
think i should make this Void for now and should change it in my next
patch, if needed.
+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.
[...]
I will correct it
+// 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.
[...]
Would correct it too.
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").
I have changed those two memory leaks i would test it with valgrind on
000017.dcm, and would debug it as needed.
Thanks for the review
Shivam Goyal
_______________________________________________
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".
